Page MenuHomeFreeBSD

callout: provide CALLOUT_TRYLOCK flag
AcceptedPublic

Authored by glebius on Wed, Jun 26, 3:56 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Jun 28, 10:31 PM
Unknown Object (File)
Thu, Jun 27, 3:31 PM
Subscribers

Details

Reviewers
kib
jtl
Group Reviewers
transport
Summary

If a callout was initialized with the flag, then the callout(9) system
will not drop the callwheel lock in softclock_call_cc() to obtain the
callout lock. Instead it will use try-lock semantic to obtain the
callout's lock. In case of a failure the callout will be rescheduled to
the nearest cycle reusing the original precision value. The main benefit
of such behavior is not the avoidance of the lock contention in the
callout thread, but the fact that callout with such flag can be actually
stopped in a safe manner, because the race window in the beginning of
softclock_call_cc() is closed.

Call of callout_stop() on such a callout would guarantee that nothing will
be executed after callout_stop() returns, neither callout lock will be
dereferenced. A callout marked as CALLOUT_TRYLOCK |
CALLOUT_RETURNUNLOCKED can call callout_stop() from the callout function
itself (0, a failure to stop, will be returned), then unlock the lock and
then free the memory containing the callout structure.

Caveat: when calling callout_stop() from outside the callout function, the
return value from callout_stop() is still inconsistent. A race window at
the end of softclock_call_cc() still exists, so callout_stop() may report
failure to stop, which would not be a true.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 58361
Build 55249: arc lint + arc unit

Event Timeline

jtl added a subscriber: jtl.

Overall, I think this approach has value. See my in-line comments for suggestions on things to review further.

sys/kern/kern_timeout.c
685

Just a note to say that it looks like this is going to change the way callout_sbt_reset_on() works. Previously, that function could cancel a callout which was waiting on the INP_LOCK. Now, it will work differently and I'm not sure it still makes sense to have the cancellation concept there for callouts with CALLOUT_TRYLOCK defined.

699

I wonder if this should be moved into the portion where we hold the CC_LOCK for the new TRYLOCK cases where we know we aren't going to wait on a lock. That might (or might not?) help address my comment about the interactions with callout_reset_sbt_on().

This revision is now accepted and ready to land.Wed, Jun 26, 5:25 PM
sys/kern/kern_timeout.c
680

Just a quick note that you may want to check for (and avoid) the case which would produce a tight loop here, which is possible if direct is true and cc->cc_lastscan is the current bucket. I would almost go so far as to suggest that you should add (1 << (32 - CC_HASH_SHIFT)) to cc->cc_lastscan when direct is true.