mutex quandry

[Brad Huntting]

>: Dear hackers:
>: 
>: My device driver, like many, maintains a counter (sc_inuse) and a flag
>: (sc_running) to keep track of how many 'users' are using the device
>: instance variables (softc) and wheather the device is trying to
>: detach.  I call the functions scm_reserver() and scm_leave() (see
>: below) before and after any block of code that uses the softc.  Then,
>: in my detach() function I clear the sc_running flag and msleep()
>: waiting for the sc_inuse counter to drain to zero before continuing,
>: eventually destroying the very mutex I use to protect these two
>: variables.  My problem is here:
>: 
>:         if (sc && mtx_initialized(&sc->sc_inuse_mtx)) {
>:                 mtx_lock(&sc->sc_inuse_mtx);
>:                 ...
>: 
>: Unfortunately,  { mtx_initialized(x) && (mtx_lock(x), 1); } is not
>: atomic.  In the unlikely (but possible) event that the lock is
>: destroyed after mtx_initialized(x) and before mtx_lock(x), the system
>: will behave badly (probably panic).
>
>Why do you care that it is initialized?  This mutex should be created
>in the attach routine, and nothing can race it there unless it is
>creating threads, registering interrupts, etc before initializing it.

I meant to check if it's been destroyed.  Does mtx_initialized()
not do this?

>: It would seem to be a common problem for most device drivers to
>: implement this 'open/closing' lock where the lock starts out
>: zero-filled in the pre-open state and destroyed in the post-closed
>: state. So how should one deal with this?  I could invent my own
>: locking with atomic_*() operations, but then I loose the advantages of
>: using standard system locks (like witness).
>
>Right.  I guess I still don't understand why you need to check to see
>if it is initialized.  You should never be destroying the mutex if
>there are other threads accessing it.

My own threads will certainly be gone by the time I destroy the
mutex.  But this particular device has methods for kernel level
consumers (with they're own threads).  It's these methods that need
to check to see if the device is still running.  They can get the
softc and check (sc && sc->sc_running).  But to ensure the device
doesnt go away while they're using it, they increment the counter
sc_inuse.

The combination of checking sc_running and incrementing sc_inuse
needs to be atomic (hence a mutex around them) but at some point
detach() has to destroy the mutex but my code may still need to use
it.  My solution was to use mtx_inialized() to see if they lock had
been destroyed before trying to mtx_lock() it.

I'm beginning to think that I should avoid a mutex entirely and
somehow use just atomic_*() ops.

>I guess I'm having problems understanding why you are even doing the
>non-atomic test, and why it is needed.  Once you remove the test, the
>code falls into standard norms.
>
>Warner
>
>
>: thanx in advance,
>: brad
>: 
>: /*
>:  * scm_reserve() increments a inuse and scm_release() keep track
>:  * of how many customers are on site.  To shut down we close the
>:  * door (sc_running=0), but we dont actually turn out the lights
>:  * untill the last customer has left (sc_inuse==0).
>:  *
>:  * If device is configured, this returns TRUE and caller must call
>:  * scm_release() when done.  If device is not configured returns
>:  * FALSE and no further action is required.
>:  */
>: static int
>: scm_reserve(struct scmicro_softc *sc)
>: {
>:         int r;
>: 
>:         if (sc && mtx_initialized(&sc->sc_inuse_mtx)) {
>:                 mtx_lock(&sc->sc_inuse_mtx);
>:                 KASSERT(sc->sc_inuse >= 0, "scmicro: scm_reserve()
>: sc_inuse <0!\n");
>:                 r = ( sc->sc_running ? ++sc->sc_inuse : 0 );
>:                 mtx_unlock(&sc->sc_inuse_mtx);
>:         } else {
>:                 r = 0;
>:                 DPRINTF(("scmicro: scm_reserve() failed to reserve
>: sc=%p!\n", sc));
>:         }
>: 
>:         return (r);
>: }
>: 
>: /* call this iff scm_reserve() returns true. */
>: static inline void
>: scm_release(struct scmicro_softc *sc)
>: {
>: 
>:         mtx_lock(&sc->sc_inuse_mtx);
>:         if (--sc->sc_inuse == 0)
>:                 wakeup(&sc->sc_inuse);
>:         KASSERT(sc->sc_inuse >= 0, "scmicro: scm_release() sc_inuse <0!\n");
>:         mtx_unlock(&sc->sc_inuse_mtx);
>: }
>: 
>: static int
>: scmicro_detach(device_t self)
>: {
>:                 /*....*/
>:                 mtx_lock(&sc->sc_inuse_mtx);
>:                 sc->sc_running = 0; /* redundant */
>:                 if (sc->sc_inuse > 0)
>:                         msleep_spin((void *)&sc->sc_inuse, &sc->sc_inuse_mtx,
>:                             "scmicro detach", 0);
>:                 mtx_unlock(&sc->sc_inuse_mtx);
>:                 /*....*/
>:                 mtx_destroy(&sc->sc_inuse_mtx);
>:                 /*....*/
>: }