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