Date: Tue, 23 Aug 2011 09:11:08 -0400 From: John Baldwin <jhb@freebsd.org> To: freebsd-arch@freebsd.org Cc: Andriy Gapon <avg@freebsd.org> Subject: Re: skipping locks, mutex_owned, usb Message-ID: <201108230911.09021.jhb@freebsd.org> In-Reply-To: <4E53986B.5000804@FreeBSD.org> References: <4E53986B.5000804@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201108230911.09021.jhb>