Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a tiled execution path for the qsim quantum simulator, adding a new qsim_tiled binary, benchmark scripts, and several supporting library files for CPU topology detection, tile scheduling, qubit remapping, and gate batch planning. It also implements best-effort Linux NUMA allocation. The review feedback identifies several critical issues: a buffer overread vulnerability in mbind due to an incorrect maxnode argument, a logic error in CPU replication that limits load balancing, a potential division-by-zero crash when no CPUs are allowed, an early loop termination that misses non-contiguous CPU IDs, and fragile parsing of sysfs files that lacks robust exception handling.
| syscall(SYS_mbind, p, size, MPOL_INTERLEAVE, mask.data(), | ||
| numa_nodes + 1, 0); |
There was a problem hiding this comment.
Passing numa_nodes + 1 as the maxnode argument to mbind causes a buffer overread when numa_nodes is a multiple of the word size (e.g., 64). In this case, words is allocated to fit exactly numa_nodes bits, but the kernel is instructed to read numa_nodes + 1 bits, leading to an out-of-bounds read from the mask vector. Since the maximum node ID is numa_nodes - 1, the number of bits to be used is exactly numa_nodes.
| syscall(SYS_mbind, p, size, MPOL_INTERLEAVE, mask.data(), | |
| numa_nodes + 1, 0); | |
| syscall(SYS_mbind, p, size, MPOL_INTERLEAVE, mask.data(), | |
| numa_nodes, 0); |
| if (result.empty()) result.push_back(0); | ||
| while (result.size() < workers_) result.push_back(result[result.size() % result.size()]); |
There was a problem hiding this comment.
The replication logic result[result.size() % result.size()] always evaluates to result[0] because any non-zero integer modulo itself is 0. This causes the scheduler to repeatedly replicate only the first CPU instead of performing a round-robin replication of all available CPUs in result, leading to poor load balancing on multi-socket/NUMA systems. We should store the original size of result before the loop and use it as the divisor.
| if (result.empty()) result.push_back(0); | |
| while (result.size() < workers_) result.push_back(result[result.size() % result.size()]); | |
| if (result.empty()) result.push_back(0); | |
| const std::size_t original_size = result.size(); | |
| while (result.size() < workers_) result.push_back(result[result.size() % original_size]); |
| std::vector<unsigned> team; | ||
| for (unsigned lane = 0; lane < lanes && lane < core.size(); ++lane) | ||
| team.push_back(core[lane]); | ||
| if (team.empty()) team.push_back(allowed[worker % allowed.size()]); |
There was a problem hiding this comment.
If allowed is empty (which can happen in highly restricted container environments or if CPU topology detection fails to find any CPUs), allowed.size() will be 0, leading to a division-by-zero crash on worker % allowed.size(). We should guard against this by checking if allowed is empty.
| if (team.empty()) team.push_back(allowed[worker % allowed.size()]); | |
| if (team.empty()) team.push_back(!allowed.empty() ? allowed[worker % allowed.size()] : 0); |
| for (unsigned cpu = 0; cpu < 4096; ++cpu) { | ||
| std::string root = "/sys/devices/system/cpu/cpu" + std::to_string(cpu); | ||
| std::string online = Read(root + "/online"); | ||
| if (cpu && online.empty() && access(root.c_str(), F_OK) != 0) break; |
There was a problem hiding this comment.
Breaking the loop on the first missing CPU ID assumes that CPU IDs are always contiguous. However, on Linux systems with hotplugged CPUs, disabled cores, or virtualized environments, CPU IDs can be non-contiguous. Breaking early will cause the topology discovery to miss subsequent online CPUs. Changing break to continue ensures all online CPUs up to 4096 are discovered.
| if (cpu && online.empty() && access(root.c_str(), F_OK) != 0) break; | |
| if (cpu && online.empty() && access(root.c_str(), F_OK) != 0) continue; |
| static std::vector<unsigned> ParseList(const std::string& text) { | ||
| std::vector<unsigned> result; | ||
| std::stringstream ss(text); std::string part; | ||
| while (std::getline(ss, part, ',')) { | ||
| auto dash = part.find('-'); | ||
| unsigned first = std::stoul(part.substr(0, dash)); | ||
| unsigned last = dash == std::string::npos ? first : std::stoul(part.substr(dash + 1)); | ||
| for (unsigned i = first; i <= last; ++i) result.push_back(i); | ||
| } | ||
| return result; | ||
| } |
There was a problem hiding this comment.
The ParseList function is fragile and can easily throw unhandled exceptions (e.g., std::invalid_argument or std::out_of_range from std::stoul) if the input string contains trailing commas, spaces, or unexpected characters. Since this parses sysfs files which can vary across environments, we should wrap the parsing logic in a try-catch block and handle empty parts robustly to prevent crashing the simulator.
static std::vector<unsigned> ParseList(const std::string& text) {
std::vector<unsigned> result;
std::stringstream ss(text); std::string part;
try {
while (std::getline(ss, part, ',')) {
if (part.empty()) continue;
auto dash = part.find('-');
unsigned first = std::stoul(part.substr(0, dash));
unsigned last = dash == std::string::npos ? first : std::stoul(part.substr(dash + 1));
for (unsigned i = first; i <= last; ++i) result.push_back(i);
}
} catch (...) {
result.clear();
}
return result;
}| unsigned level = std::stoul(level_text); | ||
| unsigned cache_id = cache; | ||
| try { cache_id = std::stoul(Read(cache_root + "/id")); } catch (...) {} | ||
| auto shared = ParseList(shared_text); | ||
| auto& groups = t.nodes[node_id].caches; | ||
| auto it = std::find_if(groups.begin(), groups.end(), [&](const CacheGroup& group) { | ||
| return group.level == level && group.id == cache_id; | ||
| }); | ||
| uint64_t bytes = 0; | ||
| try { bytes = ParseSize(Read(cache_root + "/size")); } catch (...) {} | ||
| if (it == groups.end()) groups.push_back({level, cache_id, shared, bytes}); |
There was a problem hiding this comment.
Parsing of cache levels and shared CPU lists can throw unhandled exceptions if the sysfs files contain unexpected data or are partially unreadable (e.g., in restricted container environments). Wrapping the parsing of each cache index in a try-catch block ensures that a single malformed or unreadable cache file does not crash the entire simulator.
try {
unsigned level = std::stoul(level_text);
unsigned cache_id = cache;
try { cache_id = std::stoul(Read(cache_root + "/id")); } catch (...) {}
auto shared = ParseList(shared_text);
auto& groups = t.nodes[node_id].caches;
auto it = std::find_if(groups.begin(), groups.end(), [&](const CacheGroup& group) {
return group.level == level && group.id == cache_id;
});
uint64_t bytes = 0;
try { bytes = ParseSize(Read(cache_root + "/size")); } catch (...) {}
if (it == groups.end()) groups.push_back({level, cache_id, shared, bytes});
} catch (...) {}References
- Validate all user and file-based data to guard against security vulnerabilities. (link)
2 participants
Summary
Adds an opt-in Linux tiled runner while leaving qsim_base unchanged.
Implemented:
Validation
make[1]: Entering directory '/home/louis/backup/sources/qsim/apps'
g++ -o ./qsim_tiled.x qsim_tiled.cc -std=c++17 -O3 -flto=auto -fopenmp -march=native
make[1]: Leaving directory '/home/louis/backup/sources/qsim/apps'
git submodule update --init --recursive googletest
mkdir -p /home/louis/backup/sources/qsim/tests/googletest/googletest/build
cd /home/louis/backup/sources/qsim/tests/googletest/googletest && cmake -B build -S ..
-- Configuring done
-- Generating done
-- Build files have been written to: /home/louis/backup/sources/qsim/tests/googletest/googletest/build
cd /home/louis/backup/sources/qsim/tests/googletest/googletest/build && make
make[1]: Entering directory '/home/louis/backup/sources/qsim/tests/googletest/googletest/build'
make[2]: Entering directory '/home/louis/backup/sources/qsim/tests/googletest/googletest/build'
make[3]: Entering directory '/home/louis/backup/sources/qsim/tests/googletest/googletest/build'
Consolidate compiler generated dependencies of target gtest
make[3]: Leaving directory '/home/louis/backup/sources/qsim/tests/googletest/googletest/build'
make[3]: Entering directory '/home/louis/backup/sources/qsim/tests/googletest/googletest/build'
[ 12%] Building CXX object googletest/CMakeFiles/gtest.dir/src/gtest-all.cc.o
[ 25%] Linking CXX static library ../lib/libgtest.a
make[3]: Leaving directory '/home/louis/backup/sources/qsim/tests/googletest/googletest/build'
[ 25%] Built target gtest
make[3]: Entering directory '/home/louis/backup/sources/qsim/tests/googletest/googletest/build'
Consolidate compiler generated dependencies of target gmock
make[3]: Leaving directory '/home/louis/backup/sources/qsim/tests/googletest/googletest/build'
make[3]: Entering directory '/home/louis/backup/sources/qsim/tests/googletest/googletest/build'
[ 37%] Building CXX object googlemock/CMakeFiles/gmock.dir/src/gmock-all.cc.o
[ 50%] Linking CXX static library ../lib/libgmock.a
make[3]: Leaving directory '/home/louis/backup/sources/qsim/tests/googletest/googletest/build'
[ 50%] Built target gmock
make[3]: Entering directory '/home/louis/backup/sources/qsim/tests/googletest/googletest/build'
Consolidate compiler generated dependencies of target gmock_main
make[3]: Leaving directory '/home/louis/backup/sources/qsim/tests/googletest/googletest/build'
make[3]: Entering directory '/home/louis/backup/sources/qsim/tests/googletest/googletest/build'
[ 62%] Building CXX object googlemock/CMakeFiles/gmock_main.dir/src/gmock_main.cc.o
[ 75%] Linking CXX static library ../lib/libgmock_main.a
make[3]: Leaving directory '/home/louis/backup/sources/qsim/tests/googletest/googletest/build'
[ 75%] Built target gmock_main
make[3]: Entering directory '/home/louis/backup/sources/qsim/tests/googletest/googletest/build'
Consolidate compiler generated dependencies of target gtest_main
make[3]: Leaving directory '/home/louis/backup/sources/qsim/tests/googletest/googletest/build'
make[3]: Entering directory '/home/louis/backup/sources/qsim/tests/googletest/googletest/build'
[ 87%] Building CXX object googletest/CMakeFiles/gtest_main.dir/src/gtest_main.cc.o
[100%] Linking CXX static library ../lib/libgtest_main.a
make[3]: Leaving directory '/home/louis/backup/sources/qsim/tests/googletest/googletest/build'
[100%] Built target gtest_main
make[2]: Leaving directory '/home/louis/backup/sources/qsim/tests/googletest/googletest/build'
make[1]: Leaving directory '/home/louis/backup/sources/qsim/tests/googletest/googletest/build'
g++ -o ./tiled_runner_test.x tiled_runner_test.cc -I/home/louis/backup/sources/qsim/tests/googletest/googletest/include -L/home/louis/backup/sources/qsim/tests/googletest/googletest/build/lib -lgtest -msse4 -mavx2 -mfma -mbmi2
make: Leaving directory '/home/louis/backup/sources/qsim/tests'
[==========] Running 3 tests from 1 test suite.
[----------] Global test environment set-up.
[----------] 3 tests from TiledRunnerTest
[ RUN ] TiledRunnerTest.RemappingMatchesBaseline
[ OK ] TiledRunnerTest.RemappingMatchesBaseline (5 ms)
[ RUN ] TiledRunnerTest.MeasurementMatchesBaseline
[ OK ] TiledRunnerTest.MeasurementMatchesBaseline (1 ms)
[ RUN ] TiledRunnerTest.TileSizesAndSchedulesMatchBaseline
[ OK ] TiledRunnerTest.TileSizesAndSchedulesMatchBaseline (50 ms)
[----------] 3 tests from TiledRunnerTest (58 ms total)
[----------] Global test environment tear-down
[==========] 3 tests from 1 test suite ran. (58 ms total)
[ PASSED ] 3 tests.
Executed 0 out of 1 test: 1 test passes.
All tiled tests pass, including remapping, measurement, tile-size variation, and all scheduler modes.
Initial performance
On an Intel i7-7700K, q24 native baseline at f=3 was about 1.29s; the tiled runner at L=18 and f=3 was about 1.24-1.27s, with matching amplitudes within float tolerance and norm 0.99999624. The host has one NUMA node, so NUMA effects are not measurable here. Full q30 d=30 and multi-architecture comparisons remain for the review benchmark matrix.
This is an opt-in runner for team testing; it does not replace the default simulator.