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