Copilot commented on code in PR #3122:
URL: https://github.com/apache/brpc/pull/3122#discussion_r2453972860
##########
src/bthread/task_control.cpp:
##########
@@ -42,6 +42,9 @@ DEFINE_int32(task_group_runqueue_capacity, 4096,
DEFINE_int32(task_group_ntags, 1, "TaskGroup will be grouped by number ntags");
DEFINE_bool(task_group_set_worker_name, true,
"Whether to set the name of the worker thread");
+DEFINE_bool(thread_affinity, false, "Whether to Bind Cores");
+DEFINE_string(cpu_set , "",
Review Comment:
Extra space before comma in 'cpu_set ,'. Should be 'cpu_set,' without the
space.
```suggestion
DEFINE_string(cpu_set, "",
```
##########
src/bthread/task_control.cpp:
##########
@@ -87,6 +100,9 @@ void* TaskControl::worker_thread(void* arg) {
auto dummy = static_cast<WorkerThreadArgs*>(arg);
auto c = dummy->c;
auto tag = dummy->tag;
+ if (FLAGS_thread_affinity) {
+ bind_thread(pthread_self(), _cpus[dummy->cpuId]);
Review Comment:
Array index access without bounds checking. If `cpuId` is greater than or
equal to `_cpus.size()`, this will cause out-of-bounds access. Add validation
that `dummy->cpuId < _cpus.size()` before accessing the array.
```suggestion
if (dummy->cpuId < _cpus.size()) {
bind_thread(pthread_self(), _cpus[dummy->cpuId]);
} else {
LOG(ERROR) << "cpuId " << dummy->cpuId << " is out of bounds for
_cpus (size=" << _cpus.size() << ")";
delete dummy;
return NULL;
}
```
##########
src/bthread/task_control.cpp:
##########
@@ -284,6 +308,7 @@ int TaskControl::add_workers(int num, bthread_tag_t tag) {
// _concurrency before create a worker.
_concurrency.fetch_add(1);
auto arg = new WorkerThreadArgs(this, tag);
+ arg->set_cpuId((i + old_concurency) % _cpus.size());
Review Comment:
Potential division by zero if `thread_affinity` is false. When
`thread_affinity` is disabled, `_cpus` is never populated and remains empty,
causing `_cpus.size()` to be 0. This line should only execute when
`thread_affinity` is enabled.
##########
src/bthread/task_control.cpp:
##########
@@ -241,6 +264,7 @@ int TaskControl::init(int concurrency) {
_workers.resize(_concurrency);
for (int i = 0; i < _concurrency; ++i) {
auto arg = new WorkerThreadArgs(this, i % FLAGS_task_group_ntags);
+ arg->set_cpuId(i % _cpus.size());
Review Comment:
Potential division by zero if `thread_affinity` is false. When
`thread_affinity` is disabled, `_cpus` is never populated and remains empty,
causing `_cpus.size()` to be 0. This line should only execute when
`thread_affinity` is enabled.
```suggestion
if (FLAGS_thread_affinity) {
arg->set_cpuId(i % _cpus.size());
}
```
##########
src/bthread/task_control.cpp:
##########
@@ -284,6 +308,7 @@ int TaskControl::add_workers(int num, bthread_tag_t tag) {
// _concurrency before create a worker.
_concurrency.fetch_add(1);
auto arg = new WorkerThreadArgs(this, tag);
+ arg->set_cpuId((i + old_concurency) % _cpus.size());
Review Comment:
Misspelled variable name 'old_concurency' should be 'old_concurrency'.
--
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]
