request for review, callout fix.

[Alfred Perlstein]

* John Baldwin <jhb at freebsd.org> [080321 12:08] wrote:
> On Friday 21 March 2008 12:32:25 am Alfred Perlstein wrote:
> > Hi guys, attached is a fix for the timeout/untimeout race with
> > Giant locked code.
> > 
> > Basically the old code would make the callout inaccessable
> > right before calling it inside of softclock.
> > 
> > However only the callout lock is held, so when switching to
> > the callout's associated mutex (in this case Giant) there's
> > a race where a "local" (timeout/untimeout) callout would be
> > fired even if stopped.
> > 
> > This patch fixes that.  We've run several hours of regression
> > testing on a version of this for 6.x.
> > 
> > People internal to Juniper and iedowse@ helped with this.
> > 
> > Please review/comment.
> 
> Curious as to how c->c_flags could change if CALLOUT_LOCAL_ALLOC is set?  
> Since it hasn't been enqueued on the callfree list, it isn't visible to any 
> other code, so nothing should be able to mark it active or pending.  IOW, you 
> should be able to do this in your second hunk:
> 
> 	if (c_flags & CALLOUT_LOCAL_ALLOC) {
> 		KASSERT(c->c_flags == CALLOUT_LOCAL_ALLOC, ("corrupted callout"));
> 		c->c_func = NULL;
> 		SLIST_INSERT_HEAD(...);
> 	}

It's more hairy than that.

Actually, I think you're right...

The confusion is the race for "callout_stop_safe",
but now that I think about it, the softclock will only
"grab" this callout if untimeout has NOT yet been called.

If untimeout HAS been called, then softclock won't even see
the callout.
If untimeout HAS NOT been called and softclock grabs the
callout, it will have cleared CALLOUT_PENDING and then
untimeout (callout_stop_safe) will no longer free it.

Therefore it is safe to omit the check for flags as you
suggest.

Is that right?

-- 
- Alfred Perlstein