Date: Fri, 26 Sep 2014 15:52:53 -0400 From: John Baldwin <jhb@freebsd.org> To: freebsd-arch@freebsd.org, John-Mark Gurney <jmg@funkthat.com> Cc: Hans Petter Selasky <hps@selasky.org> Subject: Re: callout(9) really this complicated? Message-ID: <1831172.E2TVxmQund@ralph.baldwin.cx> In-Reply-To: <201407091211.47642.jhb@freebsd.org> References: <20140704041521.GW45513@funkthat.com> <20140704155752.GB45513@funkthat.com> <201407091211.47642.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wednesday, July 09, 2014 12:11:47 PM John Baldwin wrote:
> On Friday, July 04, 2014 11:57:52 am John-Mark Gurney wrote:
> > Hans Petter Selasky wrote this message on Fri, Jul 04, 2014 at 08:15
+0200:
> > > Probably the documentation needs an update. The implementation is fine.
> >
> > Probably... But w/ bad docs, it makes you wonder about the
> > implementation...
>
> If you use callout_init_mtx(), then you can use callout_reset() and
> callout_stop() while holding the mutex and everything will work as expected.
> You typically only need to use callout_drain() during a device detach
> routine to "destroy" the callout().
>
> (A typical "detach" routine looks something like this:
>
> - detach external connections (ifnet, cdev, etc.)
> - stop the hardware itself (foo_stop in various NIC drivers)
> - this step typically does a callout_stop() with the relevant lock held
> - drain any pending async events
> - bus_teardown_intr() will block until the interrupt handler is
> idle
> - callout_drain() will block if the callout is pending because softclock
> had already dequeued it and it was waiting for the driver lock when
> you called callout_stop() earlier
> - taskqueue_drain() (note that tasks do not have implicit locking ala
> callout_init_mtx(), so you need to set some sort of flag that your
> task function checks and breaks out of any loops for in the
> "stop the hardware" step)
> - free resources and destroy locks, etc.
>
> > > drain is always called unlocked.
> >
> > Then why the whole long section about avoiding races? Point #3 is
> > the main one that I'm talking about... It seems like it'd be easier
> > for callout to properly maintain the active flag (maybe set a flag to
> > tell callout to do so), or not even maintain it since it really isn't
> > used for anything important if you can munge it all you want... There
> > aren't any statements about bad things happening if you call _deactivate
> > before the callout is called...
>
> The language is unclear, but you only need to use one of the 3 options to
> work around the races. Note that if you use callout_init_mtx() you fall
> into case #1 and don't need to worry about the others. If you want to
> use callout_init(.., CALLOUT_MPSAFE) and manage all the locking in your
> callout handler directly, then you need to handle the assorted races.
> However, you can generally get by with something far simpler than it
> suggests. You can just do what I described above for taskqueue_drain(),
> i.e. have your timer function do:
>
> foo(void *arg)
> {
> struct foo_softc *sc;
>
> sc = arg;
> FOO_LOCK(sc);
> if (sc->is_dead) {
> FOO_UNLOCK(sc);
> return;
> }
> ....
> callout_reset(...);
> FOO_UNLOCK(sc);
> }
>
> In this case, simply setting 'is_dead' in the "stop the hardware" step and
> using callout_drain() will work fine.
I've overhauled the timeout(9) manpage a bit (my primary motivation is I'm
getting close to removing timeout() from HEAD and I want the docs to be
callout centric instead of timeout-centric). I did not mention my 'is_dead'
flag approach above, but I think I might have covered the other issues (as
well as documenting several current gaps).
https://reviews.freebsd.org/D847
--
John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1831172.E2TVxmQund>