On Fri, Jul 05, 2002 at 02:38:08AM +1000, Bruce Evans wrote:
> On Thu, 4 Jul 2002, Jonathan Lemon wrote:
> > In article 
><local.mail.freebsd-current/[EMAIL PROTECTED]> you 
> > >in RELENG_4, when one calls callout_stop() (not nested in softclock execute
> > >path
> > >, I am not talking about this case), after it returns, he can believe that the
> > >callout is truely stopped, however in CURRENT, this assumption is false, now we
> > >
> > >must care if callout_stop() truely stopped the callout when it returned, this
> > >is all difference I see, we bring in this race which not exists in RELENG_4,
> > >see what hacking code put in kern_condvar.c and kern_synch.c in CURRENT source,
> >
> > I don't believe this is true.  There is a corresponding race in -stable,
> > where the spl() level is dropped before performing the callout, thus
> > allowing either a call to callout_stop() or callout_reset() to come in
> > immediately before the callout is actually made.
> I think Giant locking everything prevents problems in RELENG_4, at least
> when callout_stop() is called in process context (if it is called in
> interrupt context then it could easily be interrupting a callout even in
> the UP case).

In the network stack at least, callout_stop() is called in interrupt
context, so this case actually happens, and has to be handled.

> The race window extends from when the ipl or lock is dropped across the
> whole callout until the ipl or lock is regained. (The ipl is only dropped
> to splstatclock(); this prevents interruption by timeouts but not by
> other interrupts.  In -current there is nothing much to prevent softclock()
> itself being called concurrently, but in theory softclock()'s internal
> locking should prevent problems.)
> > The callout function is responsible for checking to see if it has lost
> > the race, and perform the appropriate action.  This is done with the
> >
> >         s = splnet();
> >         if (callout_pending(tp->tt_delack) || !callout_active(tp->tt_delack)) {
> >                 splx(s);
> >                 return;
> >         }
> >         callout_deactivate(tp->tt_delack);
> I think David is objecting to this complicating all callers that do the
> check and breaking all that don't.  The callers in kern_synch.c and
> kern_condvar.c have an mi_switch() and other complications to handle
> this sinc they can't just return.

I believe I had this conversation with Justin Gibbs earlier; he told 
me that the callout consumers (network, cam) had to be aware of the 
race and handle this if it matters.  I don't particularly like complicating
the callout handlers as illustrated above, though, so if a better scheme
is possible, that would be nice.

I originally wanted something equivalent to an atomic spl downgrade
(splhigh -> splnet), so the timeout code could obtain the target
ipl/lock that the callout handler wanted before dropping splhigh(), but
was told this would unnecessarily complicate things.

> > If 'CALLOUT_PENDING' is set, then we lost a race with callout_reset(),
> > and should not perform the callout.  If 'CALLOUT_ACTIVE' is clear, then
> > we lost a race with callout_stop().
> >
> > Either way, on both -current and -stable, you cannot assume that the
> > timer callback is completely gone immediately after calling callout_stop().
> tsleep() seems to assume this in RELENG_4.

tsleep() is only called from process context, and appears to be safe
because p->wchan has already been cleared by a previous call to unsleep().
The races only matter if you actually care about them.

To Unsubscribe: send mail to [EMAIL PROTECTED]
with "unsubscribe freebsd-current" in the body of the message

Reply via email to