Page MenuHomeFreeBSD

Ensure cv_waiters is always updated when a thread awakens.
ClosedPublic

Authored by jhb on May 1 2015, 7:45 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Apr 17, 8:11 AM
Unknown Object (File)
Wed, Apr 17, 8:08 AM
Unknown Object (File)
Mar 12 2024, 11:05 PM
Unknown Object (File)
Mar 11 2024, 3:37 AM
Unknown Object (File)
Jan 10 2024, 5:05 AM
Unknown Object (File)
Jan 10 2024, 5:05 AM
Unknown Object (File)
Jan 10 2024, 5:02 AM
Unknown Object (File)
Jan 10 2024, 4:49 AM
Subscribers

Details

Summary

Previously, cv_waiters was only updated by cv_signal or cv_wait. If a
thread awakened due to a time out, then cv_waiters was not decremented.
If INT_MAX threads timed out on a cv without an intervening cv_broadcast,
then cv_waiters could overflow. To fix this, have each sleeping thread
decrement cv_waiters when it resumes.

Note that previously cv_waiters was protected by the sleepq chain lock.
However, that lock is not held when threads resume from sleep. In
addition, the interlock is also not always reacquired after resuming
(cv_wait_unlock), nor is it always held by callers of cv_signal() or
cv_broadcast(). Instead, use atomic ops to update cv_waiters. Since
the sleepq chain lock is still held on every increment, it should
still be safe to compare cv_waiters against zero while holding the
lock in the wakeup routines as the only way the race should be lost
would result in extra calls to sleepq_signal() or sleepq_broadcast().

Test Plan
  • My only testing has been to boot a GENERIC kernel with INVARIANTS, etc. and verify nothing blows up.
  • Benno saw this in the field and is doing internal testing.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

jhb retitled this revision from to Ensure cv_waiters is always updated when a thread awakens..
jhb updated this object.
jhb edited the test plan for this revision. (Show Details)
jhb added reviewers: kib, benno.

This patch (on 7-era code) is being stress-tested internally right now. It's slated to run over the weekend so I'll update next week on how it went.

What for cv_waiters is used ? Unless I miss something, it is there only to not call sleepq_broadcast() if no threads wait for the signal. Since the check is already racy, and since we taken the sleepq spinlock already anyway in cv_signal() etc. Wouldn't it be simpler, and probably the same fast to call sleepq_broadcast() unconditionally and remove cv_waiters ? We add unconditional function call,but also remove atomics (from the patch).

In D2427#44534, @kostikbel wrote:

What for cv_waiters is used ? Unless I miss something, it is there only to not call sleepq_broadcast() if no threads wait for the signal. Since the check is already racy, and since we taken the sleepq spinlock already anyway in cv_signal() etc. Wouldn't it be simpler, and probably the same fast to call sleepq_broadcast() unconditionally and remove cv_waiters ? We add unconditional function call,but also remove atomics (from the patch).

I see that this would be a revert of the r127954. It was done more than 10 years ago, the reasoning and tests which lead to the revision probably too hard to resurrect.

Well, if the atomics are more expensive than the function call then it would be better to remove cv_waiters, yes. In the case that there is no sleeper this means checking all sleep queues in the hash bucket to find a matching sleep queue (and once finding none, return). Note that much pre-SMPng code in FreeBSD that used tsleep often used WAITERS or WANTED flags to serve the same purpose by making calls to wakeup() conditional.

At the time cv_waiters was also very cheap to get right (we required callers to hold the lock across cv_signal/broadcast which has since been relaxed), and I think we also didn't have cv_wait_unlock(). However, neither of those are now true. I'm not completely sure I had firm numbers to back up the original change either FWIW. :-/

benno edited edge metadata.

No issues found over the weekend.

This revision is now accepted and ready to land.May 4 2015, 3:56 PM
This revision was automatically updated to reflect the committed changes.