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