tech-kern archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

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

OK, got it.

I'll have another set of patches in a few days.

On Thu, 5 Aug 2010, Andrew Doran wrote:

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.


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


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-

   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 {
        sysconfig_lwp = l;
        /* etc etc */

| Paul Goyette     | PGP Key fingerprint:     | E-mail addresses:       |
| Customer Service | FA29 0E3B 35AF E8AE 6651 | paul at    |
| Network Engineer | 0786 F758 55DE 53BA 7731 | pgoyette at |
| Kernel Developer |                          | pgoyette at  |

Home | Main Index | Thread Index | Old Index