Re: callout_stop => callout_halt

[Ryota Ozaki <ozaki-r%netbsd.org@localhost>]

On Mon, Nov 17, 2014 at 3:00 AM, Taylor R Campbell
<campbell+netbsd-tech-kern%mumble.net@localhost> wrote:
>    Date: Mon, 17 Nov 2014 01:22:59 +0900
>    From: Ryota Ozaki <ozaki-r%netbsd.org@localhost>
>
>    Here is a patch that includes only ones that require interlock
>    and rearrangements. Others can be fixed by simply replacing
>    callout_stop with callout_halt (already committed).
>
>    --- a/sys/dev/sysmon/sysmon_envsys_events.c
>    +++ b/sys/dev/sysmon/sysmon_envsys_events.c
>    @@ -612,11 +612,19 @@ sme_events_destroy(struct sysmon_envsys *sme)
>     {
>         KASSERT(mutex_owned(&sme->sme_mtx));
>
>    -    callout_stop(&sme->sme_callout);
>    +    /*
>    +     * Unset before callout_halt to ensure callout is not scheduled again
>    +     * during callout_halt
>    +     */
>    +    sme->sme_flags &= ~SME_CALLOUT_INITIALIZED;
>    +
>    +    mutex_enter(&sme->sme_callout_mtx);
>    +    callout_halt(&sme->sme_callout, &sme->sme_callout_mtx);
>
> Can't do callout_halt like that because although it unlocks
> sme_callout_mtx, it will sleep while sme_mtx is held.
>
> It's not clear why you have to hold sme_callout_mtx here, though -- or
> why it even exists independently of sme_mtx.  The author of this code
> failed to write down any locking rules!
>
> My inclination is to nix sme_callout_mtx altogether and replace every
> use of it by sme_mtx (there are obviously some races where one thread
> holds one and another thread holds another and they try to touch the
> same data structure), but I don't know this code well enough to say
> for sure.

Sounds reasonable for me. One concern is that the callout function
calls workqueue_enqueue that enqueues a work of IPL_SOFTCLOCK and
the work also tries to acquire sme_mtx. IIUC, we need to splsoftclock
in the callout function to avoid deadlocks. Is it right?

>
>    +++ b/sys/dev/usb/ohci.c
>    @@ -350,9 +350,7 @@ ohci_detach(struct ohci_softc *sc, int flags)
>         if (rv != 0)
>                 return (rv);
>
>    -    callout_stop(&sc->sc_tmo_rhsc);
>    -
>    -    usb_delay_ms(&sc->sc_bus, 300); /* XXX let stray task complete */
>    +    callout_halt(&sc->sc_tmo_rhsc, &sc->sc_intr_lock);
>
> I wouldn't remove the delay just yet -- there may be USB tasks, like I
> mentioned earlier, still pending which this completely stupid delay
> works around.  (If code inspection can rule out the possibility of
> such USB tasks, though, go for it.)

Oh, I see. I didn't get well the delay well. I'll restore it
since my code inspection cannot ensure the situation never happen.

So the patch is updated: http://www.netbsd.org/~ozaki-r/callout_halt.diff

Thanks,
  ozaki-r