You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brpc.apache.org by "two-headBoy (via GitHub)" <gi...@apache.org> on 2026/10/20 13:41:57 UTC
[PR] Fix livelock when all TaskGroup runqueues are full (#1038) (brpc)
two-headBoy opened a new pull request, #3633:
URL: https://github.com/apache/brpc/pull/3633
### What problem does this PR solve?
Issue Number: Fixes #1038
Problem Summary:
When `_rq` is full, `push_rq()` sleeps the worker thread and retries. The
owner worker is the only consumer of `_rq` (other workers only steal), so
once every worker spins there, no one drains any queue and the whole
scheduler livelocks.
### What is changed and the side effects?
Changed:
When `_rq` is full, pop one pending task and run it in place, then retry
the push. The pop frees one slot, which guarantees the push of the task
being scheduled out succeeds, since `_rq` has a single producer and
stealers never push; the original push is then retried until the queue
has room. The task being scheduled out is saved with `current_tid()`
(remained-callbacks re-enter `push_rq()` on the next task's stack).
Pthread-mode tasks cannot be switched out, so they fall back to waiting,
and a pthread-mode task popped by the main task is put back instead of
being silently dropped.
Verified with a recursive spawner repro (every task spawns more tasks until stopped):
before the fix all workers froze in the usleep loop with CPU ~0%;
after the fix workers kept draining the queues (CPU ~400%) and the echo_c++ smoke test passes.
```cpp
// Repro for brpc issue #1038: all local run queues full -> livelock.
#include <bthread/bthread.h>
#include <stdio.h>
#include <time.h>
#include <unistd.h>
static void* spawner(void*) {
while (true) {
bthread_t tid;
// The spawned task is itself a spawner: a worker that steals it
// is "converted" into a producer.
bthread_start_background(&tid, nullptr, spawner, nullptr);
}
return nullptr;
}
int main() {
// The concurrency floor is BTHREAD_MIN_CONCURRENCY (=3+epoll, 4 on
// Linux); setting 1 fails with EINVAL. Use the floor and seed every
// worker with one spawner.
const int n = BTHREAD_MIN_CONCURRENCY;
bthread_setconcurrency(n);
for (int i = 0; i < n; ++i) {
bthread_t tid;
bthread_start_background(&tid, nullptr, spawner, nullptr);
}
fprintf(stderr, "spawner x%d started, waiting for livelock...\n", n);
time_t start = time(nullptr);
while (true) {
sleep(1);
fprintf(stderr, "main alive, %lds elapsed\n", time(nullptr) - start);
}
return 0;
}
```
Side effects:
- Performance effects: none on the hot path; only runs when the queue is full.
- Breaking backward compatibility: none.
Note: this fix guarantees scheduler liveness, not per-task completion
deadlines; rate limiting under sustained overload remains the
application's responsibility. The window where every worker is inside a
pthread-mode task when its queue fills is left as-is, consistent with
the existing stance for pthread-mode tasks in `exchange()`.
---
### 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 `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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix livelock when all TaskGroup runqueues are full (#1038) (brpc)
Posted by "wwbmmm (via GitHub) via dev" <de...@brpc.apache.org>.
wwbmmm commented on PR #3633:
URL: https://github.com/apache/brpc/pull/3633#issuecomment-6554163855
@chenBright What do you think about this PR?
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix livelock when all TaskGroup runqueues are full (#1038) (brpc)
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3633:
URL: https://github.com/apache/brpc/pull/3633#discussion_r4336688130
##########
src/bthread/task_group_inl.h:
##########
@@ -93,11 +93,65 @@ inline void TaskGroup::push_rq(bthread_t tid) {
// brpc)
flush_nosignal_tasks();
LOG_EVERY_SECOND(ERROR) << "_rq is full, capacity=" << _rq.capacity();
- // TODO(gejun): May cause deadlock when all workers are spinning here.
- // A better solution is to pop and run existing bthreads, however which
- // make set_remained()-callbacks do context switches and need extensive
- // reviews on related code.
- ::usleep(1000);
+ if (is_current_pthread_task() && !is_current_main_task()) {
+ // A pthread-mode bthread runs on _main_stack and cannot be
+ // scheduled out, fall back to waiting.
+ ::usleep(1000);
+ continue;
+ }
+ bthread_t run_tid = 0;
+#ifndef BTHREAD_FAIR_WSQ
+ const bool popped = _rq.pop(&run_tid);
+#else
+ const bool popped = _rq.steal(&run_tid);
+#endif
+ if (!popped) {
+ // The queue was drained by stealers between the failed push and
+ // the pop, retrying the push will succeed.
+ continue;
+ }
+ if (is_current_main_task() &&
+ address_meta(run_tid)->attr.stack_type == BTHREAD_STACKTYPE_PTHREAD) {
+ // pthread-mode tasks are run inline on _main_stack by the
+ // dispatch loops (run_main_task/task_runner). Switching to one
+ // from the main task itself causes no stack jump and would
+ // silently drop it, put it back and wait; stealers may run it
+ // on other workers.
+ _rq.push(run_tid);
Review Comment:
`_rq.push()` appears to be a fallible operation (it’s checked elsewhere in this function). Ignoring the return value here risks silently dropping `run_tid` if the push fails (e.g., due to unexpected queue state or implementation changes). Prefer checking the return and handling failure explicitly (retry, assert with a clear message, or take an alternate path) to avoid task loss.
##########
src/bthread/task_group_inl.h:
##########
@@ -93,11 +93,65 @@ inline void TaskGroup::push_rq(bthread_t tid) {
// brpc)
flush_nosignal_tasks();
LOG_EVERY_SECOND(ERROR) << "_rq is full, capacity=" << _rq.capacity();
- // TODO(gejun): May cause deadlock when all workers are spinning here.
- // A better solution is to pop and run existing bthreads, however which
- // make set_remained()-callbacks do context switches and need extensive
- // reviews on related code.
- ::usleep(1000);
+ if (is_current_pthread_task() && !is_current_main_task()) {
+ // A pthread-mode bthread runs on _main_stack and cannot be
+ // scheduled out, fall back to waiting.
+ ::usleep(1000);
+ continue;
+ }
+ bthread_t run_tid = 0;
+#ifndef BTHREAD_FAIR_WSQ
+ const bool popped = _rq.pop(&run_tid);
+#else
+ const bool popped = _rq.steal(&run_tid);
+#endif
+ if (!popped) {
+ // The queue was drained by stealers between the failed push and
+ // the pop, retrying the push will succeed.
+ continue;
+ }
+ if (is_current_main_task() &&
+ address_meta(run_tid)->attr.stack_type == BTHREAD_STACKTYPE_PTHREAD) {
+ // pthread-mode tasks are run inline on _main_stack by the
+ // dispatch loops (run_main_task/task_runner). Switching to one
+ // from the main task itself causes no stack jump and would
+ // silently drop it, put it back and wait; stealers may run it
+ // on other workers.
+ _rq.push(run_tid);
+ ::usleep(1000);
+ continue;
+ }
Review Comment:
This branch makes no progress toward freeing space for the original `tid` enqueue: it pops one item and immediately pushes it back, leaving `_rq` full again, then sleeps. If stealers aren’t draining (the exact failure mode this PR targets), `push_rq()` can spin/sleep indefinitely here. A concrete fix is to use the freed slot to enqueue `tid` first (so the caller can make progress), and only then handle `run_tid` (e.g., keep `run_tid` locally and retry re-queueing it later, or search for a non-pthread task to run inline instead of immediately pushing the pthread-mode one back).
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix livelock when all TaskGroup runqueues are full (#1038) (brpc)
Posted by "chenBright (via GitHub) via dev" <de...@brpc.apache.org>.
chenBright commented on code in PR #3633:
URL: https://github.com/apache/brpc/pull/3633#discussion_r4436113448
##########
src/bthread/task_group.cpp:
##########
@@ -848,13 +851,16 @@ void TaskGroup::ready_to_run(TaskMeta* meta, bool nosignal) {
_control->_task_tracer.set_status(TASK_STATUS_READY, meta);
#endif // BRPC_BTHREAD_TRACER
push_rq(meta->tid);
+ // push_rq() may switch context and the current task may be resumed by
+ // another group. Use the current group for all subsequent accounting.
+ TaskGroup* g = BAIDU_GET_VOLATILE_THREAD_LOCAL(tls_task_group);
Review Comment:
push_rq() can now suspend the caller and resume it on another worker. Updating the local pointer in ready_to_run() does not update its callers: after butex_wake_except() wakes many joiners, task_runner() still uses its old g to install _release_last_context and call ending_sched().
##########
src/bthread/task_group_inl.h:
##########
@@ -83,22 +83,138 @@ inline void TaskGroup::sched_to(TaskGroup** pg, bthread_t next_tid) {
}
inline void TaskGroup::push_rq(bthread_t tid) {
- while (!_rq.push(tid)) {
+ TaskGroup* g = this;
+ while (!g->_rq.push(tid)) {
// Created too many bthreads: a promising approach is to insert the
// task into another TaskGroup, but we don't use it because:
// * There're already many bthreads to run, inserting the bthread
// into other TaskGroup does not help.
// * Insertions into other TaskGroups perform worse when all workers
// are busy at creating bthreads (proved by test_input_messenger in
// brpc)
- flush_nosignal_tasks();
- LOG_EVERY_SECOND(ERROR) << "_rq is full, capacity=" << _rq.capacity();
- // TODO(gejun): May cause deadlock when all workers are spinning here.
- // A better solution is to pop and run existing bthreads, however which
- // make set_remained()-callbacks do context switches and need extensive
- // reviews on related code.
- ::usleep(1000);
+ g->flush_nosignal_tasks();
+ LOG_EVERY_SECOND(ERROR) << "_rq is full, capacity=" << g->_rq.capacity();
+ const auto pop_task = [g](bthread_t* t) -> bool {
+#ifndef BTHREAD_FAIR_WSQ
+ return g->_rq.pop(t);
+#else
+ return g->_rq.steal(t);
+#endif
+ };
+ if (g->_sched_recursive_guard != 0) {
+ // Remained callbacks run with the guard held, either in
+ // sched_to()'s remained loop or at task_runner() entry. Re-entering
+ // sched_to() here would be recursive, while waiting here can
+ // livelock if every worker is running a remained callback. Make
+ // room for |tid| and requeue the popped task after the guard is
+ // balanced.
+ CHECK(g->_pending_rq_tid == INVALID_BTHREAD);
+ bthread_t pending_tid;
+ if (!pop_task(&pending_tid)) {
+ continue;
+ }
+ g->_pending_rq_tid = pending_tid;
+ const bool pushed = g->_rq.push(tid);
+ CHECK(pushed) << "push_rq: failed to push after popping a task";
+ return;
+ }
+ if (g->is_current_pthread_task() && !g->is_current_main_task()) {
+ // A pthread-mode bthread runs on _main_stack and cannot be
+ // scheduled out, fall back to waiting.
+ ::usleep(1000);
+ continue;
+ }
+ bthread_t run_tid = 0;
+ if (!pop_task(&run_tid)) {
+ // The queue was drained by stealers between the failed push and
+ // the pop, retrying the push will succeed.
+ continue;
+ }
+ if (g->is_current_main_task() &&
+ address_meta(run_tid)->attr.stack_type == BTHREAD_STACKTYPE_PTHREAD) {
+ // pthread-mode tasks are run inline on _main_stack by the
+ // dispatch loops (run_main_task/task_runner). Switching to one
+ // from the main task itself causes no stack jump and would
+ // silently drop it, so it cannot be run here. But pushing it
+ // straight back makes the LIFO pop() return this same task
+ // forever, hiding any runnable task sitting deeper in the
+ // queue. Hold pthread-mode tasks aside and keep popping until
+ // a runnable one shows up.
+ // Bounded budget: the array must live on the worker's stack
+ // (no allocation on a resource-exhaustion path), and held
+ // tasks are invisible to stealers, so keeping the list short
+ // limits how long they can be delayed. 16 * sizeof(bthread_t)
+ // == 128 bytes == two cache lines.
+ bthread_t pending[16];
+ int npending = 0;
+ pending[npending++] = run_tid;
+ bool found = false;
+ while (npending < 16 && pop_task(&run_tid)) {
+ if (address_meta(run_tid)->attr.stack_type !=
+ BTHREAD_STACKTYPE_PTHREAD) {
+ found = true;
+ break;
+ }
+ pending[npending++] = run_tid;
+ }
+ // Push the held tasks back. The pops above freed at least
+ // |npending| slots, _rq has a single producer (this worker)
+ // and stealers never push, so these pushes must succeed.
+ // Reverse order restores their relative order in the queue.
+ for (int i = npending - 1; i >= 0; --i) {
+ const bool pushed = g->_rq.push(pending[i]);
+ CHECK(pushed) << "push_rq: failed to put back held task";
+ }
+ if (!found) {
+ // Everything within reach is pthread-mode: wait for
+ // stealers to drain the queue.
+ ::usleep(1000);
+ continue;
+ }
+ // Fall through: run_tid is a runnable task, run it inline
+ // below to free a slot for |tid|.
+ }
+ // Run the popped bthread in place instead of spinning, otherwise
+ // all workers may spin here and no one drains the queues any more.
+ // The pop also frees one slot, which guarantees that the deferred
+ // push of the current task succeeds: _rq has a single producer (the
+ // owning worker) and stealers never push.
+ // |args| lives on the stack that sched_to() is about to freeze and is
+ // read by the remained-callback after the switch, so it must stay in
+ // this scope: if it were declared inside the branch below, its slot
+ // could be reused before the callback has finished reading it.
+ ReadyToRunArgs args = { g->tag(), nullptr, false };
+ if (!g->is_current_main_task()) {
+ // Preserve the task being scheduled out. The push is deferred to
+ // a remained-callback: publishing a still-running task into _rq
+ // lets a stealer jump into it and run the same context
+ // concurrently on another worker. The callback runs on the stack
+ // of |run_tid| after sched_to() has frozen the current context,
+ // same as exchange(). Capture the meta now, current_tid() is
+ // |run_tid| by the time the callback runs.
+ // The callback may re-enter push_rq() while _rq is still full;
+ // the guard branch above makes room without switching and defers
+ // the popped task until the guard is balanced.
+ args.meta = g->_cur_meta;
+ g->set_remained(ready_to_run_in_worker_ignoresignal, &args);
+ }
+ // The main task is never queued; ending_sched() resumes it via
+ // _main_tid when the queue drains. Jumping to a pthread-mode task
+ // from a normal bthread resumes the dispatch loop on _main_stack
+ // which runs it inline, same as ending_sched() does.
+ sched_to(&g, run_tid);
Review Comment:
This switch makes bthread_start_background() an implicit scheduling point. Existing callers, including ExecutionQueueBase::start_execute(), deliberately use background submission because they may hold a pthread mutex.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix livelock when all TaskGroup runqueues are full (#1038) (brpc)
Posted by "two-headBoy (via GitHub) via dev" <de...@brpc.apache.org>.
two-headBoy commented on PR #3633:
URL: https://github.com/apache/brpc/pull/3633#issuecomment-6548427808
Thanks for the detailed reviews. The latest five commits address the identified issues:
- `_sched_recursive_guard` is now maintained in all build types and remains held throughout remained-callback execution, preventing recursive`sched_to()` calls.
- When`push_rq()` hits a full queue inside a remained callback, it makes room without switching context and defers the popped task until the guard is balanced. The deferred task is requeued from both the fresh-stack and resumed-stack paths.
- The pop result is now stored in a temporary and saved as pending only after a successful pop, so a pop/steal race cannot leave a stolen TID in`_pending_rq_tid` .
- After a context switch,`push_rq()` uses the`TaskGroup` returned through`sched_to()` , and`ready_to_run()` re-fetches the current group before updating nosignal accounting or signalling, avoiding stale-group access after stealing.
- Added`test/bthread_runqueue_unittest.cpp` , covering runqueue saturation with a single worker and multi-worker execution with yields and stealing. The test checks completion, start failures, and exactly-once execution of all tasks.
The regression test passes.
cc @wwbmmm
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix livelock when all TaskGroup runqueues are full (#1038) (brpc)
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3633:
URL: https://github.com/apache/brpc/pull/3633#discussion_r4348351312
##########
src/bthread/task_group_inl.h:
##########
@@ -93,11 +93,100 @@ inline void TaskGroup::push_rq(bthread_t tid) {
// brpc)
flush_nosignal_tasks();
LOG_EVERY_SECOND(ERROR) << "_rq is full, capacity=" << _rq.capacity();
- // TODO(gejun): May cause deadlock when all workers are spinning here.
- // A better solution is to pop and run existing bthreads, however which
- // make set_remained()-callbacks do context switches and need extensive
- // reviews on related code.
- ::usleep(1000);
+ if (is_current_pthread_task() && !is_current_main_task()) {
+ // A pthread-mode bthread runs on _main_stack and cannot be
+ // scheduled out, fall back to waiting.
+ ::usleep(1000);
+ continue;
+ }
+ const auto pop_task = [this](bthread_t* t) -> bool {
+#ifndef BTHREAD_FAIR_WSQ
+ return _rq.pop(t);
+#else
+ return _rq.steal(t);
+#endif
+ };
+ bthread_t run_tid = 0;
+ if (!pop_task(&run_tid)) {
+ // The queue was drained by stealers between the failed push and
+ // the pop, retrying the push will succeed.
+ continue;
+ }
+ if (is_current_main_task() &&
+ address_meta(run_tid)->attr.stack_type == BTHREAD_STACKTYPE_PTHREAD) {
+ // pthread-mode tasks are run inline on _main_stack by the
+ // dispatch loops (run_main_task/task_runner). Switching to one
+ // from the main task itself causes no stack jump and would
+ // silently drop it, so it cannot be run here. But pushing it
+ // straight back makes the LIFO pop() return this same task
+ // forever, hiding any runnable task sitting deeper in the
+ // queue. Hold pthread-mode tasks aside and keep popping until
+ // a runnable one shows up.
+ // Bounded budget: the array must live on the worker's stack
+ // (no allocation on a resource-exhaustion path), and held
+ // tasks are invisible to stealers, so keeping the list short
+ // limits how long they can be delayed. 16 * sizeof(bthread_t)
+ // == 128 bytes == two cache lines.
+ bthread_t pending[16];
+ int npending = 0;
+ pending[npending++] = run_tid;
+ bool found = false;
+ while (npending < 16 && pop_task(&run_tid)) {
+ if (address_meta(run_tid)->attr.stack_type !=
+ BTHREAD_STACKTYPE_PTHREAD) {
+ found = true;
+ break;
+ }
+ pending[npending++] = run_tid;
+ }
Review Comment:
The scan for a non-pthread task is capped at 16 entries. If there are >16 consecutive pthread-mode tasks near the pop end, `found` stays false even when a runnable non-pthread task exists deeper in `_rq`, and this path falls back to sleeping—potentially recreating the original ‘no one drains queues’ failure mode if stealers aren’t actively draining. To preserve the liveness guarantee, consider a strategy that eventually reaches deeper runnable work (e.g., iterative batches until a non-pthread task is found, or temporarily rotating/moving aside pthread-mode tasks in a way that allows deeper non-pthread tasks to surface) while still avoiding heap allocation.
##########
src/bthread/task_group_inl.h:
##########
@@ -93,11 +93,100 @@ inline void TaskGroup::push_rq(bthread_t tid) {
// brpc)
flush_nosignal_tasks();
LOG_EVERY_SECOND(ERROR) << "_rq is full, capacity=" << _rq.capacity();
- // TODO(gejun): May cause deadlock when all workers are spinning here.
- // A better solution is to pop and run existing bthreads, however which
- // make set_remained()-callbacks do context switches and need extensive
- // reviews on related code.
- ::usleep(1000);
+ if (is_current_pthread_task() && !is_current_main_task()) {
+ // A pthread-mode bthread runs on _main_stack and cannot be
+ // scheduled out, fall back to waiting.
+ ::usleep(1000);
+ continue;
+ }
+ const auto pop_task = [this](bthread_t* t) -> bool {
+#ifndef BTHREAD_FAIR_WSQ
+ return _rq.pop(t);
+#else
+ return _rq.steal(t);
+#endif
+ };
+ bthread_t run_tid = 0;
+ if (!pop_task(&run_tid)) {
+ // The queue was drained by stealers between the failed push and
+ // the pop, retrying the push will succeed.
+ continue;
+ }
+ if (is_current_main_task() &&
+ address_meta(run_tid)->attr.stack_type == BTHREAD_STACKTYPE_PTHREAD) {
+ // pthread-mode tasks are run inline on _main_stack by the
+ // dispatch loops (run_main_task/task_runner). Switching to one
+ // from the main task itself causes no stack jump and would
+ // silently drop it, so it cannot be run here. But pushing it
+ // straight back makes the LIFO pop() return this same task
+ // forever, hiding any runnable task sitting deeper in the
+ // queue. Hold pthread-mode tasks aside and keep popping until
+ // a runnable one shows up.
+ // Bounded budget: the array must live on the worker's stack
+ // (no allocation on a resource-exhaustion path), and held
+ // tasks are invisible to stealers, so keeping the list short
+ // limits how long they can be delayed. 16 * sizeof(bthread_t)
+ // == 128 bytes == two cache lines.
+ bthread_t pending[16];
+ int npending = 0;
+ pending[npending++] = run_tid;
+ bool found = false;
+ while (npending < 16 && pop_task(&run_tid)) {
+ if (address_meta(run_tid)->attr.stack_type !=
+ BTHREAD_STACKTYPE_PTHREAD) {
+ found = true;
+ break;
+ }
+ pending[npending++] = run_tid;
+ }
+ // Push the held tasks back. The pops above freed at least
+ // |npending| slots, _rq has a single producer (this worker)
+ // and stealers never push, so these pushes must succeed.
+ // Reverse order restores their relative order in the queue.
+ for (int i = npending - 1; i >= 0; --i) {
+ const bool pushed = _rq.push(pending[i]);
+ CHECK(pushed) << "push_rq: failed to put back held task";
+ }
+ if (!found) {
+ // Everything within reach is pthread-mode: wait for
+ // stealers to drain the queue.
+ ::usleep(1000);
+ continue;
+ }
Review Comment:
The scan for a non-pthread task is capped at 16 entries. If there are >16 consecutive pthread-mode tasks near the pop end, `found` stays false even when a runnable non-pthread task exists deeper in `_rq`, and this path falls back to sleeping—potentially recreating the original ‘no one drains queues’ failure mode if stealers aren’t actively draining. To preserve the liveness guarantee, consider a strategy that eventually reaches deeper runnable work (e.g., iterative batches until a non-pthread task is found, or temporarily rotating/moving aside pthread-mode tasks in a way that allows deeper non-pthread tasks to surface) while still avoiding heap allocation.
##########
src/bthread/task_group_inl.h:
##########
@@ -93,11 +93,100 @@ inline void TaskGroup::push_rq(bthread_t tid) {
// brpc)
flush_nosignal_tasks();
LOG_EVERY_SECOND(ERROR) << "_rq is full, capacity=" << _rq.capacity();
- // TODO(gejun): May cause deadlock when all workers are spinning here.
- // A better solution is to pop and run existing bthreads, however which
- // make set_remained()-callbacks do context switches and need extensive
- // reviews on related code.
- ::usleep(1000);
+ if (is_current_pthread_task() && !is_current_main_task()) {
+ // A pthread-mode bthread runs on _main_stack and cannot be
+ // scheduled out, fall back to waiting.
+ ::usleep(1000);
+ continue;
+ }
+ const auto pop_task = [this](bthread_t* t) -> bool {
+#ifndef BTHREAD_FAIR_WSQ
+ return _rq.pop(t);
+#else
Review Comment:
Under `BTHREAD_FAIR_WSQ`, `pop_task()` uses `_rq.steal()` rather than an owner-side pop. If `steal()` has different semantics (end of deque, fairness, or concurrency assumptions) than the owner-consumption path, this may change scheduling behavior in the overload path. Prefer using the owner pop API if it exists in the fair-WSQ implementation; otherwise, add an explicit comment explaining why `steal()` is the correct/safe choice for the owning worker here.
##########
src/bthread/task_group_inl.h:
##########
@@ -93,11 +93,100 @@ inline void TaskGroup::push_rq(bthread_t tid) {
// brpc)
flush_nosignal_tasks();
LOG_EVERY_SECOND(ERROR) << "_rq is full, capacity=" << _rq.capacity();
- // TODO(gejun): May cause deadlock when all workers are spinning here.
- // A better solution is to pop and run existing bthreads, however which
- // make set_remained()-callbacks do context switches and need extensive
- // reviews on related code.
- ::usleep(1000);
+ if (is_current_pthread_task() && !is_current_main_task()) {
+ // A pthread-mode bthread runs on _main_stack and cannot be
+ // scheduled out, fall back to waiting.
+ ::usleep(1000);
+ continue;
+ }
+ const auto pop_task = [this](bthread_t* t) -> bool {
+#ifndef BTHREAD_FAIR_WSQ
+ return _rq.pop(t);
+#else
+ return _rq.steal(t);
+#endif
+ };
+ bthread_t run_tid = 0;
+ if (!pop_task(&run_tid)) {
+ // The queue was drained by stealers between the failed push and
+ // the pop, retrying the push will succeed.
+ continue;
+ }
+ if (is_current_main_task() &&
+ address_meta(run_tid)->attr.stack_type == BTHREAD_STACKTYPE_PTHREAD) {
+ // pthread-mode tasks are run inline on _main_stack by the
+ // dispatch loops (run_main_task/task_runner). Switching to one
+ // from the main task itself causes no stack jump and would
+ // silently drop it, so it cannot be run here. But pushing it
+ // straight back makes the LIFO pop() return this same task
+ // forever, hiding any runnable task sitting deeper in the
+ // queue. Hold pthread-mode tasks aside and keep popping until
+ // a runnable one shows up.
+ // Bounded budget: the array must live on the worker's stack
+ // (no allocation on a resource-exhaustion path), and held
+ // tasks are invisible to stealers, so keeping the list short
+ // limits how long they can be delayed. 16 * sizeof(bthread_t)
+ // == 128 bytes == two cache lines.
+ bthread_t pending[16];
+ int npending = 0;
+ pending[npending++] = run_tid;
+ bool found = false;
+ while (npending < 16 && pop_task(&run_tid)) {
+ if (address_meta(run_tid)->attr.stack_type !=
+ BTHREAD_STACKTYPE_PTHREAD) {
+ found = true;
+ break;
+ }
+ pending[npending++] = run_tid;
+ }
+ // Push the held tasks back. The pops above freed at least
+ // |npending| slots, _rq has a single producer (this worker)
+ // and stealers never push, so these pushes must succeed.
+ // Reverse order restores their relative order in the queue.
+ for (int i = npending - 1; i >= 0; --i) {
+ const bool pushed = _rq.push(pending[i]);
+ CHECK(pushed) << "push_rq: failed to put back held task";
Review Comment:
This code path is explicitly a resource-exhaustion/overload path (full runqueue). A `CHECK(pushed)` would terminate the process if the assumption about guaranteed capacity is violated (e.g., if a future queue implementation changes producer guarantees or push can fail for reasons other than ‘full’). Consider making this non-fatal: retry push in a bounded loop, or fall back to `usleep(1000)` + `continue` with an error log when `pushed` is false, so overload doesn’t escalate into a hard crash.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix livelock when all TaskGroup runqueues are full (#1038) (brpc)
Posted by "two-headBoy (via GitHub) via dev" <de...@brpc.apache.org>.
two-headBoy closed pull request #3633: Fix livelock when all TaskGroup runqueues are full (#1038)
URL: https://github.com/apache/brpc/pull/3633
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix livelock when all TaskGroup runqueues are full (#1038) (brpc)
Posted by "wwbmmm (via GitHub)" <gi...@apache.org>.
wwbmmm commented on code in PR #3633:
URL: https://github.com/apache/brpc/pull/3633#discussion_r4348993148
##########
src/bthread/task_group_inl.h:
##########
@@ -93,11 +93,100 @@ inline void TaskGroup::push_rq(bthread_t tid) {
// brpc)
flush_nosignal_tasks();
LOG_EVERY_SECOND(ERROR) << "_rq is full, capacity=" << _rq.capacity();
- // TODO(gejun): May cause deadlock when all workers are spinning here.
- // A better solution is to pop and run existing bthreads, however which
- // make set_remained()-callbacks do context switches and need extensive
- // reviews on related code.
- ::usleep(1000);
+ if (is_current_pthread_task() && !is_current_main_task()) {
+ // A pthread-mode bthread runs on _main_stack and cannot be
+ // scheduled out, fall back to waiting.
+ ::usleep(1000);
+ continue;
+ }
+ const auto pop_task = [this](bthread_t* t) -> bool {
+#ifndef BTHREAD_FAIR_WSQ
+ return _rq.pop(t);
+#else
+ return _rq.steal(t);
+#endif
+ };
+ bthread_t run_tid = 0;
+ if (!pop_task(&run_tid)) {
+ // The queue was drained by stealers between the failed push and
+ // the pop, retrying the push will succeed.
+ continue;
+ }
+ if (is_current_main_task() &&
+ address_meta(run_tid)->attr.stack_type == BTHREAD_STACKTYPE_PTHREAD) {
+ // pthread-mode tasks are run inline on _main_stack by the
+ // dispatch loops (run_main_task/task_runner). Switching to one
+ // from the main task itself causes no stack jump and would
+ // silently drop it, so it cannot be run here. But pushing it
+ // straight back makes the LIFO pop() return this same task
+ // forever, hiding any runnable task sitting deeper in the
+ // queue. Hold pthread-mode tasks aside and keep popping until
+ // a runnable one shows up.
+ // Bounded budget: the array must live on the worker's stack
+ // (no allocation on a resource-exhaustion path), and held
+ // tasks are invisible to stealers, so keeping the list short
+ // limits how long they can be delayed. 16 * sizeof(bthread_t)
+ // == 128 bytes == two cache lines.
+ bthread_t pending[16];
+ int npending = 0;
+ pending[npending++] = run_tid;
+ bool found = false;
+ while (npending < 16 && pop_task(&run_tid)) {
+ if (address_meta(run_tid)->attr.stack_type !=
+ BTHREAD_STACKTYPE_PTHREAD) {
+ found = true;
+ break;
+ }
+ pending[npending++] = run_tid;
+ }
+ // Push the held tasks back. The pops above freed at least
+ // |npending| slots, _rq has a single producer (this worker)
+ // and stealers never push, so these pushes must succeed.
+ // Reverse order restores their relative order in the queue.
+ for (int i = npending - 1; i >= 0; --i) {
+ const bool pushed = _rq.push(pending[i]);
+ CHECK(pushed) << "push_rq: failed to put back held task";
+ }
+ if (!found) {
+ // Everything within reach is pthread-mode: wait for
+ // stealers to drain the queue.
Review Comment:
[replied by brpc-oncall robot] push_rq() is also called from remained callbacks (ready_to_run_in_worker, ready_to_run_in_worker_ignoresignal, priority_to_run). Those callbacks run at the beginning of the next task in task_runner(), and inside sched_to() right after jump_stack() — in both cases `_sched_recursive_guard` has already been incremented by the switch that led here and is decremented only *after* the remained loop. Calling sched_to() from there nests the switch, so the guard goes 1 -> 2 and sched_to() hits `LOG(FATAL) << "Recursively(...) call sched_to"` in non-NDEBUG builds.
This is not a corner case: with the repro in the PR description (every task spawns children until _rq is full), the parent's `ready_to_run()` is executed as the remained callback of the child that was just started, and lands directly in this full-queue branch. The removed TODO warned about exactly this ("make set_remained()-callbacks do context switches and need extensive reviews").
Please either detect that push_rq() is running inside a remained callback and fall back to waiting there (or defer the drained task to the outer scheduling loop), or move the drain-and-run logic out of push_rq() into the paths that are known not to be inside a switch.
##########
src/bthread/task_group_inl.h:
##########
@@ -93,11 +93,100 @@ inline void TaskGroup::push_rq(bthread_t tid) {
// brpc)
flush_nosignal_tasks();
LOG_EVERY_SECOND(ERROR) << "_rq is full, capacity=" << _rq.capacity();
- // TODO(gejun): May cause deadlock when all workers are spinning here.
- // A better solution is to pop and run existing bthreads, however which
- // make set_remained()-callbacks do context switches and need extensive
- // reviews on related code.
- ::usleep(1000);
+ if (is_current_pthread_task() && !is_current_main_task()) {
+ // A pthread-mode bthread runs on _main_stack and cannot be
+ // scheduled out, fall back to waiting.
+ ::usleep(1000);
+ continue;
+ }
+ const auto pop_task = [this](bthread_t* t) -> bool {
+#ifndef BTHREAD_FAIR_WSQ
+ return _rq.pop(t);
+#else
+ return _rq.steal(t);
+#endif
+ };
+ bthread_t run_tid = 0;
+ if (!pop_task(&run_tid)) {
+ // The queue was drained by stealers between the failed push and
+ // the pop, retrying the push will succeed.
+ continue;
+ }
+ if (is_current_main_task() &&
+ address_meta(run_tid)->attr.stack_type == BTHREAD_STACKTYPE_PTHREAD) {
+ // pthread-mode tasks are run inline on _main_stack by the
+ // dispatch loops (run_main_task/task_runner). Switching to one
+ // from the main task itself causes no stack jump and would
+ // silently drop it, so it cannot be run here. But pushing it
+ // straight back makes the LIFO pop() return this same task
+ // forever, hiding any runnable task sitting deeper in the
+ // queue. Hold pthread-mode tasks aside and keep popping until
+ // a runnable one shows up.
+ // Bounded budget: the array must live on the worker's stack
+ // (no allocation on a resource-exhaustion path), and held
+ // tasks are invisible to stealers, so keeping the list short
+ // limits how long they can be delayed. 16 * sizeof(bthread_t)
+ // == 128 bytes == two cache lines.
+ bthread_t pending[16];
+ int npending = 0;
+ pending[npending++] = run_tid;
+ bool found = false;
+ while (npending < 16 && pop_task(&run_tid)) {
+ if (address_meta(run_tid)->attr.stack_type !=
+ BTHREAD_STACKTYPE_PTHREAD) {
+ found = true;
+ break;
+ }
+ pending[npending++] = run_tid;
+ }
+ // Push the held tasks back. The pops above freed at least
+ // |npending| slots, _rq has a single producer (this worker)
+ // and stealers never push, so these pushes must succeed.
+ // Reverse order restores their relative order in the queue.
+ for (int i = npending - 1; i >= 0; --i) {
+ const bool pushed = _rq.push(pending[i]);
+ CHECK(pushed) << "push_rq: failed to put back held task";
+ }
+ if (!found) {
+ // Everything within reach is pthread-mode: wait for
+ // stealers to drain the queue.
+ ::usleep(1000);
Review Comment:
[replied by brpc-oncall robot] After this sched_to() returns, the current task may no longer belong to this TaskGroup: the task was published by the remained callback above and can be stolen by another worker, which resumes it under its own group (that is why sched_to() takes `TaskGroup** pg`). The loop then keeps using `this`:
- `_rq.push(tid)` would push into the old group's local queue from a foreign thread, violating WorkStealingQueue's "never push in parallel with pop()/push()" contract and racing with the old group's owner;
- `is_current_pthread_task()` / `is_current_main_task()` read a stale `_cur_meta` that is concurrently maintained by the old group's owner, and `flush_nosignal_tasks()` signals the wrong group;
- `ready_to_run()`'s tail (`_num_nosignal`, `_control->signal_task(_tag)`) has the same staleness problem, so the retried tid may end up queued in a group that is never signalled.
The `TaskGroup* g = this; sched_to(&g, run_tid);` above already captures the new group but then never uses it. Please re-fetch `BAIDU_GET_VOLATILE_THREAD_LOCAL(tls_task_group)` after every switch and use that group for the retry and for the signalling.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix livelock when all TaskGroup runqueues are full (#1038) (brpc)
Posted by "two-headBoy (via GitHub)" <gi...@apache.org>.
two-headBoy commented on code in PR #3633:
URL: https://github.com/apache/brpc/pull/3633#discussion_r4400254332
##########
src/bthread/task_group_inl.h:
##########
@@ -93,11 +93,100 @@ inline void TaskGroup::push_rq(bthread_t tid) {
// brpc)
flush_nosignal_tasks();
LOG_EVERY_SECOND(ERROR) << "_rq is full, capacity=" << _rq.capacity();
- // TODO(gejun): May cause deadlock when all workers are spinning here.
- // A better solution is to pop and run existing bthreads, however which
- // make set_remained()-callbacks do context switches and need extensive
- // reviews on related code.
- ::usleep(1000);
+ if (is_current_pthread_task() && !is_current_main_task()) {
+ // A pthread-mode bthread runs on _main_stack and cannot be
+ // scheduled out, fall back to waiting.
+ ::usleep(1000);
+ continue;
+ }
+ const auto pop_task = [this](bthread_t* t) -> bool {
+#ifndef BTHREAD_FAIR_WSQ
+ return _rq.pop(t);
+#else
+ return _rq.steal(t);
+#endif
+ };
+ bthread_t run_tid = 0;
+ if (!pop_task(&run_tid)) {
+ // The queue was drained by stealers between the failed push and
+ // the pop, retrying the push will succeed.
+ continue;
+ }
+ if (is_current_main_task() &&
+ address_meta(run_tid)->attr.stack_type == BTHREAD_STACKTYPE_PTHREAD) {
+ // pthread-mode tasks are run inline on _main_stack by the
+ // dispatch loops (run_main_task/task_runner). Switching to one
+ // from the main task itself causes no stack jump and would
+ // silently drop it, so it cannot be run here. But pushing it
+ // straight back makes the LIFO pop() return this same task
+ // forever, hiding any runnable task sitting deeper in the
+ // queue. Hold pthread-mode tasks aside and keep popping until
+ // a runnable one shows up.
+ // Bounded budget: the array must live on the worker's stack
+ // (no allocation on a resource-exhaustion path), and held
+ // tasks are invisible to stealers, so keeping the list short
+ // limits how long they can be delayed. 16 * sizeof(bthread_t)
+ // == 128 bytes == two cache lines.
+ bthread_t pending[16];
+ int npending = 0;
+ pending[npending++] = run_tid;
+ bool found = false;
+ while (npending < 16 && pop_task(&run_tid)) {
+ if (address_meta(run_tid)->attr.stack_type !=
+ BTHREAD_STACKTYPE_PTHREAD) {
+ found = true;
+ break;
+ }
+ pending[npending++] = run_tid;
+ }
+ // Push the held tasks back. The pops above freed at least
+ // |npending| slots, _rq has a single producer (this worker)
+ // and stealers never push, so these pushes must succeed.
+ // Reverse order restores their relative order in the queue.
+ for (int i = npending - 1; i >= 0; --i) {
+ const bool pushed = _rq.push(pending[i]);
+ CHECK(pushed) << "push_rq: failed to put back held task";
Review Comment:
The suggestion replaces`_rq.push()` +`CHECK` with a retry loop, so the failure branch no longer aborts — that part is fine. But the loop is not actually bounded:`nretry` is reset to 0 every 3 attempts, so if`push()` keeps failing, this worker spins forever while the remaining`pending[]` entries stay unqueued and invisible to stealers — the same livelock class this PR removes.
In the current implementation`push()` can only fail if the single-producer invariant is already broken (the pops freed the slots, and stealers never push), i.e. through the cross-thread push path. If we want a non-fatal fallback, it should terminate: after N attempts, hand the task to the mutex-protected`_remote_rq` instead of retrying forever. Otherwise`CHECK` is the right signal here, and it becomes unreachable once the stale-`this` issue is fixed.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix livelock when all TaskGroup runqueues are full (#1038) (brpc)
Posted by "wwbmmm (via GitHub) via dev" <de...@brpc.apache.org>.
wwbmmm commented on PR #3633:
URL: https://github.com/apache/brpc/pull/3633#issuecomment-6554158160
I think we should be very careful to merge this PR. It change the core behavior of bthread. This issue exists for a long time but no one succeed to fix it before.
When you encounter this "_rq is full" issue, you should first try to increase your worker count or queue size.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix livelock when all TaskGroup runqueues are full (#1038) (brpc)
Posted by "two-headBoy (via GitHub)" <gi...@apache.org>.
two-headBoy commented on code in PR #3633:
URL: https://github.com/apache/brpc/pull/3633#discussion_r4347146494
##########
src/bthread/task_group_inl.h:
##########
@@ -93,11 +93,65 @@ inline void TaskGroup::push_rq(bthread_t tid) {
// brpc)
flush_nosignal_tasks();
LOG_EVERY_SECOND(ERROR) << "_rq is full, capacity=" << _rq.capacity();
- // TODO(gejun): May cause deadlock when all workers are spinning here.
- // A better solution is to pop and run existing bthreads, however which
- // make set_remained()-callbacks do context switches and need extensive
- // reviews on related code.
- ::usleep(1000);
+ if (is_current_pthread_task() && !is_current_main_task()) {
+ // A pthread-mode bthread runs on _main_stack and cannot be
+ // scheduled out, fall back to waiting.
+ ::usleep(1000);
+ continue;
+ }
+ bthread_t run_tid = 0;
+#ifndef BTHREAD_FAIR_WSQ
+ const bool popped = _rq.pop(&run_tid);
+#else
+ const bool popped = _rq.steal(&run_tid);
+#endif
+ if (!popped) {
+ // The queue was drained by stealers between the failed push and
+ // the pop, retrying the push will succeed.
+ continue;
+ }
+ if (is_current_main_task() &&
+ address_meta(run_tid)->attr.stack_type == BTHREAD_STACKTYPE_PTHREAD) {
+ // pthread-mode tasks are run inline on _main_stack by the
+ // dispatch loops (run_main_task/task_runner). Switching to one
+ // from the main task itself causes no stack jump and would
+ // silently drop it, put it back and wait; stealers may run it
+ // on other workers.
+ _rq.push(run_tid);
+ ::usleep(1000);
+ continue;
+ }
Review Comment:
Fixed, though with a different approach than enqueueing`tid` first: the pop only frees one slot, so enqueueing`tid` immediately would leave`run_tid` with nowhere to go.
The deeper root cause is that the non-FAIR WSQ is LIFO on the owner side: pushing`run_tid` straight back makes the next`pop()` return this very same task, so a runnable task sitting deeper in the queue is never reached. The new code holds popped pthread-mode tasks in a small local array and keeps popping until a runnable one shows up, then runs it inline to free a slot (your second suggestion). If everything within reach (16 entries) is pthread-mode, the held tasks are pushed back in reverse order and we fall back to usleep waiting for stealers — that all-pthread window is a known limitation of pthread-mode, as noted in the PR description.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix livelock when all TaskGroup runqueues are full (#1038) (brpc)
Posted by "chenBright (via GitHub) via dev" <de...@brpc.apache.org>.
chenBright commented on PR #3633:
URL: https://github.com/apache/brpc/pull/3633#issuecomment-6559464923
> I think we should be very careful to merge this PR. It change the core behavior of bthread. This issue exists for a long time but no one succeed to fix it before. When you encounter this "_rq is full" issue, you should first try to increase your worker count or queue size.
Agree. Making push_rq() a potential context-switching point changes a fundamental assumption across its callers, including bthread_start_background() and internal wakeup paths.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix livelock when all TaskGroup runqueues are full (#1038) (brpc)
Posted by "two-headBoy (via GitHub) via dev" <de...@brpc.apache.org>.
two-headBoy commented on PR #3633:
URL: https://github.com/apache/brpc/pull/3633#issuecomment-6627796610
Thanks for the reviews. I agree the root problem is the design: making `push_rq()` an implicit scheduling point breaks existing assumptions of `bthread_start_background()` and its callers. The follow-up fixes only address individual consequences of this change.
I’ll abandon this implementation and close this PR.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix livelock when all TaskGroup runqueues are full (#1038) (brpc)
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3633:
URL: https://github.com/apache/brpc/pull/3633#discussion_r4326651238
##########
src/bthread/task_group_inl.h:
##########
@@ -93,11 +93,55 @@ inline void TaskGroup::push_rq(bthread_t tid) {
// brpc)
flush_nosignal_tasks();
LOG_EVERY_SECOND(ERROR) << "_rq is full, capacity=" << _rq.capacity();
- // TODO(gejun): May cause deadlock when all workers are spinning here.
- // A better solution is to pop and run existing bthreads, however which
- // make set_remained()-callbacks do context switches and need extensive
- // reviews on related code.
- ::usleep(1000);
+ if (is_current_pthread_task() && !is_current_main_task()) {
+ // A pthread-mode bthread runs on _main_stack and cannot be
+ // scheduled out, fall back to waiting.
+ ::usleep(1000);
+ continue;
+ }
+ bthread_t run_tid = 0;
+#ifndef BTHREAD_FAIR_WSQ
+ const bool popped = _rq.pop(&run_tid);
+#else
+ const bool popped = _rq.steal(&run_tid);
+#endif
+ if (!popped) {
+ // The queue was drained by stealers between the failed push and
+ // the pop, retrying the push will succeed.
+ continue;
+ }
+ if (is_current_main_task() &&
+ address_meta(run_tid)->attr.stack_type == BTHREAD_STACKTYPE_PTHREAD) {
+ // pthread-mode tasks are run inline on _main_stack by the
+ // dispatch loops (run_main_task/task_runner). Switching to one
+ // from the main task itself causes no stack jump and would
+ // silently drop it, put it back and wait; stealers may run it
+ // on other workers.
+ _rq.push(run_tid);
Review Comment:
`_rq.push(...)` is called without checking whether it succeeded. In this function we’re explicitly in the “queue is/was full” slow-path; if `_rq.push` returns a status (as is typical for bounded runqueues and implied by the surrounding retry-loop), ignoring the result can silently drop `run_tid` / `current_tid()` on failure. Please handle the return value (retry or assert/LOG+fallback) to guarantee no task is lost on this path.
##########
src/bthread/task_group_inl.h:
##########
@@ -93,11 +93,55 @@ inline void TaskGroup::push_rq(bthread_t tid) {
// brpc)
flush_nosignal_tasks();
LOG_EVERY_SECOND(ERROR) << "_rq is full, capacity=" << _rq.capacity();
- // TODO(gejun): May cause deadlock when all workers are spinning here.
- // A better solution is to pop and run existing bthreads, however which
- // make set_remained()-callbacks do context switches and need extensive
- // reviews on related code.
- ::usleep(1000);
+ if (is_current_pthread_task() && !is_current_main_task()) {
+ // A pthread-mode bthread runs on _main_stack and cannot be
+ // scheduled out, fall back to waiting.
+ ::usleep(1000);
+ continue;
+ }
+ bthread_t run_tid = 0;
+#ifndef BTHREAD_FAIR_WSQ
+ const bool popped = _rq.pop(&run_tid);
+#else
+ const bool popped = _rq.steal(&run_tid);
+#endif
+ if (!popped) {
+ // The queue was drained by stealers between the failed push and
+ // the pop, retrying the push will succeed.
+ continue;
+ }
+ if (is_current_main_task() &&
+ address_meta(run_tid)->attr.stack_type == BTHREAD_STACKTYPE_PTHREAD) {
+ // pthread-mode tasks are run inline on _main_stack by the
+ // dispatch loops (run_main_task/task_runner). Switching to one
+ // from the main task itself causes no stack jump and would
+ // silently drop it, put it back and wait; stealers may run it
+ // on other workers.
+ _rq.push(run_tid);
+ ::usleep(1000);
+ continue;
+ }
+ // Run the popped bthread in place instead of spinning, otherwise
+ // all workers may spin here and no one drains the queues any more.
+ // The pop also frees one slot, which guarantees that the push of
+ // the current task below succeeds: _rq has a single producer (the
+ // owning worker) and stealers never push.
+ if (!is_current_main_task()) {
+ // Preserve the task being scheduled out. Note that it is
+ // current_tid() rather than |tid|: push_rq() can be re-entered
+ // from remained-callbacks which run on the stack of the next
+ // task, where the task to preserve is the one whose context
+ // sched_to() is about to freeze.
+ _rq.push(current_tid());
Review Comment:
`_rq.push(...)` is called without checking whether it succeeded. In this function we’re explicitly in the “queue is/was full” slow-path; if `_rq.push` returns a status (as is typical for bounded runqueues and implied by the surrounding retry-loop), ignoring the result can silently drop `run_tid` / `current_tid()` on failure. Please handle the return value (retry or assert/LOG+fallback) to guarantee no task is lost on this path.
##########
src/bthread/task_group_inl.h:
##########
@@ -93,11 +93,55 @@ inline void TaskGroup::push_rq(bthread_t tid) {
// brpc)
flush_nosignal_tasks();
LOG_EVERY_SECOND(ERROR) << "_rq is full, capacity=" << _rq.capacity();
- // TODO(gejun): May cause deadlock when all workers are spinning here.
- // A better solution is to pop and run existing bthreads, however which
- // make set_remained()-callbacks do context switches and need extensive
- // reviews on related code.
- ::usleep(1000);
+ if (is_current_pthread_task() && !is_current_main_task()) {
+ // A pthread-mode bthread runs on _main_stack and cannot be
+ // scheduled out, fall back to waiting.
+ ::usleep(1000);
+ continue;
+ }
+ bthread_t run_tid = 0;
+#ifndef BTHREAD_FAIR_WSQ
+ const bool popped = _rq.pop(&run_tid);
+#else
+ const bool popped = _rq.steal(&run_tid);
+#endif
+ if (!popped) {
+ // The queue was drained by stealers between the failed push and
+ // the pop, retrying the push will succeed.
+ continue;
+ }
+ if (is_current_main_task() &&
+ address_meta(run_tid)->attr.stack_type == BTHREAD_STACKTYPE_PTHREAD) {
+ // pthread-mode tasks are run inline on _main_stack by the
+ // dispatch loops (run_main_task/task_runner). Switching to one
+ // from the main task itself causes no stack jump and would
+ // silently drop it, put it back and wait; stealers may run it
+ // on other workers.
+ _rq.push(run_tid);
+ ::usleep(1000);
+ continue;
+ }
+ // Run the popped bthread in place instead of spinning, otherwise
+ // all workers may spin here and no one drains the queues any more.
+ // The pop also frees one slot, which guarantees that the push of
+ // the current task below succeeds: _rq has a single producer (the
+ // owning worker) and stealers never push.
+ if (!is_current_main_task()) {
+ // Preserve the task being scheduled out. Note that it is
+ // current_tid() rather than |tid|: push_rq() can be re-entered
+ // from remained-callbacks which run on the stack of the next
+ // task, where the task to preserve is the one whose context
+ // sched_to() is about to freeze.
+ _rq.push(current_tid());
+ }
Review Comment:
This enqueues `current_tid()` onto `_rq` before the current task has actually been scheduled out (the actual switch happens at `sched_to`). If stealers can concurrently take items from `_rq`, this can expose a still-running task as runnable, which risks concurrent execution or other invariant violations unless there is an explicit state transition/guard that prevents thieves from running it until after the switch. Consider using the existing “enqueue-current-and-switch” primitive (if one exists) that atomically marks the task non-running and publishes it, or introduce a guard (e.g., disable stealing for the brief window, or only publish after the scheduler has committed the switch) that preserves the runqueue/task-state invariants.
##########
src/bthread/task_group_inl.h:
##########
@@ -93,11 +93,55 @@ inline void TaskGroup::push_rq(bthread_t tid) {
// brpc)
flush_nosignal_tasks();
LOG_EVERY_SECOND(ERROR) << "_rq is full, capacity=" << _rq.capacity();
- // TODO(gejun): May cause deadlock when all workers are spinning here.
- // A better solution is to pop and run existing bthreads, however which
- // make set_remained()-callbacks do context switches and need extensive
- // reviews on related code.
- ::usleep(1000);
+ if (is_current_pthread_task() && !is_current_main_task()) {
+ // A pthread-mode bthread runs on _main_stack and cannot be
+ // scheduled out, fall back to waiting.
+ ::usleep(1000);
+ continue;
+ }
+ bthread_t run_tid = 0;
+#ifndef BTHREAD_FAIR_WSQ
+ const bool popped = _rq.pop(&run_tid);
+#else
+ const bool popped = _rq.steal(&run_tid);
+#endif
+ if (!popped) {
+ // The queue was drained by stealers between the failed push and
+ // the pop, retrying the push will succeed.
+ continue;
+ }
+ if (is_current_main_task() &&
+ address_meta(run_tid)->attr.stack_type == BTHREAD_STACKTYPE_PTHREAD) {
+ // pthread-mode tasks are run inline on _main_stack by the
+ // dispatch loops (run_main_task/task_runner). Switching to one
+ // from the main task itself causes no stack jump and would
+ // silently drop it, put it back and wait; stealers may run it
+ // on other workers.
+ _rq.push(run_tid);
+ ::usleep(1000);
+ continue;
+ }
+ // Run the popped bthread in place instead of spinning, otherwise
+ // all workers may spin here and no one drains the queues any more.
+ // The pop also frees one slot, which guarantees that the push of
+ // the current task below succeeds: _rq has a single producer (the
+ // owning worker) and stealers never push.
+ if (!is_current_main_task()) {
+ // Preserve the task being scheduled out. Note that it is
+ // current_tid() rather than |tid|: push_rq() can be re-entered
+ // from remained-callbacks which run on the stack of the next
+ // task, where the task to preserve is the one whose context
+ // sched_to() is about to freeze.
+ _rq.push(current_tid());
+ }
+ // The main task is never queued; ending_sched() resumes it via
+ // _main_tid when the queue drains. Jumping to a pthread-mode task
+ // from a normal bthread resumes the dispatch loop on _main_stack
+ // which runs it inline, same as ending_sched() does.
+ TaskGroup* g = this;
+ sched_to(&g, run_tid);
Review Comment:
This enqueues `current_tid()` onto `_rq` before the current task has actually been scheduled out (the actual switch happens at `sched_to`). If stealers can concurrently take items from `_rq`, this can expose a still-running task as runnable, which risks concurrent execution or other invariant violations unless there is an explicit state transition/guard that prevents thieves from running it until after the switch. Consider using the existing “enqueue-current-and-switch” primitive (if one exists) that atomically marks the task non-running and publishes it, or introduce a guard (e.g., disable stealing for the brief window, or only publish after the scheduler has committed the switch) that preserves the runqueue/task-state invariants.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix livelock when all TaskGroup runqueues are full (#1038) (brpc)
Posted by "two-headBoy (via GitHub)" <gi...@apache.org>.
two-headBoy commented on code in PR #3633:
URL: https://github.com/apache/brpc/pull/3633#discussion_r4399924127
##########
src/bthread/task_group_inl.h:
##########
@@ -93,11 +93,100 @@ inline void TaskGroup::push_rq(bthread_t tid) {
// brpc)
flush_nosignal_tasks();
LOG_EVERY_SECOND(ERROR) << "_rq is full, capacity=" << _rq.capacity();
- // TODO(gejun): May cause deadlock when all workers are spinning here.
- // A better solution is to pop and run existing bthreads, however which
- // make set_remained()-callbacks do context switches and need extensive
- // reviews on related code.
- ::usleep(1000);
+ if (is_current_pthread_task() && !is_current_main_task()) {
+ // A pthread-mode bthread runs on _main_stack and cannot be
+ // scheduled out, fall back to waiting.
+ ::usleep(1000);
+ continue;
+ }
+ const auto pop_task = [this](bthread_t* t) -> bool {
+#ifndef BTHREAD_FAIR_WSQ
+ return _rq.pop(t);
+#else
Review Comment:
Under BTHREAD_FAIR_WSQ, pop_task() uses _rq.steal() rather than an owner-side pop. If steal() has different semantics (end of deque, fairness, or concurrency assumptions) than the owner-consumption path, this may change scheduling behavior in the overload path. Prefer using the owner pop API if it exists in the fair-WSQ implementation; otherwise, add an explicit comment explaining why steal() is the correct/safe choice for the owning worker here.
##########
src/bthread/task_group_inl.h:
##########
@@ -93,11 +93,100 @@ inline void TaskGroup::push_rq(bthread_t tid) {
// brpc)
flush_nosignal_tasks();
LOG_EVERY_SECOND(ERROR) << "_rq is full, capacity=" << _rq.capacity();
- // TODO(gejun): May cause deadlock when all workers are spinning here.
- // A better solution is to pop and run existing bthreads, however which
- // make set_remained()-callbacks do context switches and need extensive
- // reviews on related code.
- ::usleep(1000);
+ if (is_current_pthread_task() && !is_current_main_task()) {
+ // A pthread-mode bthread runs on _main_stack and cannot be
+ // scheduled out, fall back to waiting.
+ ::usleep(1000);
+ continue;
+ }
+ const auto pop_task = [this](bthread_t* t) -> bool {
+#ifndef BTHREAD_FAIR_WSQ
+ return _rq.pop(t);
+#else
Review Comment:
Under BTHREAD_FAIR_WSQ, pop_task() uses _rq.steal() rather than an owner-side pop. If steal() has different semantics (end of deque, fairness, or concurrency assumptions) than the owner-consumption path, this may change scheduling behavior in the overload path. Prefer using the owner pop API if it exists in the fair-WSQ implementation; otherwise, add an explicit comment explaining why steal() is the correct/safe choice for the owning worker here.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix livelock when all TaskGroup runqueues are full (#1038) (brpc)
Posted by "two-headBoy (via GitHub)" <gi...@apache.org>.
two-headBoy commented on code in PR #3633:
URL: https://github.com/apache/brpc/pull/3633#discussion_r4400380758
##########
src/bthread/task_group_inl.h:
##########
@@ -93,11 +93,100 @@ inline void TaskGroup::push_rq(bthread_t tid) {
// brpc)
flush_nosignal_tasks();
LOG_EVERY_SECOND(ERROR) << "_rq is full, capacity=" << _rq.capacity();
- // TODO(gejun): May cause deadlock when all workers are spinning here.
- // A better solution is to pop and run existing bthreads, however which
- // make set_remained()-callbacks do context switches and need extensive
- // reviews on related code.
- ::usleep(1000);
+ if (is_current_pthread_task() && !is_current_main_task()) {
+ // A pthread-mode bthread runs on _main_stack and cannot be
+ // scheduled out, fall back to waiting.
+ ::usleep(1000);
+ continue;
+ }
+ const auto pop_task = [this](bthread_t* t) -> bool {
+#ifndef BTHREAD_FAIR_WSQ
+ return _rq.pop(t);
+#else
+ return _rq.steal(t);
+#endif
+ };
+ bthread_t run_tid = 0;
+ if (!pop_task(&run_tid)) {
+ // The queue was drained by stealers between the failed push and
+ // the pop, retrying the push will succeed.
+ continue;
+ }
+ if (is_current_main_task() &&
+ address_meta(run_tid)->attr.stack_type == BTHREAD_STACKTYPE_PTHREAD) {
+ // pthread-mode tasks are run inline on _main_stack by the
+ // dispatch loops (run_main_task/task_runner). Switching to one
+ // from the main task itself causes no stack jump and would
+ // silently drop it, so it cannot be run here. But pushing it
+ // straight back makes the LIFO pop() return this same task
+ // forever, hiding any runnable task sitting deeper in the
+ // queue. Hold pthread-mode tasks aside and keep popping until
+ // a runnable one shows up.
+ // Bounded budget: the array must live on the worker's stack
+ // (no allocation on a resource-exhaustion path), and held
+ // tasks are invisible to stealers, so keeping the list short
+ // limits how long they can be delayed. 16 * sizeof(bthread_t)
+ // == 128 bytes == two cache lines.
+ bthread_t pending[16];
+ int npending = 0;
+ pending[npending++] = run_tid;
+ bool found = false;
+ while (npending < 16 && pop_task(&run_tid)) {
+ if (address_meta(run_tid)->attr.stack_type !=
+ BTHREAD_STACKTYPE_PTHREAD) {
+ found = true;
+ break;
+ }
+ pending[npending++] = run_tid;
+ }
+ // Push the held tasks back. The pops above freed at least
+ // |npending| slots, _rq has a single producer (this worker)
+ // and stealers never push, so these pushes must succeed.
+ // Reverse order restores their relative order in the queue.
+ for (int i = npending - 1; i >= 0; --i) {
+ const bool pushed = _rq.push(pending[i]);
+ CHECK(pushed) << "push_rq: failed to put back held task";
+ }
+ if (!found) {
+ // Everything within reach is pthread-mode: wait for
+ // stealers to drain the queue.
+ ::usleep(1000);
+ continue;
+ }
Review Comment:
Duplicate of the comment above; marking as won't fix for the same reason.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix livelock when all TaskGroup runqueues are full (#1038) (brpc)
Posted by "two-headBoy (via GitHub)" <gi...@apache.org>.
two-headBoy commented on code in PR #3633:
URL: https://github.com/apache/brpc/pull/3633#discussion_r4327702550
##########
src/bthread/task_group_inl.h:
##########
@@ -93,11 +93,55 @@ inline void TaskGroup::push_rq(bthread_t tid) {
// brpc)
flush_nosignal_tasks();
LOG_EVERY_SECOND(ERROR) << "_rq is full, capacity=" << _rq.capacity();
- // TODO(gejun): May cause deadlock when all workers are spinning here.
- // A better solution is to pop and run existing bthreads, however which
- // make set_remained()-callbacks do context switches and need extensive
- // reviews on related code.
- ::usleep(1000);
+ if (is_current_pthread_task() && !is_current_main_task()) {
+ // A pthread-mode bthread runs on _main_stack and cannot be
+ // scheduled out, fall back to waiting.
+ ::usleep(1000);
+ continue;
+ }
+ bthread_t run_tid = 0;
+#ifndef BTHREAD_FAIR_WSQ
+ const bool popped = _rq.pop(&run_tid);
+#else
+ const bool popped = _rq.steal(&run_tid);
+#endif
+ if (!popped) {
+ // The queue was drained by stealers between the failed push and
+ // the pop, retrying the push will succeed.
+ continue;
+ }
+ if (is_current_main_task() &&
+ address_meta(run_tid)->attr.stack_type == BTHREAD_STACKTYPE_PTHREAD) {
+ // pthread-mode tasks are run inline on _main_stack by the
+ // dispatch loops (run_main_task/task_runner). Switching to one
+ // from the main task itself causes no stack jump and would
+ // silently drop it, put it back and wait; stealers may run it
+ // on other workers.
+ _rq.push(run_tid);
+ ::usleep(1000);
+ continue;
+ }
+ // Run the popped bthread in place instead of spinning, otherwise
+ // all workers may spin here and no one drains the queues any more.
+ // The pop also frees one slot, which guarantees that the push of
+ // the current task below succeeds: _rq has a single producer (the
+ // owning worker) and stealers never push.
+ if (!is_current_main_task()) {
+ // Preserve the task being scheduled out. Note that it is
+ // current_tid() rather than |tid|: push_rq() can be re-entered
+ // from remained-callbacks which run on the stack of the next
+ // task, where the task to preserve is the one whose context
+ // sched_to() is about to freeze.
+ _rq.push(current_tid());
Review Comment:
This is safe by the single-producer contract of `WorkStealingQueue`:
1. `push()` is documented as "May run in parallel with `steal()`. Never run
in parallel with `pop()` or another `push()`" (work_stealing_queue.h).
`_rq` is only ever pushed by its owning worker inside `push_rq()` — all
cross-thread submissions go through `_remote_rq` instead (see
`ready_to_run_general()`). Stealers only consume via `steal()`.
2. Both `_rq.push(...)` calls here immediately follow a successful
`_rq.pop()` in this same function, which frees exactly one slot. Since
no other thread can push into `_rq`, the freed slot cannot be refilled
between the pop and the push — stealers can only make the queue even
less full. So the push is guaranteed to succeed and no task can be lost.
This invariant is documented in the comment right above the push ("The pop
also frees one slot, which guarantees that the push of the current task
below succeeds: _rq has a single producer ... and stealers never push").
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix livelock when all TaskGroup runqueues are full (#1038) (brpc)
Posted by "two-headBoy (via GitHub)" <gi...@apache.org>.
two-headBoy commented on code in PR #3633:
URL: https://github.com/apache/brpc/pull/3633#discussion_r4327696532
##########
src/bthread/task_group_inl.h:
##########
@@ -93,11 +93,55 @@ inline void TaskGroup::push_rq(bthread_t tid) {
// brpc)
flush_nosignal_tasks();
LOG_EVERY_SECOND(ERROR) << "_rq is full, capacity=" << _rq.capacity();
- // TODO(gejun): May cause deadlock when all workers are spinning here.
- // A better solution is to pop and run existing bthreads, however which
- // make set_remained()-callbacks do context switches and need extensive
- // reviews on related code.
- ::usleep(1000);
+ if (is_current_pthread_task() && !is_current_main_task()) {
+ // A pthread-mode bthread runs on _main_stack and cannot be
+ // scheduled out, fall back to waiting.
+ ::usleep(1000);
+ continue;
+ }
+ bthread_t run_tid = 0;
+#ifndef BTHREAD_FAIR_WSQ
+ const bool popped = _rq.pop(&run_tid);
+#else
+ const bool popped = _rq.steal(&run_tid);
+#endif
+ if (!popped) {
+ // The queue was drained by stealers between the failed push and
+ // the pop, retrying the push will succeed.
+ continue;
+ }
+ if (is_current_main_task() &&
+ address_meta(run_tid)->attr.stack_type == BTHREAD_STACKTYPE_PTHREAD) {
+ // pthread-mode tasks are run inline on _main_stack by the
+ // dispatch loops (run_main_task/task_runner). Switching to one
+ // from the main task itself causes no stack jump and would
+ // silently drop it, put it back and wait; stealers may run it
+ // on other workers.
+ _rq.push(run_tid);
Review Comment:
This is safe by the single-producer contract of `WorkStealingQueue`:
1. `push()` is documented as "May run in parallel with `steal()`. Never run
in parallel with `pop()` or another `push()`" (work_stealing_queue.h).
`_rq` is only ever pushed by its owning worker inside `push_rq()` — all
cross-thread submissions go through `_remote_rq` instead (see
`ready_to_run_general()`). Stealers only consume via `steal()`.
2. Both `_rq.push(...)` calls here immediately follow a successful
`_rq.pop()` in this same function, which frees exactly one slot. Since
no other thread can push into `_rq`, the freed slot cannot be refilled
between the pop and the push — stealers can only make the queue even
less full. So the push is guaranteed to succeed and no task can be lost.
This invariant is documented in the comment right above the push ("The pop
also frees one slot, which guarantees that the push of the current task
below succeeds: _rq has a single producer ... and stealers never push").
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix livelock when all TaskGroup runqueues are full (#1038) (brpc)
Posted by "two-headBoy (via GitHub)" <gi...@apache.org>.
two-headBoy commented on code in PR #3633:
URL: https://github.com/apache/brpc/pull/3633#discussion_r4347149544
##########
src/bthread/task_group_inl.h:
##########
@@ -93,11 +93,65 @@ inline void TaskGroup::push_rq(bthread_t tid) {
// brpc)
flush_nosignal_tasks();
LOG_EVERY_SECOND(ERROR) << "_rq is full, capacity=" << _rq.capacity();
- // TODO(gejun): May cause deadlock when all workers are spinning here.
- // A better solution is to pop and run existing bthreads, however which
- // make set_remained()-callbacks do context switches and need extensive
- // reviews on related code.
- ::usleep(1000);
+ if (is_current_pthread_task() && !is_current_main_task()) {
+ // A pthread-mode bthread runs on _main_stack and cannot be
+ // scheduled out, fall back to waiting.
+ ::usleep(1000);
+ continue;
+ }
+ bthread_t run_tid = 0;
+#ifndef BTHREAD_FAIR_WSQ
+ const bool popped = _rq.pop(&run_tid);
+#else
+ const bool popped = _rq.steal(&run_tid);
+#endif
+ if (!popped) {
+ // The queue was drained by stealers between the failed push and
+ // the pop, retrying the push will succeed.
+ continue;
+ }
+ if (is_current_main_task() &&
+ address_meta(run_tid)->attr.stack_type == BTHREAD_STACKTYPE_PTHREAD) {
+ // pthread-mode tasks are run inline on _main_stack by the
+ // dispatch loops (run_main_task/task_runner). Switching to one
+ // from the main task itself causes no stack jump and would
+ // silently drop it, put it back and wait; stealers may run it
+ // on other workers.
+ _rq.push(run_tid);
Review Comment:
Fixed. The re-queue pushes now`CHECK` the return value. The push is guaranteed to succeed here — the pops above freed at least that many slots,`_rq` has a single producer (this worker) and stealers never push — but the`CHECK` turns this invariant into a runtime assertion, so any future change breaking it (e.g. WSQ semantics) fails loudly instead of silently dropping a task.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix livelock when all TaskGroup runqueues are full (#1038) (brpc)
Posted by "wwbmmm (via GitHub)" <gi...@apache.org>.
wwbmmm commented on code in PR #3633:
URL: https://github.com/apache/brpc/pull/3633#discussion_r4370309723
##########
src/bthread/task_group_inl.h:
##########
@@ -93,11 +93,100 @@ inline void TaskGroup::push_rq(bthread_t tid) {
// brpc)
flush_nosignal_tasks();
LOG_EVERY_SECOND(ERROR) << "_rq is full, capacity=" << _rq.capacity();
- // TODO(gejun): May cause deadlock when all workers are spinning here.
- // A better solution is to pop and run existing bthreads, however which
- // make set_remained()-callbacks do context switches and need extensive
- // reviews on related code.
- ::usleep(1000);
+ if (is_current_pthread_task() && !is_current_main_task()) {
+ // A pthread-mode bthread runs on _main_stack and cannot be
+ // scheduled out, fall back to waiting.
+ ::usleep(1000);
+ continue;
+ }
+ const auto pop_task = [this](bthread_t* t) -> bool {
+#ifndef BTHREAD_FAIR_WSQ
+ return _rq.pop(t);
+#else
+ return _rq.steal(t);
+#endif
+ };
+ bthread_t run_tid = 0;
+ if (!pop_task(&run_tid)) {
+ // The queue was drained by stealers between the failed push and
+ // the pop, retrying the push will succeed.
+ continue;
+ }
+ if (is_current_main_task() &&
+ address_meta(run_tid)->attr.stack_type == BTHREAD_STACKTYPE_PTHREAD) {
+ // pthread-mode tasks are run inline on _main_stack by the
+ // dispatch loops (run_main_task/task_runner). Switching to one
+ // from the main task itself causes no stack jump and would
+ // silently drop it, so it cannot be run here. But pushing it
+ // straight back makes the LIFO pop() return this same task
+ // forever, hiding any runnable task sitting deeper in the
+ // queue. Hold pthread-mode tasks aside and keep popping until
+ // a runnable one shows up.
+ // Bounded budget: the array must live on the worker's stack
+ // (no allocation on a resource-exhaustion path), and held
+ // tasks are invisible to stealers, so keeping the list short
+ // limits how long they can be delayed. 16 * sizeof(bthread_t)
+ // == 128 bytes == two cache lines.
+ bthread_t pending[16];
+ int npending = 0;
+ pending[npending++] = run_tid;
+ bool found = false;
+ while (npending < 16 && pop_task(&run_tid)) {
+ if (address_meta(run_tid)->attr.stack_type !=
+ BTHREAD_STACKTYPE_PTHREAD) {
+ found = true;
+ break;
+ }
+ pending[npending++] = run_tid;
+ }
+ // Push the held tasks back. The pops above freed at least
+ // |npending| slots, _rq has a single producer (this worker)
+ // and stealers never push, so these pushes must succeed.
+ // Reverse order restores their relative order in the queue.
+ for (int i = npending - 1; i >= 0; --i) {
+ const bool pushed = _rq.push(pending[i]);
+ CHECK(pushed) << "push_rq: failed to put back held task";
+ }
+ if (!found) {
+ // Everything within reach is pthread-mode: wait for
+ // stealers to drain the queue.
+ ::usleep(1000);
+ continue;
+ }
+ // Fall through: run_tid is a runnable task, run it inline
+ // below to free a slot for |tid|.
+ }
+ // Run the popped bthread in place instead of spinning, otherwise
+ // all workers may spin here and no one drains the queues any more.
+ // The pop also frees one slot, which guarantees that the deferred
+ // push of the current task succeeds: _rq has a single producer (the
+ // owning worker) and stealers never push.
+ // |args| lives on the stack that sched_to() is about to freeze and is
+ // read by the remained-callback after the switch, so it must stay in
+ // this scope: if it were declared inside the branch below, its slot
+ // could be reused before sched_to() reads it.
+ ReadyToRunArgs args = { tag(), nullptr, false };
+ if (!is_current_main_task()) {
+ // Preserve the task being scheduled out. The push is deferred to
+ // a remained-callback: publishing a still-running task into _rq
+ // lets a stealer jump into it and run the same context
+ // concurrently on another worker. The callback runs on the stack
+ // of |run_tid| after sched_to() has frozen the current context,
+ // same as exchange(). Capture the meta now, current_tid() is
+ // |run_tid| by the time the callback runs. Note that push_rq()
+ // can be re-entered from remained-callbacks, in which case the
+ // task to preserve is still the one being frozen here.
+ args.meta = _cur_meta;
+ set_remained(ready_to_run_in_worker_ignoresignal, &args);
+ }
+ // The main task is never queued; ending_sched() resumes it via
+ // _main_tid when the queue drains. Jumping to a pthread-mode task
+ // from a normal bthread resumes the dispatch loop on _main_stack
+ // which runs it inline, same as ending_sched() does.
+ TaskGroup* g = this;
+ sched_to(&g, run_tid);
+ // Resume here after the popped bthread (and possibly others) ran,
Review Comment:
[replied by brpc-oncall robot] Still unaddressed: after this sched_to() returns, the current task may no longer belong to this TaskGroup. The task was published by the remained callback above, so another worker can steal it from `_rq` (TaskControl::steal_task) and resume it under its own group, which is exactly why sched_to() takes `TaskGroup** pg`. The loop then keeps using `this`:
- `_rq.push(tid)` pushes into the old group's local queue from a foreign thread, violating WorkStealingQueue's "never push in parallel with pop()/push()" contract and racing with the old group's owner;
- `is_current_pthread_task()` / `is_current_main_task()` read a stale `_cur_meta` that the old group's owner may be updating concurrently, and `flush_nosignal_tasks()` signals the wrong group;
- `ready_to_run()`'s tail (`_num_nosignal`, `_control->signal_task(_tag)`) has the same staleness problem.
`TaskGroup* g = this; sched_to(&g, run_tid);` already captures the new group but it is never used. Please re-fetch `BAIDU_GET_VOLATILE_THREAD_LOCAL(tls_task_group)` after every switch and use that group for the retry, the pthread/main-task checks and the signalling.
##########
src/bthread/task_group_inl.h:
##########
@@ -93,11 +93,100 @@ inline void TaskGroup::push_rq(bthread_t tid) {
// brpc)
flush_nosignal_tasks();
LOG_EVERY_SECOND(ERROR) << "_rq is full, capacity=" << _rq.capacity();
- // TODO(gejun): May cause deadlock when all workers are spinning here.
- // A better solution is to pop and run existing bthreads, however which
- // make set_remained()-callbacks do context switches and need extensive
- // reviews on related code.
- ::usleep(1000);
+ if (is_current_pthread_task() && !is_current_main_task()) {
+ // A pthread-mode bthread runs on _main_stack and cannot be
+ // scheduled out, fall back to waiting.
+ ::usleep(1000);
+ continue;
+ }
+ const auto pop_task = [this](bthread_t* t) -> bool {
+#ifndef BTHREAD_FAIR_WSQ
+ return _rq.pop(t);
+#else
+ return _rq.steal(t);
+#endif
+ };
+ bthread_t run_tid = 0;
+ if (!pop_task(&run_tid)) {
+ // The queue was drained by stealers between the failed push and
+ // the pop, retrying the push will succeed.
+ continue;
+ }
+ if (is_current_main_task() &&
+ address_meta(run_tid)->attr.stack_type == BTHREAD_STACKTYPE_PTHREAD) {
+ // pthread-mode tasks are run inline on _main_stack by the
+ // dispatch loops (run_main_task/task_runner). Switching to one
+ // from the main task itself causes no stack jump and would
+ // silently drop it, so it cannot be run here. But pushing it
+ // straight back makes the LIFO pop() return this same task
+ // forever, hiding any runnable task sitting deeper in the
+ // queue. Hold pthread-mode tasks aside and keep popping until
+ // a runnable one shows up.
+ // Bounded budget: the array must live on the worker's stack
+ // (no allocation on a resource-exhaustion path), and held
+ // tasks are invisible to stealers, so keeping the list short
+ // limits how long they can be delayed. 16 * sizeof(bthread_t)
+ // == 128 bytes == two cache lines.
+ bthread_t pending[16];
+ int npending = 0;
+ pending[npending++] = run_tid;
+ bool found = false;
+ while (npending < 16 && pop_task(&run_tid)) {
+ if (address_meta(run_tid)->attr.stack_type !=
+ BTHREAD_STACKTYPE_PTHREAD) {
+ found = true;
+ break;
+ }
+ pending[npending++] = run_tid;
+ }
+ // Push the held tasks back. The pops above freed at least
+ // |npending| slots, _rq has a single producer (this worker)
+ // and stealers never push, so these pushes must succeed.
+ // Reverse order restores their relative order in the queue.
+ for (int i = npending - 1; i >= 0; --i) {
+ const bool pushed = _rq.push(pending[i]);
+ CHECK(pushed) << "push_rq: failed to put back held task";
+ }
+ if (!found) {
+ // Everything within reach is pthread-mode: wait for
+ // stealers to drain the queue.
+ ::usleep(1000);
+ continue;
+ }
+ // Fall through: run_tid is a runnable task, run it inline
+ // below to free a slot for |tid|.
+ }
+ // Run the popped bthread in place instead of spinning, otherwise
+ // all workers may spin here and no one drains the queues any more.
+ // The pop also frees one slot, which guarantees that the deferred
+ // push of the current task succeeds: _rq has a single producer (the
+ // owning worker) and stealers never push.
+ // |args| lives on the stack that sched_to() is about to freeze and is
+ // read by the remained-callback after the switch, so it must stay in
+ // this scope: if it were declared inside the branch below, its slot
+ // could be reused before sched_to() reads it.
+ ReadyToRunArgs args = { tag(), nullptr, false };
+ if (!is_current_main_task()) {
+ // Preserve the task being scheduled out. The push is deferred to
+ // a remained-callback: publishing a still-running task into _rq
+ // lets a stealer jump into it and run the same context
+ // concurrently on another worker. The callback runs on the stack
+ // of |run_tid| after sched_to() has frozen the current context,
+ // same as exchange(). Capture the meta now, current_tid() is
+ // |run_tid| by the time the callback runs. Note that push_rq()
+ // can be re-entered from remained-callbacks, in which case the
+ // task to preserve is still the one being frozen here.
+ args.meta = _cur_meta;
+ set_remained(ready_to_run_in_worker_ignoresignal, &args);
+ }
+ // The main task is never queued; ending_sched() resumes it via
+ // _main_tid when the queue drains. Jumping to a pthread-mode task
+ // from a normal bthread resumes the dispatch loop on _main_stack
+ // which runs it inline, same as ending_sched() does.
+ TaskGroup* g = this;
+ sched_to(&g, run_tid);
Review Comment:
[replied by brpc-oncall robot] Still unaddressed from the previous review: push_rq() is called from remained callbacks (ready_to_run_in_worker, ready_to_run_in_worker_ignoresignal, priority_to_run). Those run at the beginning of the next task in task_runner(), and inside sched_to() right after jump_stack(); in both places the increment of `_sched_recursive_guard` done by the switch that led there is only undone *after* the remained loop. So this sched_to() nests: the guard goes 1 -> 2 and sched_to() hits `LOG(FATAL) << "Recursively(...) call sched_to"` in non-NDEBUG builds.
This is on the exact path the fix targets: when a bthread keeps spawning children until _rq is full, the parent's ready_to_run() runs as the remained callback of the child that was just started and lands in the full-queue branch here. Please handle it, e.g. make `_sched_recursive_guard` unconditional and, when it is non-zero (we are inside a remained callback), skip the inline run and fall back to the bounded wait, or move the drain-and-run out of push_rq() into the scheduler loop.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix livelock when all TaskGroup runqueues are full (#1038) (brpc)
Posted by "two-headBoy (via GitHub)" <gi...@apache.org>.
two-headBoy commented on code in PR #3633:
URL: https://github.com/apache/brpc/pull/3633#discussion_r4399935030
##########
src/bthread/task_group_inl.h:
##########
@@ -93,11 +93,100 @@ inline void TaskGroup::push_rq(bthread_t tid) {
// brpc)
flush_nosignal_tasks();
LOG_EVERY_SECOND(ERROR) << "_rq is full, capacity=" << _rq.capacity();
- // TODO(gejun): May cause deadlock when all workers are spinning here.
- // A better solution is to pop and run existing bthreads, however which
- // make set_remained()-callbacks do context switches and need extensive
- // reviews on related code.
- ::usleep(1000);
+ if (is_current_pthread_task() && !is_current_main_task()) {
+ // A pthread-mode bthread runs on _main_stack and cannot be
+ // scheduled out, fall back to waiting.
+ ::usleep(1000);
+ continue;
+ }
+ const auto pop_task = [this](bthread_t* t) -> bool {
+#ifndef BTHREAD_FAIR_WSQ
+ return _rq.pop(t);
+#else
Review Comment:
Marking this as incorrect: there is no separate fair-WSQ implementation —`_rq` is always`WorkStealingQueue<bthread_t>` and`pop()` is always available;`BTHREAD_FAIR_WSQ` only selects`pop` vs`steal` .`steal()` is intentional here to keep FIFO consumption, consistent with`ending_sched()` /`sched()` .
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix livelock when all TaskGroup runqueues are full (#1038) (brpc)
Posted by "two-headBoy (via GitHub)" <gi...@apache.org>.
two-headBoy commented on code in PR #3633:
URL: https://github.com/apache/brpc/pull/3633#discussion_r4400378441
##########
src/bthread/task_group_inl.h:
##########
@@ -93,11 +93,100 @@ inline void TaskGroup::push_rq(bthread_t tid) {
// brpc)
flush_nosignal_tasks();
LOG_EVERY_SECOND(ERROR) << "_rq is full, capacity=" << _rq.capacity();
- // TODO(gejun): May cause deadlock when all workers are spinning here.
- // A better solution is to pop and run existing bthreads, however which
- // make set_remained()-callbacks do context switches and need extensive
- // reviews on related code.
- ::usleep(1000);
+ if (is_current_pthread_task() && !is_current_main_task()) {
+ // A pthread-mode bthread runs on _main_stack and cannot be
+ // scheduled out, fall back to waiting.
+ ::usleep(1000);
+ continue;
+ }
+ const auto pop_task = [this](bthread_t* t) -> bool {
+#ifndef BTHREAD_FAIR_WSQ
+ return _rq.pop(t);
+#else
+ return _rq.steal(t);
+#endif
+ };
+ bthread_t run_tid = 0;
+ if (!pop_task(&run_tid)) {
+ // The queue was drained by stealers between the failed push and
+ // the pop, retrying the push will succeed.
+ continue;
+ }
+ if (is_current_main_task() &&
+ address_meta(run_tid)->attr.stack_type == BTHREAD_STACKTYPE_PTHREAD) {
+ // pthread-mode tasks are run inline on _main_stack by the
+ // dispatch loops (run_main_task/task_runner). Switching to one
+ // from the main task itself causes no stack jump and would
+ // silently drop it, so it cannot be run here. But pushing it
+ // straight back makes the LIFO pop() return this same task
+ // forever, hiding any runnable task sitting deeper in the
+ // queue. Hold pthread-mode tasks aside and keep popping until
+ // a runnable one shows up.
+ // Bounded budget: the array must live on the worker's stack
+ // (no allocation on a resource-exhaustion path), and held
+ // tasks are invisible to stealers, so keeping the list short
+ // limits how long they can be delayed. 16 * sizeof(bthread_t)
+ // == 128 bytes == two cache lines.
+ bthread_t pending[16];
+ int npending = 0;
+ pending[npending++] = run_tid;
+ bool found = false;
+ while (npending < 16 && pop_task(&run_tid)) {
+ if (address_meta(run_tid)->attr.stack_type !=
+ BTHREAD_STACKTYPE_PTHREAD) {
+ found = true;
+ break;
+ }
+ pending[npending++] = run_tid;
+ }
Review Comment:
Won't fix. The 16-entry cap and the fallback are a deliberate trade-off: the array must live on the worker's stack on a resource-exhaustion path, and held tasks are invisible to stealers while they are in`pending[]` , so the list has to stay short. "Iterative batches" cannot reach deeper work because`pop()` is LIFO — after re-pushing the same set, the next batch sees the identical tasks. Moving them aside (e.g. to`_remote_rq` ) would surface deeper work but adds locking and reordering on this path. When`found == false` the held tasks are pushed back and remain stealable, so liveness only degrades if every worker is stuck here at once. This has been reviewed and accepted upstream as well.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org