Date: Fri, 22 Feb 2002 21:34:55 -0800 (PST) From: Matthew Dillon <dillon@apollo.backplane.com> To: John Baldwin <jhb@FreeBSD.ORG> Cc: current@FreeBSD.ORG Subject: Re: RE: First (easy) td_ucred patch Message-ID: <200202230534.g1N5Ytr34236@apollo.backplane.com> References: <XFMail.020223000035.jhb@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
:> I'm currently testing the following patch whcih is a subset of the td_ucred :> changes. It involves no API changes, but only contains 2 basic changes: :> :> 1) We still need Giant when doing the crhold() to set td_ucred in :> cred_update_thread(). This is an old bug that is my fault. I knew that :> PROC_LOCK was sufficient yet which was my reason for not using td_ucred. :> However, we could still be derferencing a stale p_ucred and doing very bad :> things, so this needs to be fixed until p_ucred is fully protected by the :> PROC_LOCK. This also means that td_ucred is now safe to use. As such: :> :> 2) All the "easy" p->p_ucred -> td->td_ucred changes that don't involve the :> changes to API's such as suser() and p_canfoo(). The next patch in this :> series will most likely be the suser() API change. :> :> http://www.FreeBSD.org/~jhb/patches/ucred.patch Well, I have some issues with this patch. It seems to include a number of structural changes ranging from the removal of braces (syntactical changes) to straightforward but major flow changes such as found in getgroups(). Some of these changes, for example return()ing in the middle of a procedure, are highly dependant on the removal of Giant. goto's are questionable but replacing them with return()s in the middle of a procedure isn't too hot an idea either. * I do not think it is a good idea to mix these changes with the ucred changes, even if they appear to be straightforward. You are making a large number of changes to the system all at once. The changes should focus only on what is absolutely necessary in this round. Leave syntactical (cleanup?) to a later round. * I strongly, *strongly* disagree with the removal of Giant at this time, even in 'read-only' functions. I would much rather see a methodology whereby Giant is replaced with an instrumented Giant such as found in the patches I was working on. If you are really worried about the 10ns of call overhead I believe Peter has been interested in making the instrumentation optionable at compile time (and I am not against that provided it is done correctly). The reason I am strongly opposed to the removal of Giant is several fold: - Giant serves to mark where we were essentially single threading the kernel before and this will be important in understanding and tracking down bugs we find in the future. And make no mistake, there will be many. - Giant is going to be pushed down into many subsystems in coming months. It is highly likely that many hard-to-find bugs are going to come out of the woodwork, no matter how conservative we are or how well we believe we understand the code. Without instrumentation tracking down the less obvious bugs is going to be difficult. - It's a bad idea to second-guess the code, even if a piece of code looks like Giant can simply be removed (i.e. due to being a read-only function like getuid()). The removal of Giant creates all sorts of side effects, from something as simple as a difference in performance to something more complex such as creating memory sync points and removing (pseudo giant-enforced) atomicy that was previously depended upon. We are almost certainly going to face hard-to-find races in the code as Giant is unwound, even if we are extremely conservative in our commits. It's going to happen. The code was never designed for MP operation. Instrumenting Giant unconditionally at this early stage will make our jobs a whole lot easier. If you are completely against doing Giant instrumentation in your own patch sets then I would like both a head's up (like you just did) of patch sets you intend to commit to the main tree, and also permission to add Giant instrumentation in a secondary commit after you make the initial commit (which also means I would request that you not make code flow changes if at all possible). I think it's *that* important. I would much prefer that you do it yourself but if you won't I feel strongly enough about the issue to want to do it myself. --- on cred_update_thread() -- I suggested this same thing to Julian so I agree with adding Giant back in. Again, I would instrument it instead of just adding back in, i.e.: s = mtx_lock_giant(kern_giant_proc); PROC_LOCK(p); td->td_ucred = crhold(p->p_ucred); PROC_UNLOCK(p); mtx_unlock_giant(s); Instead of: mtx_lock(&Giant); PROC_LOCK(p); td->td_ucred = crhold(p->p_ucred); PROC_UNLOCK(p); mtx_unlock(&Giant); In anycase, if you are willing to either instrument Giant or allow me to then I am willing to do full reviews of reasonably-chunked patch sets (the URL you just posted is quite reasonably chunked BTW). -Matt To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-current" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200202230534.g1N5Ytr34236>