need for another mutex type/flag?

[Robert Watson]

On Tue, 27 Jan 2009, Julian Elischer wrote:

> Robert Watson wrote:
>> On Sun, 25 Jan 2009, Julian Elischer wrote:
>> 
>>> Even purely documentary would be good but given the option, I'd like it to 
>>> scream when Witness is enabled and you try get another mutex....
>>> 
>>> there are certain contexts (e.g. in most netgraph nodes) where we really 
>>> want the authors to only take such locks and taking more unpredictable 
>>> mutexes plays havoc with netgraph response times as a system as a whole, 
>>> since if one node blocks, teh thread doesn't get to go off and service 
>>> other nodes.
>> 
>> I thought lots of Netgraph nodes liked to do things like call m_pullup() 
>> and m_prepend()?  Disallowing acquiring mutexes will prevent them from 
>> doing that.
>
> I think I mentioned that in another mail. The problem I see is that some 
> module writers are tempted to just use a mutex in their node without 
> thinking about what the result will be.  My understanding is that the mbuf 
> allocation code has been tuned to within an inch of its life to try keep 
> it's waits to a minimum.  I am worried about it, but I am more worried about 
> a netgraph node waiting on something that is depending on some piece of 
> hardware such as what I think HPS had in his suggested patch for the 
> bluetooth code..

Right, but what I'm saying is: if we have a MTX_LEAFNODE flag for mtx_init(9), 
it won't work for any code that holds the lock over a call to the mbuf 
routines.  I am happy with us adding a MTX_LEAFNODE flag and would use it 
myself, I just not sure it will work for Netgraph node mutexes.

The case you're talking about is generally problematic for mutexes anyway -- 
in principle we divide the world of sleeping into two categories: sleeps of 
strictly "bounded" length that generall correspond to the sleeps associated 
with mutex or rwlock acquire, and sleeps of potentially "unbounded" length, 
such as those associated with msleep(9), cv_wait(9), sx(9), lockmgr(9), etc. 
If you try to perform an unbounded sleep while holding a mutex, then WITNESS 
will already complain about sleeping while holding a lock.

The tricky case is the "may sleep" case, in which case we already have a 
WITNESS call you can do to say this code may sleep, even though it doesn't in 
the common case, so warn if this is called with a mutex held so that we can 
catch that happening".  For example, mb_reclaim does this so that any attempt 
to call it with a mutex held will throw up a red flag:

/*
  * This is the protocol drain routine.
  *
  * No locks should be held when this is called.  The drain routines have to
  * presently acquire some locks which raises the possibility of lock order
  * reversal.
  */
static void
mb_reclaim(void *junk)
{
         struct domain *dp;
         struct protosw *pr;

         WITNESS_WARN(WARN_GIANTOK | WARN_SLEEPOK | WARN_PANIC, NULL,
             "mb_reclaim()");

         for (dp = domains; dp != NULL; dp = dp->dom_next)
                 for (pr = dp->dom_protosw; pr < dp->dom_protoswNPROTOSW; pr++)
                         if (pr->pr_drain != NULL)
                                 (*pr->pr_drain)();
}

If there is a common or unconditional call to msleep(9) in code called by 
Netgraph with a mutex held, however, then it should already be displaying a 
warning when that happens.  If you want to simulate this effect without a lock 
necessarily be held, you can do what the softclock() code does:

                                 THREAD_NO_SLEEPING();
                                 c_func(c_arg);
                                 THREAD_SLEEPING_OK();

This causes WITNESS to complain loudly if the callout handler attempts to 
perform an unbounded sleep (i.e., msleep(), cv_wait(), etc).  Splitting the 
world into bounded an unbounded sleeps is quite useful, and is entirely about 
the sort of deadlock/cascading delay scenario you have in mind: code inside a 
mutex-protected block is essentially a critical section, and should never wait 
a long time, especially given that ithreads may acquire mutexes leading to 
potential deadlocks if msleep() is effectively waiting on an interrupt that 
will never be delivered -- if we allow ithreads to wait on msleep(), which we 
don't because of the above mechanisms.

Robert N M Watson
Computer Laboratory
University of Cambridge