Re: Module and device configuration locking [was Re: Modules loading modules?]

[Andrew Doran <ad%NetBSD.org@localhost>]

On Wed, Aug 04, 2010 at 10:28:56AM -0700, Paul Goyette wrote:
> On Wed, 4 Aug 2010, Andrew Doran wrote:
> 
> >Sorry for not replying sooner.
> >
> >Please don't add a generic recursive lock facility, it would be abused.
> 
> Yeah!  My current patches have no new generic facilities at all.
> 
> >I'd like something roughly like the following.  I think this should
> >also cover major configuration tasks such as device attach and detach,
> >so it wouldn't really be module specific.  The naming is a poor suggestion.
> >
> >void
> >sysconfig_lock(void)
> >{
> >
> >     if (mutex_owned(&sysconfig_mutex)) {
> >             /*
> >              * At this point it's OK for the thread to hold other
> >              * locks, since the system configuration lock was the
> >              * first taken on the way in.
> >              */
> >             sysconfig_recurse++;
> >             return;
> >     }
> >     /*
> >      * I don't remember the correct argument sequence to the
> >      * following, but basically we need to assert that NO locks,
> >      * sleep or spin, other than kernel_lock are held,
> >        * as the system configuration lock would be the outermost
> >      * lock in the system.
> >      */
> >     LOCKDEBUG_BARRIER(...);
> >     mutex_enter(&sysconfig_mutex);
> >     KASSERT(sysconfig_recurse == 0);
> >}
> >
> >For the boot autoconfig path where you have things being driven by the
> >kernel, this would work OK.  Situations where there's a kthread or
> >something attaching and detaching devices on the fly are a bit different,
> >since they likely have local atomicity requirements (need to take device
> >private locks).  There you'd probably need the caller to take the lock.
> >USB comes to mind (along with some hardware RAID drivers that do hotplug
> >attach/detach).
> >
> >Thoughts?
> 
> Well, at first glance, this proposal makes sense.  But it is
> certainly a much larger task than dealing with the immediate problem
> - modules which want to load other modules.
> 
> So, I really have three questions:
> 
> 1. In keeping with earlier concerns about holding locks for long periods
>    of time (eeh and mrg both commented on this), the approach described

For module lock the wait time isn't really a big concern.  Module load
and unload is a heavyweight operation so the wait time is expected.
There is nothing intrinsically wrong with holding a mutex for "a long time"
provided that you've got a good handle on the potential side effects.
It's a different story for other locks in the system like say proc_lock,
which can't be held for a long time without disrupting the user experience
and/or deadlocking the system.

>    above would seem to have an even longer hold-time than the current
>    module_lock.  Would not something similar to haad's condvar approach
>    be appropriate for this proposal, as well as for the more-limited-in-
>    scope recursive module lock?

condvar gives you another lock in a roundabout way, without priority
inheritance to help move things along if something has clogged the
pipework. :-).  So I don't see benefit over a mutex.

> 2. Would it be reasonable to solve the immediate problem as previously
>    proposed, rather than waiting for this "ultimate" solution?  I think
>    it would be a long time before we could find and resolve all of the
>    "issues" that might be created in the various "threaded" situations
>    that may exist.  I know I'm certainly not sufficiently qualified to
>    identify all those "situations", and I also know that I don't have
>    sufficient available work-hours to do this in any reasonable time-
>    frame.
> 
>    I'd still like to move forward with the most recent solution to the
>    module_lock problem.

I'm not suggesting that you need to tackle autoconfiguration locking at
the same time.. What I'm suggesting is to put the primitives in place
(say in kern_lock.c or something) and then use these for the benefit of
the modules code. It would provide a statment of direction and avoid
re-hashing things later when somebody decides to tackle autoconf.
Sorry for being unclear.

> 3. Since we're still technically abusing the mutex(9) man-page caveat
>    about using mutex_owned() for making locking decisions, it would
>    be very much appreciated (at least by myself) to have an explanation
>    of WHY it is OK in this case to do it here, but not somewhere else.

Ok well I think the simplest course of action would be to replace the
mutex_owned() with a global variable.  It would do much the same thing
but for the price of 4 or 8 bytes there is no question of contradicting
the mantra. So like:

/*
 * Ok to check this unlocked, as for the value to equal curlwp it must
 * have been set by the current thread of execution (i.e. not interrupt
 * context or another LWP).
 */
l = curlwp;
if (sysconfig_lwp == l) { 
        /* recurse */
} else {
        mutex_enter(&sysconfig_lock);
        sysconfig_lwp = l;
        /* etc etc */
}