[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
Re: callout_stop => callout_halt
On Mon, Nov 17, 2014 at 3:00 AM, Taylor R Campbell
> 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)
> - 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
Main Index |
Thread Index |