:> 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

    - 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

    - 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

    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);
        td->td_ucred = crhold(p->p_ucred);

    Instead of:

        td->td_ucred = crhold(p->p_ucred);

    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).


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

Reply via email to