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 */
}
