callout(9) really this complicated?

[John Baldwin]

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