chenBright opened a new pull request, #3539:
URL: https://github.com/apache/brpc/pull/3539
### What problem does this PR solve?
Issue Number: resolve
Problem Summary:
When `--parking_lot_no_signal_when_no_waiter` is enabled,
`ParkingLot::signal()` avoids
calling `futex_wake_private()` when `_waiter_num` is zero.
However, the optimization introduces a Store-Load synchronization
requirement between
`signal()` and `wait()`:
- `signal()` increments `_pending_signal`, then checks `_waiter_num`.
- `wait()` increments `_waiter_num`, then lets `futex_wait_private()` check
`_pending_signal` and enqueue the worker.
The original relaxed waiter-counter accesses do not prevent both sides from
observing
stale values. As a result, the signaler may decide that no worker is waiting
while a worker
concurrently observes the old signal state and enters futex wait. No wakeup
is issued for
that worker, causing a lost wakeup until a later signal arrives.
Timing of the problematic interleaving:
```text
signaler thread waiter thread
--------------- -------------
_pending_signal += 2 (signal published)
observes old _pending_signal
_waiter_num += 1
load(_waiter_num) == 0
return without futex_wake_private()
futex_wait_private(
&_pending_signal, old_state)
-> state still matches old_state
-> worker sleeps
No futex wakeup is issued for this signal.
The worker remains parked until a subsequent signal wakes it up.
```
This is a Dekker-style Store-Load race: each side publishes its own state
and then
observes the other side's state. Without matching Store-Load ordering, both
sides may
observe the previous value.
### What is changed and the side effects?
Changed:
- Add a sequentially consistent fence in ParkingLot::signal() after
publishing the pending
signal and before loading `_waiter_num`.
- Add the matching sequentially consistent fence in ParkingLot::wait() after
incrementing
`_waiter_num` and before entering `futex_wait_private()`.
The two fences establish the required Store-Load ordering, ensuring that a
signaler
cannot skip `futex_wake_private()` while a concurrent waiter misses that
signal and
successfully parks.
Timing after the fix:
```text
signaler thread waiter thread
--------------- -------------
_pending_signal += 2 (release)
seq_cst fence
|
| The Store-Load ordering is established with the matching fence below.
| Therefore, both sides cannot simultaneously observe the other side's
| old value.
|
observes old
_pending_signal
_waiter_num += 1
seq_cst fence
|
| _waiter_num
publication is
| ordered before
futex_wait's
| state check.
v
load(_waiter_num)
|
+-- _waiter_num > 0 --------------------------> futex_wait_private(
| &_pending_signal,
old_state)
| -> waiter is queued /
waiting
| -> futex_wake_private()
wakes it
|
+-- _waiter_num == 0
|
+-- waiter has not published itself yet
|
+-- waiter subsequently checks _pending_signal
-> observes the updated state
-> futex_wait_private() returns immediately
-> worker does not sleep
```
Equivalently, the two valid outcomes are:
```text
Case 1: waiter is visible first
--------------------------------
waiter: _waiter_num += 1; seq_cst fence
signaler: _pending_signal += 2; seq_cst fence; load(_waiter_num) > 0
signaler: futex_wake_private()
result: the waiter is woken, or observes the changed futex value and does
not sleep.
Case 2: signal is visible first
--------------------------------
signaler: _pending_signal += 2; seq_cst fence; load(_waiter_num) == 0
signaler: return without futex_wake_private()
waiter: _waiter_num += 1; seq_cst fence; futex_wait_private(..., old_state)
result: futex value no longer matches old_state; futex_wait_private()
returns
immediately and the waiter does not sleep.
```
Side effects:
- Performance effects:
- Breaking backward compatibility:
---
### Check List:
- Please make sure your changes are compilable.
- When providing us with a new feature, it is best to add related tests.
- Please follow [Contributor Covenant Code of
Conduct](https://github.com/apache/brpc/blob/master/CODE_OF_CONDUCT.md).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
