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