SharedMutex potential lost wakeup with exactly 3 or 4 contending writers
authorNathan Bronson <ngbronson@fb.com>
Fri, 10 Apr 2015 02:45:58 +0000 (19:45 -0700)
committerViswanath Sivakumar <viswanath@fb.com>
Fri, 10 Apr 2015 03:35:19 +0000 (20:35 -0700)
commitb927c0557670eee673ca64a782d58d23bd1dab5b
tree23bb7bbf3ac93a4184992ce5a5a02ea743e99458
parente426c673e9cf39a1cf9011bebbee540b775a0911
SharedMutex potential lost wakeup with exactly 3 or 4 contending writers

Summary:
SharedMutex used a saturating counter that records the number of
waiting lock() calls, but an ABA problem on futexWait could lead to a lost
wakeup when there was exactly 3 or 4 threads contending on the RW lock
in W mode.  This diff changes the kWaitingE count to be heuristic (it is
possible that the count says 1 but there are two waiters), saturates at
2 instead of 3 (because there is no benefit from differentiating those
two), and doesn't decrement the count on a successful wakeup.

Also, I noticed while debugging this that boost::noncopyable was causing
SharedMutex to be 8 bytes when it should only be 4.

One way the wakeup could be lost in the old code:

1. A calls lock()
2. A updates state <- kHasE
3. A returns
4. B calls lock()
5. B spins
6. B updates state <- kHasE + 1 * kIncrWaitingE
7. A calls unlock()
8. A updates state <- 0
9. A calls futexWake(), which returns 0
10. A calls lock()
11. A updates state <- kHasE
12. A returns
13. C calls lock()
14. C spins
15. C updates state <- kHasE + 1 * kIncrWaitingE
16. C calls futexWait, expecting kHasE + 1 * kIncrWaitingE
17. B calls futexWait, expecting kHasE + 1 * kIncrWaitingE
18. A calls unlock()
19. A updates state <- 0
20. A calls futexWake(), which returns 1
21. C receives the wakeup
22. C updates state <- kHasE
23. C returns
24. C calls unlock()
25. C updates state <- 0

B missed the wakeup that was intended for it (sent at step 9, wait
started at step 17), but went to sleep anyway because it saw the write
state at step 17. Now there are two waiters but only 1 recorded in the
SharedMutex, at which point failure is inevitable.

Test Plan:
1. DeterministicSchedule test using uniformSubset that can repro the problem
2. Test in production scenario that produced occasional deadlocks under high stress

Reviewed By: yfeldblum@fb.com

Subscribers: folly-diffs@, yfeldblum, chalfant

FB internal diff: D1980210

Tasks: 6720328

Signature: t1:1980210:1428623932:ef1c00c3f88154578b2b253ac0cfdbadf9f31d8c
folly/experimental/SharedMutex.h
folly/experimental/test/SharedMutexTest.cpp