Copilot commented on code in PR #3331:
URL: https://github.com/apache/brpc/pull/3331#discussion_r3361028493
##########
src/bthread/task_control.cpp:
##########
@@ -208,6 +214,8 @@ TaskControl::TaskControl()
, _priority_queues(FLAGS_task_group_ntags)
, _pl_num_of_each_tag(FLAGS_bthread_parking_lot_of_each_tag)
, _tagged_pl(FLAGS_task_group_ntags)
+ , _tag_cpus(FLAGS_task_group_ntags)
+ , _tag_next_worker_id(FLAGS_task_group_ntags)
{}
Review Comment:
_tag_next_worker_id is constructed with default-constructed
butil::atomic<int> elements. In butil/atomicops.h, atomic() is an empty
constructor (does not initialize the value), so tag_wid fetch_add() later reads
an indeterminate starting value (undefined behavior). Initialize each element
to 0 in the constructor body.
##########
src/bthread/task_control.cpp:
##########
@@ -106,15 +111,17 @@ void* TaskControl::worker_thread(void* arg) {
}
g->_tid = pthread_self();
-
- int worker_id = c->_next_worker_id.fetch_add(
- 1, butil::memory_order_relaxed);
- if (!c->_cpus.empty()) {
- bind_thread_to_cpu(pthread_self(), c->_cpus[worker_id %
c->_cpus.size()]);
+ // tag_wid is a per-tag monotonic counter: same-tag workers get 0,1,2,...
+ // Used both for CPU round-robin affinity and the thread name suffix.
+ int tag_wid = c->_tag_next_worker_id[tag].fetch_add(
+ 1, butil::memory_order_relaxed);
Review Comment:
PR description says per-tag affinity round-robins using a global
_next_worker_id counter, but the implementation now uses a per-tag counter
(_tag_next_worker_id[tag]) for round-robin and naming. Please update the PR
description (or the implementation) so they match to avoid reviewer/user
confusion.
##########
src/bthread/task_control.cpp:
##########
@@ -328,40 +336,103 @@ TaskGroup* TaskControl::choose_one_group(bthread_tag_t
tag) {
return NULL;
}
-int TaskControl::parse_cpuset(std::string value, std::vector<unsigned>& cpus) {
+// Parse a single cpu-range-list such as "0-3,5,7" into a sorted, deduplicated
+// vector of CPU IDs. Returns 0 on success, -1 on error.
+static int parse_one_cpuset(const std::string& value, std::vector<unsigned>&
cpus) {
static std::regex r("(\\d+-)?(\\d+)(,(\\d+-)?(\\d+))*");
std::smatch match;
std::set<unsigned> cpuset;
if (value.empty()) {
return -1;
}
- if (std::regex_match(value, match, r)) {
- for (butil::StringSplitter split(value.data(), ','); split; ++split) {
- butil::StringPiece cpu_ids(split.field(), split.length());
- cpu_ids.trim_spaces();
- butil::StringPiece begin = cpu_ids;
- butil::StringPiece end = cpu_ids;
- auto dash = cpu_ids.find('-');
- if (dash != cpu_ids.npos) {
- begin = cpu_ids.substr(0, dash);
- end = cpu_ids.substr(dash + 1);
+ if (!std::regex_match(value, match, r)) {
+ return -1;
+ }
+ for (butil::StringSplitter split(value.data(), ','); split; ++split) {
+ butil::StringPiece cpu_ids(split.field(), split.length());
+ cpu_ids.trim_spaces();
+ butil::StringPiece begin = cpu_ids;
+ butil::StringPiece end = cpu_ids;
+ auto dash = cpu_ids.find('-');
+ if (dash != cpu_ids.npos) {
+ begin = cpu_ids.substr(0, dash);
+ end = cpu_ids.substr(dash + 1);
+ }
+ unsigned first = UINT_MAX;
+ unsigned last = 0;
+ int ret = butil::StringSplitter(begin, '\t').to_uint(&first);
+ ret = ret | butil::StringSplitter(end, '\t').to_uint(&last);
+ if (ret != 0 || first > last) {
+ return -1;
+ }
+ for (auto i = first; i <= last; ++i) {
+ cpuset.insert(i);
+ }
Review Comment:
The range expansion loop can overflow when last == UINT_MAX (e.g., value
"4294967295"), causing i++ to wrap to 0 and the loop to never terminate. Use a
loop that breaks on i == last to avoid overflow.
--
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]
