From 1b3206c8f6e49db8d63d22f084e948dd4bfdcc1b Mon Sep 17 00:00:00 2001 From: Vadim Skipin Date: Thu, 7 May 2026 11:58:36 +0000 Subject: [PATCH] Fix CPU topology parsing, add UTs --- src/fibers/cpu.cpp | 84 ++++++++++++++++++++++++---------- src/fibers/cpu.h | 7 +++ src/fibers/tests/cpu-test.cpp | 85 ++++++++++++++++++++++++++++++++++- 3 files changed, 152 insertions(+), 24 deletions(-) diff --git a/src/fibers/cpu.cpp b/src/fibers/cpu.cpp index b13dc77..8529580 100644 --- a/src/fibers/cpu.cpp +++ b/src/fibers/cpu.cpp @@ -45,38 +45,70 @@ static int readSysfsUint32(const char * path, uint32_t * out) noexcept return 0; } -static bool cpuInCpulist(uint32_t cpu, const char * list) noexcept +// Parse a non-negative decimal integer at *p, advancing p past the digits. +// Returns the parsed value, or 0 if no digits are present. +static uint32_t parseUint32(const char *& p) noexcept { - const char * p = list; - while (*p && *p != '\n') + uint32_t value = 0; + while (*p >= '0' && *p <= '9') { - uint32_t start = 0; - while (*p >= '0' && *p <= '9') - { - start = start * 10 + static_cast(*p++ - '0'); - } - uint32_t end = start; - if (*p == '-') + value = value * 10 + static_cast(*p++ - '0'); + } + return value; +} + +// Parse a single Linux cpulist entry "[a[-b[:c[/d]]]]" and return whether @p cpu +// is included. The kernel's full grammar (see bitmap_parselist) selects bit +// positions in [a, b] for which (pos - a) % d < c -- the stride form is rare +// outside of cgroup-restricted layouts but is valid sysfs output. +static bool cpuInCpulistEntry(uint32_t cpu, const char *& p) noexcept +{ + uint32_t start = parseUint32(p); + uint32_t end = start; + if (*p == '-') + { + ++p; + end = parseUint32(p); + } + + // Optional stride: ":used[/group]". used defaults to 1 (every position), + // group defaults to used (a:c is shorthand for a:c/c, which selects all + // positions in the range). + uint32_t used = 1; + uint32_t group = 1; + if (*p == ':') + { + ++p; + used = parseUint32(p); + group = used; + if (*p == '/') { ++p; - end = 0; - while (*p >= '0' && *p <= '9') - { - end = end * 10 + static_cast(*p++ - '0'); - } + group = parseUint32(p); } - if (cpu >= start && cpu <= end) + } + + if (cpu < start || cpu > end || group == 0) + { + return false; + } + return (cpu - start) % group < used; +} + +bool cpuInCpulist(uint32_t cpu, const char * list) noexcept +{ + const char * p = list; + while (*p && *p != '\n') + { + if (cpuInCpulistEntry(cpu, p)) { return true; } - if (*p == ',') - { - ++p; - } - else + if (*p != ',') { break; } + ++p; } return false; } @@ -93,7 +125,10 @@ void readCpuTopologies(CpuTopology * topologies, uint32_t processorCount) noexce ::snprintf(path, sizeof(path), "/sys/devices/system/cpu/cpu%u/topology/core_id", cpu); readSysfsUint32(path, &topologies[cpu].coreId); - topologies[cpu].numaNodeId = 0; + // numaNodeId stays at UINT32_MAX until the NUMA pass below sets it. + // In environments without NUMA sysfs (containers, minimal kernels) it + // remains UINT32_MAX so topologyCostCycles falls through to the safe + // cross-NUMA cost rather than misclassifying every CPU as same-node. } // Open each NUMA node file once and fill all CPUs that belong to it. @@ -137,7 +172,10 @@ uint64_t topologyCostCycles(const CpuTopology & first, const CpuTopology & secon // HT sibling ~1 us return Tsc::nanosecondsToCycles(1'000); } - if (first.numaNodeId == second.numaNodeId) + // Treat unknown NUMA (UINT32_MAX, e.g. containers without /sys/devices/system/node) + // as cross-NUMA: the equality check below would otherwise classify two unknown + // CPUs as same-node. + if (first.numaNodeId != UINT32_MAX && first.numaNodeId == second.numaNodeId) { // same NUMA ~50 us return Tsc::nanosecondsToCycles(50'000); diff --git a/src/fibers/cpu.h b/src/fibers/cpu.h index 2246e02..446da02 100644 --- a/src/fibers/cpu.h +++ b/src/fibers/cpu.h @@ -24,4 +24,11 @@ void readCpuTopologies(CpuTopology * topologies, uint32_t processorCount) noexce */ uint64_t topologyCostCycles(const CpuTopology & first, const CpuTopology & second) noexcept; +/** + * Test whether @p cpu is in a Linux cpulist string. Accepts the kernel grammar + * "[a[-b[:c[/d]]]](,...)" with optional stride: positions in [a, b] for which + * (pos - a) % d < c. Defaults are b=a, c=1, d=c. Exposed for unit tests. + */ +bool cpuInCpulist(uint32_t cpu, const char * list) noexcept; + } // namespace silk diff --git a/src/fibers/tests/cpu-test.cpp b/src/fibers/tests/cpu-test.cpp index 5a7c36c..181a1c1 100644 --- a/src/fibers/tests/cpu-test.cpp +++ b/src/fibers/tests/cpu-test.cpp @@ -65,7 +65,90 @@ TEST(CpuTopology, crossNumaCost) EXPECT_EQ(topologyCostCycles(a, b), Tsc::nanosecondsToCycles(500'000)); } -// readCpuTopologies: smoke test — must not crash and should populate at least +// Container-style: package and core known but no NUMA sysfs, so numaNodeId +// stays at the default UINT32_MAX. Both ends are "unknown NUMA"; the cost +// must fall through to cross-NUMA, not classify them as same-node by virtue +// of UINT32_MAX == UINT32_MAX. +TEST(CpuTopology, unknownNumaReturnsCrossNumaCost) +{ + CpuTopology a{0, 0, UINT32_MAX}; + CpuTopology b{1, 0, UINT32_MAX}; + EXPECT_EQ(topologyCostCycles(a, b), Tsc::nanosecondsToCycles(500'000)); +} + +// cpuInCpulist: single CPU. +TEST(CpuTopology, cpuInCpulistSingle) +{ + EXPECT_TRUE(cpuInCpulist(5, "5")); + EXPECT_FALSE(cpuInCpulist(4, "5")); + EXPECT_FALSE(cpuInCpulist(6, "5")); +} + +// cpuInCpulist: contiguous range. +TEST(CpuTopology, cpuInCpulistRange) +{ + EXPECT_TRUE(cpuInCpulist(0, "0-7")); + EXPECT_TRUE(cpuInCpulist(7, "0-7")); + EXPECT_TRUE(cpuInCpulist(3, "0-7")); + EXPECT_FALSE(cpuInCpulist(8, "0-7")); +} + +// cpuInCpulist: comma-separated entries. +TEST(CpuTopology, cpuInCpulistMultiple) +{ + EXPECT_TRUE(cpuInCpulist(1, "0-3,5,7-9")); + EXPECT_TRUE(cpuInCpulist(5, "0-3,5,7-9")); + EXPECT_TRUE(cpuInCpulist(9, "0-3,5,7-9")); + EXPECT_FALSE(cpuInCpulist(4, "0-3,5,7-9")); + EXPECT_FALSE(cpuInCpulist(6, "0-3,5,7-9")); + EXPECT_FALSE(cpuInCpulist(10, "0-3,5,7-9")); +} + +// cpuInCpulist: stride syntax. "0-7:2/4" means positions in [0,7] for which +// (pos - 0) % 4 < 2 -- i.e. {0,1,4,5}. +TEST(CpuTopology, cpuInCpulistStride) +{ + EXPECT_TRUE(cpuInCpulist(0, "0-7:2/4")); + EXPECT_TRUE(cpuInCpulist(1, "0-7:2/4")); + EXPECT_FALSE(cpuInCpulist(2, "0-7:2/4")); + EXPECT_FALSE(cpuInCpulist(3, "0-7:2/4")); + EXPECT_TRUE(cpuInCpulist(4, "0-7:2/4")); + EXPECT_TRUE(cpuInCpulist(5, "0-7:2/4")); + EXPECT_FALSE(cpuInCpulist(6, "0-7:2/4")); + EXPECT_FALSE(cpuInCpulist(7, "0-7:2/4")); + EXPECT_FALSE(cpuInCpulist(8, "0-7:2/4")); +} + +// cpuInCpulist: ":c" without "/d" defaults d=c, so all positions in the range +// are selected (kernel shorthand for the no-stride case). +TEST(CpuTopology, cpuInCpulistStrideDefaultsGroupToUsed) +{ + EXPECT_TRUE(cpuInCpulist(0, "0-7:2")); + EXPECT_TRUE(cpuInCpulist(3, "0-7:2")); + EXPECT_TRUE(cpuInCpulist(7, "0-7:2")); + EXPECT_FALSE(cpuInCpulist(8, "0-7:2")); +} + +// cpuInCpulist: stride combined with comma list. +TEST(CpuTopology, cpuInCpulistStrideMixed) +{ + // "0-3,8-15:1/2": {0,1,2,3} ∪ {8,10,12,14} + EXPECT_TRUE(cpuInCpulist(2, "0-3,8-15:1/2")); + EXPECT_FALSE(cpuInCpulist(4, "0-3,8-15:1/2")); + EXPECT_TRUE(cpuInCpulist(8, "0-3,8-15:1/2")); + EXPECT_FALSE(cpuInCpulist(9, "0-3,8-15:1/2")); + EXPECT_TRUE(cpuInCpulist(14, "0-3,8-15:1/2")); + EXPECT_FALSE(cpuInCpulist(15, "0-3,8-15:1/2")); +} + +// cpuInCpulist: trailing newline must be tolerated (sysfs reads include it). +TEST(CpuTopology, cpuInCpulistTrailingNewline) +{ + EXPECT_TRUE(cpuInCpulist(3, "0-7\n")); + EXPECT_FALSE(cpuInCpulist(8, "0-7\n")); +} + +// readCpuTopologies: smoke test -- must not crash and should populate at least // one CPU with a valid package ID on a real Linux system. TEST(CpuTopology, readDoesNotCrash) {