bthread: skip signal_task when no worker is waiting by Lin807188583 · Pull Request #3418 · apache/brpc

Lin807188583 · 2026-07-29T03:35:45Z

Problem

ready_to_run* unconditionally calls signal_task() on every task-ready event, even when all workers are busy and nobody is parked in wait(). This results in unnecessary futex_wake syscalls that wake no thread, wasting CPU under load.

Solution

Add per-tag waiter counting via _tagged_waiter_num: ParkingLot::wait() tracks waiters entering/leaving (via parking_lot_waiter_add/sub out-of-line forwarders) BEFORE the state check. ready_to_run* checks has_waiting_workers(tag) before calling signal_task — if no worker of the given tag is parked, the wakeup is skipped (BAIDU_UNLIKELY marks the true branch as cold since under load all workers are busy).

Race-free design

The waiter_add() is called BEFORE the get_state() check in wait(). This ensures that any concurrent ready_to_run() that calls has_waiting_workers() will see the waiter even if it races between the state check and futex_wait(). If the state already changed (signal was sent), wait() undoes the tracking and returns immediately without sleeping. This eliminates the lost-wakeup window entirely.

Why it's safe

Tasks are already enqueued (push_rq) before the signal check, so they will be picked up by the next steal regardless of signaling.
_tagged_waiter_num uses relaxed atomics (it's a hint, not a synchronization primitive); the actual wake ordering is handled by parking_lot's existing futex mechanism.

Multi-tag benefit

Per-tag accounting avoids tag-A's ready_to_run() issuing a signal_task() just because tag-B has parked workers.

Tested: bthread_unittest 20/20 on Linux x86 (Ubuntu 24.04/GCC 13) and macOS arm64 (M4).

What problem does this PR solve?

Issue Number: resolve

Problem Summary:

What is changed and the side effects?

Changed:

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.

## Problem ready_to_run* unconditionally calls signal_task() on every task-ready event, even when all workers are busy and nobody is parked in wait(). This results in unnecessary futex_wake syscalls that wake no thread, wasting CPU under load. ## Solution Add per-tag waiter counting via _tagged_waiter_num: ParkingLot::wait() tracks waiters entering/leaving (via parking_lot_waiter_add/sub out-of-line forwarders) BEFORE the state check. ready_to_run* checks has_waiting_workers(tag) before calling signal_task — if no worker of the given tag is parked, the wakeup is skipped (BAIDU_UNLIKELY marks the true branch as cold since under load all workers are busy). ## Race-free design The waiter_add() is called BEFORE the get_state() check in wait(). This ensures that any concurrent ready_to_run() that calls has_waiting_workers() will see the waiter even if it races between the state check and futex_wait(). If the state already changed (signal was sent), wait() undoes the tracking and returns immediately without sleeping. This eliminates the lost-wakeup window entirely. ## Why it's safe - Tasks are already enqueued (push_rq) before the signal check, so they will be picked up by the next steal regardless of signaling. - _tagged_waiter_num uses relaxed atomics (it's a hint, not a synchronization primitive); the actual wake ordering is handled by parking_lot's existing futex mechanism. ## Multi-tag benefit Per-tag accounting avoids tag-A's ready_to_run() issuing a signal_task() just because tag-B has parked workers. Tested: bthread_unittest 20/20 on Linux x86 (Ubuntu 24.04/GCC 13) and macOS arm64 (M4).

Copilot

Pull request overview

This PR optimizes bthread scheduling by skipping TaskControl::signal_task() when no worker of the relevant tag is parked in ParkingLot::wait(), reducing unnecessary futex wake syscalls under load.

Changes:

Add per-tag parked-worker accounting in TaskControl and update ParkingLot::wait() to maintain the counters.
Gate TaskGroup::ready_to_run*() signaling on has_waiting_workers(tag) to skip wakeups when no same-tag worker is waiting.
Wire ParkingLot instances back to TaskControl during initialization so ParkingLot::wait() can update per-tag waiter counters.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File	Description
src/bthread/task_group.cpp	Skip `signal_task()` when `has_waiting_workers(_tag)` is false.
src/bthread/task_control.h	Add per-tag waiter counter APIs (`has_waiting_workers`, `waiter_add/sub`) and storage.
src/bthread/task_control.cpp	Define forwarders for `ParkingLot` waiter accounting; initialize/tag wiring for parking lots.
src/bthread/parking_lot.h	Track waiters per tag and forward add/sub events to `TaskControl`.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+    , _tagged_waiter_num(FLAGS_task_group_ntags)
    , _tag_cpus(FLAGS_task_group_ntags)
    , _tag_next_worker_id(FLAGS_task_group_ntags)
 {}


+    // Wire each ParkingLot back to this TaskControl so that ParkingLot::wait()
+    // can maintain the per-tag _tagged_waiter_num counter consulted by
+    // has_waiting_workers(tag). The outer index `i' is the owning tag.
+    for (size_t i = 0; i < _tagged_pl.size(); ++i) {
+        for (size_t j = 0; j < _pl_num_of_each_tag; ++j) {


+    bool has_waiting_workers(bthread_tag_t tag) const {
+        return tag < (bthread_tag_t)_tagged_waiter_num.size()
+               && _tagged_waiter_num[tag].load(butil::memory_order_relaxed) > 0;
+    }


+    void waiter_add(bthread_tag_t tag) {
+        _tagged_waiter_num[tag].fetch_add(1, butil::memory_order_relaxed);
+    }
+    void waiter_sub(bthread_tag_t tag) {
+        _tagged_waiter_num[tag].fetch_sub(1, butil::memory_order_relaxed);
+    }


+// Out-of-line helpers that forward to TaskControl's per-tag waiter accounting.
+// Defined in task_control.cpp (where TaskControl is complete), so that
+// parking_lot.h can keep TaskControl as an incomplete type and avoid a
+// circular include with task_control.h.
+void parking_lot_waiter_add(TaskControl* tc, bthread_tag_t tag);


Address Copilot review feedback on PR apache#3418: 1. Move set_task_control() BEFORE pthread_create() in TaskControl::init() to eliminate a race where workers could enter ParkingLot::wait() with _tc==nullptr, causing has_waiting_workers() to return false and signal_task() to be skipped — permanently sleeping the worker. 2. Explicitly zero-initialize _tagged_waiter_num elements in the TaskControl constructor body. butil::atomic's default constructor does not guarantee a zero value. 3. Add tag >= 0 bounds checks to has_waiting_workers(), waiter_add(), and waiter_sub() to prevent negative-tag (BTHREAD_TAG_INVALID=-1) array indexing (UB). 4. Include bthread/types.h directly in parking_lot.h for bthread_tag_t, making the header self-contained. Tested: bthread_butex_unittest 12/12 on macOS arm64.

…imization The per-tag has_waiting_workers() check in ready_to_run*() introduced a lost-wakeup race: 1. worker saves _last_pl_state via get_state() 2. worker scans queues, finds nothing 3. producer enqueues task 4. producer checks has_waiting_workers() -> false (waiter not registered yet) 5. producer skips signal_task() entirely, _pending_signal unchanged 6. worker registers as waiter, re-checks state -> unchanged -> sleeps 7. task stuck indefinitely The existing ParkingLot::signal() already handles this safely: it always updates _pending_signal (so the worker's re-check in step 6 detects the change) and only skips futex_wake when no waiter is present. The --parking_lot_no_signal_when_no_waiter flag already enables this per-lot optimization without the race. Revert all has_waiting_workers() gating in task_group.cpp (4 sites). Remove _tagged_waiter_num, waiter_add/sub, has_waiting_workers, set_task_control, and related forwarders from task_control.{h,cpp} and parking_lot.h. Restore parking_lot.h to its pre-optimization structure (retaining the pre-existing _no_signal_when_no_waiter fast path). Tested: bthread_butex_unittest 12/12 on macOS arm64.

Lin807188583 · 2026-07-29T17:13:05Z

确认存在丢唤醒竞态。worker 的"拍快照"和"登记 waiter"不是原子的，producer 卡在这个窗口判断 has_waiting_workers() 为 0 就跳过了 signal_task()，但 _pending_signal 没更新。应该始终更新 _pending_signal，只跳过 futex_wake。现有 --parking_lot_no_signal_when_no_waiter 已实现这个。先关闭

chenBright requested a review from Copilot July 29, 2026 06:03

Copilot started reviewing on behalf of chenBright July 29, 2026 06:03 View session

Copilot AI reviewed Jul 29, 2026

View reviewed changes

linc1231 added 2 commits July 30, 2026 00:29

Lin807188583 closed this Jul 29, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

bthread: skip signal_task when no worker is waiting - #3418

bthread: skip signal_task when no worker is waiting#3418
Lin807188583 wants to merge 3 commits into
apache:masterfrom
Lin807188583:pr-signal-task-optimization

Lin807188583 commented Jul 29, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Lin807188583 commented Jul 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

Lin807188583 commented Jul 29, 2026

Problem

Solution

Race-free design

Why it's safe

Multi-tag benefit

What problem does this PR solve?

What is changed and the side effects?

Check List:

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Lin807188583 commented Jul 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants