Copilot commented on code in PR #3633:
URL: https://github.com/apache/brpc/pull/3633#discussion_r4326651238
##########
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:
`_rq.push(...)` is called without checking whether it succeeded. In this
function we’re explicitly in the “queue is/was full” slow-path; if `_rq.push`
returns a status (as is typical for bounded runqueues and implied by the
surrounding retry-loop), ignoring the result can silently drop `run_tid` /
`current_tid()` on failure. Please handle the return value (retry or
assert/LOG+fallback) to guarantee no task is lost on this path.
##########
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);
+ ::usleep(1000);
+ continue;
+ }
+ // 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 push of
+ // the current task below succeeds: _rq has a single producer (the
+ // owning worker) and stealers never push.
+ if (!is_current_main_task()) {
+ // Preserve the task being scheduled out. Note that it is
+ // current_tid() rather than |tid|: push_rq() can be re-entered
+ // from remained-callbacks which run on the stack of the next
+ // task, where the task to preserve is the one whose context
+ // sched_to() is about to freeze.
+ _rq.push(current_tid());
Review Comment:
`_rq.push(...)` is called without checking whether it succeeded. In this
function we’re explicitly in the “queue is/was full” slow-path; if `_rq.push`
returns a status (as is typical for bounded runqueues and implied by the
surrounding retry-loop), ignoring the result can silently drop `run_tid` /
`current_tid()` on failure. Please handle the return value (retry or
assert/LOG+fallback) to guarantee no task is lost on this path.
##########
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);
+ ::usleep(1000);
+ continue;
+ }
+ // 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 push of
+ // the current task below succeeds: _rq has a single producer (the
+ // owning worker) and stealers never push.
+ if (!is_current_main_task()) {
+ // Preserve the task being scheduled out. Note that it is
+ // current_tid() rather than |tid|: push_rq() can be re-entered
+ // from remained-callbacks which run on the stack of the next
+ // task, where the task to preserve is the one whose context
+ // sched_to() is about to freeze.
+ _rq.push(current_tid());
+ }
Review Comment:
This enqueues `current_tid()` onto `_rq` before the current task has
actually been scheduled out (the actual switch happens at `sched_to`). If
stealers can concurrently take items from `_rq`, this can expose a
still-running task as runnable, which risks concurrent execution or other
invariant violations unless there is an explicit state transition/guard that
prevents thieves from running it until after the switch. Consider using the
existing “enqueue-current-and-switch” primitive (if one exists) that atomically
marks the task non-running and publishes it, or introduce a guard (e.g.,
disable stealing for the brief window, or only publish after the scheduler has
committed the switch) that preserves the runqueue/task-state invariants.
##########
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);
+ ::usleep(1000);
+ continue;
+ }
+ // 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 push of
+ // the current task below succeeds: _rq has a single producer (the
+ // owning worker) and stealers never push.
+ if (!is_current_main_task()) {
+ // Preserve the task being scheduled out. Note that it is
+ // current_tid() rather than |tid|: push_rq() can be re-entered
+ // from remained-callbacks which run on the stack of the next
+ // task, where the task to preserve is the one whose context
+ // sched_to() is about to freeze.
+ _rq.push(current_tid());
+ }
+ // 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:
This enqueues `current_tid()` onto `_rq` before the current task has
actually been scheduled out (the actual switch happens at `sched_to`). If
stealers can concurrently take items from `_rq`, this can expose a
still-running task as runnable, which risks concurrent execution or other
invariant violations unless there is an explicit state transition/guard that
prevents thieves from running it until after the switch. Consider using the
existing “enqueue-current-and-switch” primitive (if one exists) that atomically
marks the task non-running and publishes it, or introduce a guard (e.g.,
disable stealing for the brief window, or only publish after the scheduler has
committed the switch) that preserves the runqueue/task-state invariants.
--
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]
