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