Date: Mon, 18 Feb 2002 22:26:43 -0800 (PST) From: Matthew Dillon <dillon@apollo.backplane.com> To: Robert Watson <rwatson@FreeBSD.ORG> Cc: arch@FreeBSD.ORG, jhb@FreeBSD.ORG Subject: Re: John Balwdin's proc-locking patch Message-ID: <200202190626.g1J6QhG58642@apollo.backplane.com> References: <Pine.NEB.3.96L.1020218193120.69361O-100000@fledge.watson.org>
next in thread | previous in thread | raw e-mail | index | archive | help
: :For review, John Baldwin's proc locking patch. : :Robert N M Watson FreeBSD Core Team, TrustedBSD Project :robert@fledge.watson.org NAI Labs, Safeport Network Services :... : :I've put up a diff of the jhb_proc branch here: :http://people.freebsd.org/~jake/jhb_proc.diff : :Jake Problem areas: * PROC locking for proc_rwmem(), aka: ... + PROC_LOCK(td->td_proc); return proc_rwmem(td->td_proc, &uio); This is rather odd. proc_rwmem() apparently now needs to have the proc locked on entry and will unlock it on return. Our VOP code did a lot of this sort of thing and the VOP code was massively confusing. So confusing, in fact, that there are *STILL* a couple of VOP's we haven't been able to unwind. At the very least proc_rwmem() should be documented and contain proper assertions to prevent confusion, but I'd prefer it if the proc lock could also be either internalized or externalized rather then the mix we have now. * Complexity in some of the ucred patches. For example, in kern/kern_descrip.c. The patch itself may be fine, but I would never want to see this committed at the same time everything else is. * Lack of use of Giant wrappers, such as in getuid() (though that is a fairly simple routine). The whole point of using Giant wrappers is to allow piecemeal commits which are able to test subsystem mutexes (e.g. with Witness turned on) without having to let go of Giant, thus keeping the system stable for other developers until all the piecemeal commits have been done and to allow the committer to test turning on and off Giant for a subsytem dynamically, at run-time. General comments: This patch is big, 310 KBytes, and touches a huge number of files. There are patches for half a dozen subsystems here. Now I can see something like Julian's first KSE patch needing to be separated out and committed all in one go because we have no other choice, but 85% of this particular patch by my reading could have been committed directly to the main tree in a piecemeal fashion without harming -current in the least. It is totally unecessary to accumulate so many easily-made-incremental changes off-tree and, in fact, I believe it to be detrimental to the project. It locks up large areas of code for long periods of time and creates both a synchronization hassle (even with P4) and a debugging nightmare when finally committed due to the sheer number of changes involved. - This is not a good development philosophy. I'm sorry, it just isn't. And here I am not blaming John so much as I am blaming the whole P4 philosophy. I may commit things faster, but I commit much smaller, more easily tested (and instrumented where necessary) chunks. Forward progress is made steadily. The commits go in faster because it takes far less time to test each one and if a problem arises, it is usually pretty damn obvious what piece is to blame. In our entire history there are only a handful of things that could be said to legitimately require a separate branch to develop. I can only think of two right off the bat: CAM and KSEs. In anycase, the areas of the patch related to what I was testing are almost identical, except I include instrumented Giant code and John simply removes Giant entirely. I prefer my code, frankly. I do not think it is a good idea to remove Giant entirely for even the simpler ucred/proc-related system calls at this time. That is my position. -Matt Matthew Dillon <dillon@backplane.com> To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-arch" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200202190626.g1J6QhG58642>