Date: Sat, 23 Feb 2002 01:08:39 -0500 (EST) From: John Baldwin <jhb@FreeBSD.org> To: Matthew Dillon <dillon@apollo.backplane.com> Cc: current@FreeBSD.ORG Subject: Re: RE: First (easy) td_ucred patch Message-ID: <XFMail.020223010839.jhb@FreeBSD.org> In-Reply-To: <200202230534.g1N5Ytr34236@apollo.backplane.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On 23-Feb-02 Matthew Dillon wrote: >:> 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. It's how the code is going to look as the final rendition. It also restores the code more to its 4.x flow making a diff to see what actual changes SMPng made easier to read. > * 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. They weren't committed separately from adding Giant to the functions when they went in. :) > * 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). I don't care about whatever time it takes to do teh check. I don't make decisions about debugging code based on clock cycles. Here are my concerns: - kern.giant.proc as you would have it now is far too broad. Most of the proc locking currently in the tree and in my work tree is not safe yet. This is because certain fields are only locked in certain places. For a field to be safe outside of Giant, it needs to be locked everywhere. You include both code that contains partially locked fields and fully locked fields under the same sysctl. This means I can't actually turn the sysctl on to do the testing safely, so I might as well just leave Giant in there rahter than bother with a useless sysctl. One solution might be to split ths sysctl up. Well then, how many are we going to have, one sysctl for each field in proc? This won't scale in my opinion. It may be useful for covering fields that aren't fully locked, but for stuff that is done, I don't think you need it. - How many various locking systems do syscalls like read() going call into? Are we going to eventually need to check 8, 10, or 16 sysctl's? Trying to keep all that straight will be a major pain. This is another reason I don't think they scale well. - We eventually have to go and remove all this stuff anyways. - As another note specific to td_ucred: there is no other lock that you are "covering up" for. It is a private per-thread pointer to a read-only structure. I can see needing to turn Giant back on around a lock done wrong, but there is no lock in this instance. We need to people to test stuff as it comes out from under Giant so we can find the bugs sooner rather than later, the current way these things work is far too coarse grained and I think I'll be spending more time figuring out how to split them up to make them useful and then figure out where they need to be acquired. For example, assuming I used kern.giant.proc.ucred for just the td_ucred stuff, since I've changed just about every VOP in the system, I now need to add instrumentation around every single syscall that might call a VOP, or that might call any of the other functions I changed. Maybe if you want SMPng to take 5 times as long... -- John Baldwin <jhb@FreeBSD.org> <>< http://www.FreeBSD.org/~jhb/ "Power Users Use the Power to Serve!" - http://www.FreeBSD.org/ 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?XFMail.020223010839.jhb>