Date: Thu, 10 Jul 2014 15:30:32 -0400 From: John Baldwin <jhb@freebsd.org> To: "John-Mark Gurney" <jmg@funkthat.com> Cc: Hans Petter Selasky <hps@selasky.org>, arch@freebsd.org, freebsd-arch@freebsd.org Subject: Re: callout(9) really this complicated? Message-ID: <201407101530.32533.jhb@freebsd.org> In-Reply-To: <20140710061955.GT45513@funkthat.com> References: <20140704041521.GW45513@funkthat.com> <201407091211.47642.jhb@freebsd.org> <20140710061955.GT45513@funkthat.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thursday, July 10, 2014 2:19:55 am John-Mark Gurney wrote:
> John Baldwin wrote this message on Wed, Jul 09, 2014 at 12:11 -0400:
> > 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().
>
> So, you're say that we should just remove this text:
> However, the callout subsystem does guarantee that the callout will be
> fully stopped before callout_drain() returns.
No, the statement is correct (and I somewhat misspoke above). It is helpful
to note that callout_drain() predated callout_init_mtx() IIRC (and this probably
has made the language more confusing that it should be).
Originally, callout_drain() was primarily concerned with solving one race.
Namely, ensuring that the actual function registered with callout_reset() is
not running so that 1) it is safe to remove the function (e.g. kldunload
unmapping the text segment) and 2) the function isn't accessing data you care
about. This same race is the one that both bus_teardown_intr() and
taskqueue_drain() solve. callout_drain() does solve this in that it sabotages
any callout_reset()'s that occur while it is waiting and it blocks until it
is sure that the callout is either dequeued or that softclock() is finished
with the callout.
However, there are other issues with callout functions as you also have races
with callout_stop() if you use a plain callout_init(). In general you would
need to always have some sort of "dead" flag you set under the lock that you
then check at the start of your callout routine (as I described below).
callout_init_mtx() was added to provide a way to move all these checks into
the callout implementation itself. It allows the "dead" flag to live in the
callout structure and be protected by the lock you provide. softclock() is
able to grab this lock to handle the callout_stop() race. The only downside
is that it introduces a different kind of race. (And this is where I misspoke
yesterday) Using callout_init_mtx() resolves the race between callout_stop()
and the function registered via callout_reset(). However, it now stores a
reference to your lock in the callout system. This reference is what the
final callout_drain() is for. It blocks to ensure that softclock() will have
a chance to grab your lock if it is currently waiting for it thus allowing
you to safely destroy the lock after callout_drain() has returned.
> > (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
>
> It isn't clear that point #1 only applies to callout_init_mtx, and that
> the other points don't apply...
Hmm, I think the first part is quite clear as it explicitly mentions
callout_init_mtx:
1. If the callout has an associated mutex that was specified
using the callout_init_mtx() function ...
That seems to imply that 1) only applies if you use callout_init_mtx().
It also says:
FALSE), then this mutex is used to avoid the race conditions.
And for further clarification it states almost exactly what I siad in my first
mail:
The associated mutex must be acquired by the caller before
calling callout_stop() or callout_reset() and it is guaranteed
that the callout will be correctly stopped or reset as
expected.
> And I would say that the text:
> The callout subsystem provides a number of mechanisms to address these
> synchronization concerns:
>
> Is wrong, it only provides a mechanism for ONE case, the _init_mtx case,
> in all other cases you have to provide the mechanism to avoid the race..
Eh, I would also disagree. All three mechanisms require cooperation from
the caller:
1) For this mode you have to agree to the contract that you will hold
the associated lock when you invoke callout_stop() or callout_reset()
and you have to use callout_drain() before you destroy the lock.
2) You have to hold the lock while you manage your "running" flag, but
you do at least get notified by callout_stop() that you are in the
midst of a race so you know when to invoke your special handling.
3) Again, you have to do your own locking, but you can use the flags to
detect the different states your callout is in when your callout
handler runs.
That said, 1) is far simpler to use and is appropriate for almost all of
the callouts I've worked with. I don't think I've ever used 2) or 3).
> > 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.
>
> Ok, unless someone commits a patch soon, I'll create one..
>
> Thanks for the clarification...
>
> --
> John-Mark Gurney Voice: +1 415 225 5579
>
> "All that I will do, has been done, All that I have, has not."
>
--
John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201407101530.32533.jhb>