Copilot commented on code in PR #3641:
URL: https://github.com/apache/brpc/pull/3641#discussion_r4349535061
##########
src/bthread/remote_task_queue.h:
##########
@@ -66,7 +72,12 @@ class RemoteTaskQueue {
}
bool push_locked(bthread_t task) {
- return _tasks.push(task);
+ if (!_tasks.push(task)) {
+ return false;
+ }
+ const size_t ntask = _ntask.load(butil::memory_order_relaxed);
+ _ntask.store(ntask + 1, butil::memory_order_relaxed);
+ return true;
Review Comment:
Using `memory_order_relaxed` for `_ntask` updates allows reordering w.r.t.
the queue mutation (`_tasks.push/pop`). Because `pop()` can return early based
solely on `_ntask == 0` (without locking), it’s important that `_ntask` never
becomes `0` ‘before’ the queue is actually emptied (and never becomes non-zero
‘before’ the queue actually has the item). Consider switching the `_ntask`
writes to at least release semantics (and the empty fast-path load to acquire),
and prefer `fetch_add/fetch_sub` over load+store. This creates an explicit
ordering edge that prevents the counter from being observed in a state that
contradicts the actual queue contents and avoids subtle reorder-induced missed
work.
##########
src/bthread/remote_task_queue.h:
##########
@@ -49,11 +50,16 @@ class RemoteTaskQueue {
}
bool pop(bthread_t* task) {
- if (_tasks.empty()) {
+ if (_ntask.load(butil::memory_order_relaxed) == 0) {
return false;
}
_mutex.lock();
const bool result = _tasks.pop(task);
+ if (result) {
+ const size_t ntask = _ntask.load(butil::memory_order_relaxed);
+ DCHECK_GT(ntask, 0u);
+ _ntask.store(ntask - 1, butil::memory_order_relaxed);
+ }
Review Comment:
Using `memory_order_relaxed` for `_ntask` updates allows reordering w.r.t.
the queue mutation (`_tasks.push/pop`). Because `pop()` can return early based
solely on `_ntask == 0` (without locking), it’s important that `_ntask` never
becomes `0` ‘before’ the queue is actually emptied (and never becomes non-zero
‘before’ the queue actually has the item). Consider switching the `_ntask`
writes to at least release semantics (and the empty fast-path load to acquire),
and prefer `fetch_add/fetch_sub` over load+store. This creates an explicit
ordering edge that prevents the counter from being observed in a state that
contradicts the actual queue contents and avoids subtle reorder-induced missed
work.
--
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]
