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 <[EMAIL PROTECTED]>  <><  http://www.FreeBSD.org/~jhb/
"Power Users Use the Power to Serve!"  -  http://www.FreeBSD.org/

To Unsubscribe: send mail to [EMAIL PROTECTED]
with "unsubscribe freebsd-current" in the body of the message

Reply via email to