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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
