Re: Anomalies while handling p_nstopchild count

[Taylor R Campbell <campbell+netbsd-tech-kern%mumble.net@localhost>]

   Date: Sat, 10 Oct 2015 14:31:41 -0500
   From: Don Lee <MacPPC2%c.icompute.com@localhost>

   On Oct 10, 2015, at 10:27 AM, Taylor R Campbell <campbell+netbsd-tech-kern%mumble.net@localhost> wrote:

   > proc_lock may be held by another thread even if the caller is
   > guaranteed not to hold it.  The other thread may furthermore be
   > waiting on p->p_lock, in which case acquiring p->p_lock here would
   > lead to deadlock -- but mutex_tryenter would simply fail.
   > 
   > The optimization is to optimistically assume nobody else is holding
   > proc_lock and do mutex_tryenter.  Otherwise, we back out and acquire
   > both locks in the correct order.

   Pardon me for opining on this without full knowledge, but it seems
   a bit creepy to me to be dealing with locks as though you don't
   know who holds them.  The normal case should be that either you
   hold the lock or you don't.  You should know.

   If you don't, there is a hierarchy and an order to claiming them,
   and you follow it.

There is a lock order: proc_lock first, then p->p_lock for any single
p.  Nobody is proposing to violate or weasel around this lock order.

The patch does not reverse the lock order, nor allow the caller to
hold or not hold proc_lock -- the caller must not hold proc_lock.  The
patch only transitions from holding only p->p_lock to holding both
proc_lock and p->p_lock both.

Releasing p->p_lock here is OK because this branch is about to release
all locks and sleep anyway, so subsequent code does not rely on any
invariants that are lost.  The transition could be done with

mutex_exit(p->p_lock);
mutex_enter(proc_lock);
mutex_enter(p->p_lock);

but if usually no other threads hold proc_lock, it costs less on
average to do

if (!mutex_tryenter(proc_lock)) {
	mutex_exit(p->p_lock);
	mutex_enter(proc_lock);
	mutex_enter(p->p_lock);
}

instead, since each mutex_(try)enter entails an expensive atomic
operation.