msleep(9) and recursed mutex

[Hans Petter Selasky]

On Mon, Oct 11, 2004 at 03:10:45PM +0200, damien.bergamini at free.fr wrote:
> msleep(9) behaves strangely with recursed mutexes.
> In the attached code, calling foo_f() will make the kernel hang for
> two seconds.
> It seems like msleep does not release the mtx mutex completely but
> simply decrement its reference count by one. When calling msleep, the
> mtx mutex is still held which prevent the interrupt from happening.
> msleep will then return error code EWOULDBLOCK after two seconds.
> If I remove calls to mtx_lock/unlock from function foo_g(), it works
> without problem but this is not a solution since foo_g() can be
> called outside of function foo_f().
> Of course, the mtx mutex was created with the MTX_RECURSE flag set.
> 
> Is it an expected behaviour? In msleep(9) it is said:
> "The msleep() function is a variation on tsleep.  The parameter mtx
> is a mutex which will be released before sleeping and reacquired
> before msleep() returns."
>
> Seems like the mutex is not *really* released if it recurses.
>

Only the "Giant" mutex is released if it recurses. By the way there is
a bug in msleep:

mtx_lock(&Giant);
msleep(xxx, &Giant, 0, "foo", 0);
mtx_unlock(&Giant);

doesn't work. See attached patch.

> 
> Thanks,
> Damien Bergamini
> 
> ----
> 
> void foo_intr(void *arg)
> {
>     struct foo_softc *sc = arg;
> 
>     mtx_lock(&sc->mtx);
>     ...
>     wakeup(sc);
>     ...
>     mtx_unlock(&sc->mtx);
> }
> 
> void foo_f(struct foo_softc *sc)
> {
>     mtx_lock(&sc->mtx);
>     ...
>     foo_g(sc);
>     ...
>     mtx_unlock(&sc->mtx);
> }
> 
> void foo_g(struct foo_softc *sc)
> {
>     /* mtx will recurse if called from foo_f() */
>     mtx_lock(&sc->mtx);
>     ...
>     /* do something that will make hardware raise an interrupt */
>     ...
>     /* wait at most 2 second for the interrupt */
>     msleep(sc, &sc->mtx, 0, "foo", 2 * hz);
>     ...
>     mtx_unlock(&sc->mtx);
> }

-HPS
-------------- next part --------------
*** src/sys/kern/kern_synch.c.ref	Wed Sep 29 14:21:09 2004
--- src/sys/kern/kern_synch.c	Wed Sep 29 14:22:25 2004
***************
*** 182,193 ****
  	CTR5(KTR_PROC, "msleep: thread %p (pid %ld, %s) on %s (%p)",
  	    (void *)td, (long)p->p_pid, p->p_comm, wmesg, ident);
  
- 	DROP_GIANT();
  	if (mtx != NULL) {
  		mtx_assert(mtx, MA_OWNED | MA_NOTRECURSED);
  		WITNESS_SAVE(&mtx->mtx_object, mtx);
  		mtx_unlock(mtx);
  	}
  
  	/*
  	 * We put ourselves on the sleep queue and start our timeout
--- 182,194 ----
  	CTR5(KTR_PROC, "msleep: thread %p (pid %ld, %s) on %s (%p)",
  	    (void *)td, (long)p->p_pid, p->p_comm, wmesg, ident);
  
  	if (mtx != NULL) {
  		mtx_assert(mtx, MA_OWNED | MA_NOTRECURSED);
  		WITNESS_SAVE(&mtx->mtx_object, mtx);
  		mtx_unlock(mtx);
  	}
+ 	/* drop Giant after mtx in case mtx == &Giant */
+ 	DROP_GIANT();
  
  	/*
  	 * We put ourselves on the sleep queue and start our timeout