gitccl commented on code in PR #3009:
URL: https://github.com/apache/brpc/pull/3009#discussion_r2173169174
##########
src/bthread/task_control.cpp:
##########
@@ -524,12 +525,29 @@ void TaskControl::print_rq_sizes(std::ostream& os) {
double TaskControl::get_cumulated_worker_time() {
int64_t cputime_ns = 0;
+ int64_t now = butil::cpuwide_time_ns();
BAIDU_SCOPED_LOCK(_modify_group_mutex);
for_each_task_group([&](TaskGroup* g) {
if (g) {
- cputime_ns += g->_cumulated_cputime_ns;
+ // With the acquire-release atomic operation, the CPU time of the
bthread is
+ // only calculated once through `_cumulated_cputime_ns' or
`_last_run_ns'.
+ cputime_ns +=
g->_cumulated_cputime_ns.load(butil::memory_order_acquire);
+ // The bthread is still running on the worker,
+ // so we need to add the elapsed time since it started.
+ // In extreme cases, before getting `_last_run_ns_in_tc',
+ // `_last_run_ns_in_tc' may have been updated multiple times,
+ // and `cputime_ns' will miss some cpu time, which is ok.
+ int64_t last_run_ns = g->_last_run_ns;
+ if (last_run_ns > _last_get_cumulated_time_ns) {
+ g->_last_run_ns_in_tc = last_run_ns;
+ cputime_ns += now - last_run_ns;
+ } else if (last_run_ns == g->_last_run_ns_in_tc) {
+ // The bthread is still running on the same worker.
+ cputime_ns += now - last_run_ns;
+ }
Review Comment:
This change may introduce a subtle race condition when sampling cumulative
CPU time.
Suppose `bthread_worker_usage` starts sampling at T=0s, and a task also
starts execution at the same time. The task keeps running continuously for 3s.
According to the current implementation, `get_cumulated_worker_time()` will
return approximately 0, 1, and 2 at T=0s, T=1s, and T=2s respectively — which
is expected.
However, at T=3s, a race can occur as follows:
- The sampling thread first reads `_cumulated_cputime_ns`, which is still 0.
- Then, the worker thread updates `_last_run_ns` and `_cumulated_cputime_ns`
to 3e9.
- The sampling thread then proceeds to read `_last_run_ns` = 3e9.
- Since `last_run_ns` > `_last_get_cumulated_time_ns`, it goes into the
branch:
```
cputime_ns += now - last_run_ns;
```
But `now` was captured earlier and is slightly less than `last_run_ns`,
resulting in a **negative delta**.
As a result, the sampling thread misses the 3 seconds of accumulated CPU
time, causes `get_cumulated_worker_time()` at T=3s to return a smaller value
than at T=2s(**maybe even return a negative value**), finally leads to a
negative value in `bthread_worker_usage`.
To prevent this issue, `_cumulated_cputime_ns` and `_last_run_ns` should be
read together under a consistent view. One possible approach without using a
lock maybe to read `_last_run_ns` **before and after** reading
`_cumulated_cputime_ns`, and verify that both `_last_run_ns` reads return the
same value.
--
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: dev-unsubscr...@brpc.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@brpc.apache.org
For additional commands, e-mail: dev-h...@brpc.apache.org
