skipping locks, mutex_owned, usb

[John Baldwin]

On Tuesday, August 23, 2011 8:09:15 am Andriy Gapon wrote:
> 
> Yes, the subject sounds quite hairy, so please let me try to explain it.
> First, let's consider one concrete function:
> 
> static int
> ukbd_poll(keyboard_t *kbd, int on)
> {
>         struct ukbd_softc *sc = kbd->kb_data;
> 
>         if (!mtx_owned(&Giant)) {
>                 /* XXX cludge */
>                 int retval;
>                 mtx_lock(&Giant);
>                 retval = ukbd_poll(kbd, on);
>                 mtx_unlock(&Giant);
>                 return (retval);
>         }
> 
>         if (on) {
>                 sc->sc_flags |= UKBD_FLAG_POLLING;
>                 sc->sc_poll_thread = curthread;
>         } else {
>                 sc->sc_flags &= ~UKBD_FLAG_POLLING;
>                 ukbd_start_timer(sc);   /* start timer */
>         }
>         return (0);
> }
> 
> This "XXX cludge" [sic] pattern is scattered around a few functions in the ukbd
> code and perhaps other usb code:
> func()
> {
> 	if (!mtx_owned(&Giant)) {
> 		mtx_lock(&Giant);
>                 func();
>                 mtx_unlock(&Giant);
> 		return;
> 	}
> 
> 	// etc ...
> }
> 
> I have a few question directly and indirectly related to this pattern.
> 
> I.  [Why] do we need this pattern?
> Can the code be re-written in a smarter (if not to say proper) way?

Giant is recursive, it should just be always acquired.  Also, this recursive
call pattern is very non-obvious.  A far more straightforward approach would
be to just have:

static int
ukbd_poll(keyboard_t *kbd, int on)
{
        struct ukbd_softc *sc = kbd->kb_data;

        mtx_lock(&Giant);
        if (on) {
                sc->sc_flags |= UKBD_FLAG_POLLING;
                sc->sc_poll_thread = curthread;
        } else {
                sc->sc_flags &= ~UKBD_FLAG_POLLING;
                ukbd_start_timer(sc);   /* start timer */
        }
        mtx_unlock(&Giant);
        return (0);
}

 
> II.  ukbd_poll() in particular can be called from the kdb context (kdb_trap ->
> db_trap -> db_command_loop -> etc).  What would happen if the Giant is held by a
> thread on some other CPU (which would be hard-stopped by kdp_trap)?  Won't we get
> a lockup here?
> So shouldn't this code actually check for kdb_active?

Probably, yes.

> III.  With my stop_scheduler_on_panic patch ukbd_poll() produces infinite chains
> of 'infinite' recursion because mtx_owned() always returns false.  This is because
> I skip all lock/unlock/etc operations in the post-panic context.  I think that
> it's a good philosophical question: what operations like mtx_owned(),
> mtx_recursed(), mtx_trylock() 'should' return when we actually act as if no locks
> exist at all?

Most code should not be abusing mtx_owned() in this fashion.  For Giant you
should just follow a simple pattern like above that lets it recurse.  For your
own locks you generally should use things like mtx_assert() to require all
callers of a given routine to hold the lock rather than recursively acquiring
the lock.  Very few legitimate cases of mtx_owned() exist IMO.  It is
debatable if we should even have a mtx_owned() routine since we have
mtx_assert().

Given this, I would leave mtx_owned() and mtx_trylock() as-is.

> That question III actually brings another thought: perhaps the whole of idea of
> skipping locks in a certain context is not a correct direction?
> Perhaps, instead we should identify all the code that could be legitimately
> executed in the after-panic and/or kdb contexts and make that could explicitly
> aware of its execution context.  That is, instead of trying to make _lock,
> _unlock, _owned, _trylock, etc do the right thing auto-magically, we should try to
> make the calling code check panicstr and kdb_active and do the right thing on that
> level (which would include not invoking those lock-related operations or other
> inappropriate operations).

I believe that you'd end up having to maintain duplicate code for the two cases
and that it would be a bigger headache as a result.

-- 
John Baldwin