Date: Wed, 9 Jul 2014 12:11:47 -0400 From: John Baldwin <jhb@freebsd.org> To: freebsd-arch@freebsd.org Cc: Hans Petter Selasky <hps@selasky.org>, arch@freebsd.org, John-Mark Gurney <jmg@funkthat.com> Subject: Re: callout(9) really this complicated? Message-ID: <201407091211.47642.jhb@freebsd.org> In-Reply-To: <20140704155752.GB45513@funkthat.com> References: <20140704041521.GW45513@funkthat.com> <53B64694.9030100@selasky.org> <20140704155752.GB45513@funkthat.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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:
> > On 07/04/14 06:15, John-Mark Gurney wrote:
> > >So, I was going to look at using callout(9) for some time delayed
> > >actions... But upon reading the docs, a) the docs are inconsistent,
> > >and b) the docs only talk about requirements in other section...
> > >
> > >Is there a better interface? If so, can we mark callout(9) deprecated?
> > >If not, I have some questions...
> > >
> > >If you want callout_drain to work properly, you have to add extra code
> > >to both your callout, and around the usage of it...
> > >
> > >callout_drain does not drain the callout:
> > > However, the callout subsystem does guarantee that the callout will
> > > be
> > > fully stopped before callout_drain() returns.
> > >
> > >Yet other parse of the docs say that you can depend upon the callout
> > >being fully stopped.. I've sent email to Ian (iedowse) about why he
> > >added this statement...
> > >
> > >Second, the amount of work you have to do to make sure you drain
> > >seems pretty crazy as documented in Avoiding Race Conditions...
> > >
> > >It seems like if I have created a callout w/ callout_init_mtx,
> > >that I shouldn't have to do extra work to make it work correctly...
> > >
> > >When calling _callout_stop_safe as callout_drain, there is no assert
> > >that the lock is held, though it is documented as requiring it by:
> > > The function callout_drain() is identical to callout_stop() except
> > > that
> > > it will wait for the callout to be completed if it is already in
> > > progress.
> > >
> > >Maybe we need to add an additional statement here? and assert that it
> > >isn't locked?
> > >
> > >Also, I have tried to follow the code, but it's complicated, so I'm
> > >hoping that I can get some help here.
>
>
> > 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.
--
John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201407091211.47642.jhb>