Copilot commented on code in PR #3351:
URL: https://github.com/apache/brpc/pull/3351#discussion_r3440773844
##########
src/bthread/task_group.cpp:
##########
@@ -1118,7 +1118,7 @@ int TaskGroup::interrupt(bthread_t tid, TaskControl* c,
bthread_tag_t tag) {
if (!c) {
return EINVAL;
}
- c->choose_one_group(tag)->ready_to_run_remote(m);
+ c->choose_one_group(m->attr.tag)->ready_to_run_remote(m);
}
Review Comment:
`m->attr.tag` is not guaranteed to be a valid tag to pass into
`TaskControl::choose_one_group()`. In current code, `TaskMeta::attr` is copied
from the user-provided `bthread_attr_t` (often `BTHREAD_ATTR_NORMAL`), whose
`tag` defaults to `BTHREAD_TAG_INVALID (-1)`. Using it here can trigger
`CHECK(tag >= BTHREAD_TAG_DEFAULT ...)` inside `choose_one_group()` or still
mis-route tasks whose effective tag is the current TaskGroup but whose
specified tag is INVALID. To safely choose the correct tag group on
interruption from a non-worker thread, the effective tag needs to be stored
explicitly (e.g., set `TaskMeta::attr.tag` to the TaskGroup’s actual tag at
creation time, or add a separate `TaskMeta` field for the effective tag and use
that here).
##########
src/bthread/butex.cpp:
##########
@@ -446,7 +445,7 @@ int butex_wake_except(void* arg, bthread_t
excluded_bthread) {
ButexBthreadWaiter* w =
static_cast<ButexBthreadWaiter*>(bthread_waiters.tail()->value());
w->RemoveFromList();
unsleep_if_necessary(w, get_global_timer_thread());
- auto g = get_task_group(w->control, w->tag);
+ auto g = get_task_group(w->control, w->task_meta->attr.tag);
g->ready_to_run_general(w->task_meta, true);
Review Comment:
`w->task_meta->attr.tag` may be `BTHREAD_TAG_INVALID (-1)` for many bthreads
(default attrs), so `get_task_group()` can call `choose_one_group(-1)` and hit
a CHECK-failure. Wakeup should use the bthread’s effective/resolved tag, not
the possibly-invalid specified tag stored in `TaskMeta::attr`.
##########
src/bthread/butex.cpp:
##########
@@ -373,7 +372,7 @@ int butex_wake_n(void* arg, size_t n, bool nosignal) {
bthread_waiters.tail()->value());
w->RemoveFromList();
unsleep_if_necessary(w, get_global_timer_thread());
- auto g = get_task_group(w->control, w->tag);
+ auto g = get_task_group(w->control, w->task_meta->attr.tag);
g->ready_to_run_general(w->task_meta, true);
nwakeups[g->tag()] = g;
Review Comment:
Same issue as in `butex_wake`: `w->task_meta->attr.tag` may be
`BTHREAD_TAG_INVALID (-1)` for normal bthreads, which can make
`get_task_group()`/`choose_one_group()` crash via CHECKs or schedule onto the
wrong tag group. This code needs the bthread’s effective tag rather than the
raw `TaskMeta::attr.tag`.
##########
src/bthread/butex.cpp:
##########
@@ -531,7 +531,8 @@ inline bool erase_from_butex(ButexWaiter* bw, bool wakeup,
WaiterState state) {
if (erased && wakeup) {
if (bw->tid) {
ButexBthreadWaiter* bbw = static_cast<ButexBthreadWaiter*>(bw);
- get_task_group(bbw->control,
bbw->tag)->ready_to_run_general(bbw->task_meta);
+ auto g = get_task_group(bbw->control, bbw->task_meta->attr.tag);
+ g->ready_to_run_general(bbw->task_meta);
Review Comment:
`bbw->task_meta->attr.tag` may be `BTHREAD_TAG_INVALID (-1)` (default
attrs), so `get_task_group()` can end up calling `choose_one_group(-1)` and
fail. The wakeup should use the effective/resolved tag of the bthread rather
than the raw specified tag stored in `TaskMeta::attr`.
##########
src/bthread/butex.cpp:
##########
@@ -320,7 +319,7 @@ int butex_wake(void* arg, bool nosignal) {
}
ButexBthreadWaiter* bbw = static_cast<ButexBthreadWaiter*>(front);
unsleep_if_necessary(bbw, get_global_timer_thread());
- TaskGroup* g = get_task_group(bbw->control, bbw->tag);
+ TaskGroup* g = get_task_group(bbw->control, bbw->task_meta->attr.tag);
if (g == tls_task_group) {
Review Comment:
`bbw->task_meta->attr.tag` is not necessarily a valid scheduling tag.
`TaskMeta::attr` is copied from the user attribute and commonly has `tag ==
BTHREAD_TAG_INVALID (-1)` (e.g. `BTHREAD_ATTR_NORMAL`). Passing this value into
`get_task_group(..., tag)` can reach `TaskControl::choose_one_group(tag)` and
trip its `CHECK(tag >= BTHREAD_TAG_DEFAULT ...)`, or select the wrong tag
group. The wakeup path should use the bthread’s effective tag (resolved to an
actual TaskGroup tag) rather than the user-specified/possibly-invalid
`attr.tag`.
##########
src/bthread/butex.cpp:
##########
@@ -489,11 +488,12 @@ int butex_requeue(void* arg, void* arg2) {
}
ButexBthreadWaiter* bbw = static_cast<ButexBthreadWaiter*>(front);
unsleep_if_necessary(bbw, get_global_timer_thread());
- auto g = is_same_tag(bbw->tag) ? tls_task_group : NULL;
+ auto g = is_same_tag(bbw->task_meta->attr.tag) ? tls_task_group : NULL;
if (g) {
TaskGroup::exchange(&g, bbw->task_meta);
} else {
-
bbw->control->choose_one_group(bbw->tag)->ready_to_run_remote(bbw->task_meta);
+ g = bbw->control->choose_one_group(bbw->task_meta->attr.tag);
+ g->ready_to_run_remote(bbw->task_meta);
Review Comment:
`bbw->task_meta->attr.tag` is used both for the fast-path tag comparison and
for selecting a remote TaskGroup, but `TaskMeta::attr.tag` may be
`BTHREAD_TAG_INVALID (-1)` for normal bthreads. This can (1) make
`is_same_tag()` always false even when the waiter is in the current tag group,
and (2) crash in `choose_one_group(-1)` due to CHECKs. This path should use the
bthread’s effective/resolved tag (actual TaskGroup tag), not the user-specified
`attr.tag`.
--
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]
