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: [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]
