From 154d33369157e9483b7411fb616bf724e092c7f4 Mon Sep 17 00:00:00 2001 From: Aaron Miller Date: Sat, 15 Aug 2026 12:35:00 +0100 Subject: [PATCH 01/10] =?UTF-8?q?fix(partition):=20=F0=9F=90=9B=20place=20?= =?UTF-8?q?partitions=20inside=20the=20cgroup=20the=20batch=20system=20car?= =?UTF-8?q?ved?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Placement must not divide an already-divided machine. `enumerate_physical_cores` reports only the cores inside the calling thread's affinity mask. When a launcher has already given each co-located rank its own disjoint slice -- `srun --cpu-bind=cores`, or any cgroup-confined launch -- that slice IS the rank's share. Passing the node-wide ranks-per-node through as `group_count` then asks for group_count x n cores out of a list that only ever held n, `placement_order` correctly refuses, and every rank silently runs UNPLACED. The two-level barrier loses its domains at the same time, because `cpuset_domains` derives them from the placement. Measured on 8 ranks x 16 partitions: 437 us/sync unplaced against 15.5 us/sync placed. `PartitionGroup` therefore allgathers the masks over its node-local communicator and `classify_node_mask` MEASURES disjointness; a `NodeMask::PerRank` result collapses `group_count` to 1. Mask width cannot substitute for that measurement: "8 ranks holding 16 cores each" and "8 ranks sharing one 16-core mask" both leave a rank seeing 16 of 128, and they need opposite placement. Collapsing in the wrong direction pins every co-located rank to the SAME cores, so `Shared` is the default and the safe error. This regressed once already, when topology discovery was rewritten onto hwloc, because the guard lives in the placement policy rather than in discovery -- any rework of that layer has to re-check it. `cpu_topology_policy_per_rank_slice_starves_without_collapse` pins the mechanism without needing live hardware, which is what makes it a regression test rather than a machine-specific one. Assisted-by: ClaudeCode:claude-opus-5 --- AGENTS.md | 15 +++ cpp/monoprop/detail/partition/CpuTopology.cpp | 92 ++++++++++++++++++- cpp/monoprop/detail/partition/CpuTopology.h | 46 +++++++++- .../detail/partition/PartitionGroup.h | 66 +++++++++++-- cpp/tests/cpu_topology_tests.cpp | 40 +++++++- 5 files changed, 244 insertions(+), 15 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 35bcd09e..a9765033 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -104,6 +104,21 @@ Key files: for the mutating/collecting paths, which run on the partitions' own pinned masters; `sum_partitions_`, `fold_partitions_`, `first_partition_` for reads off quiescent partitions) rather than hand-rolling a `run_on_all` loop — the declarations record which helper is legal where. +- **Placement must not divide an already-divided machine.** `enumerate_physical_cores` reports only cores + inside the calling thread's affinity mask, so when a launcher has given each co-located rank its own + disjoint slice (`srun --cpu-bind=cores`), the slice *is* the rank's share. Passing the node-wide + ranks-per-node through as `group_count` then asks for `group_count × n` cores out of a list that only + held `n`, `placement_order` correctly refuses, and every rank silently runs unpinned — which also costs + the two-level barrier its domains, because `cpuset_domains` derives them from the placement. Measured on + Deucalion at 8 ranks × 16 partitions: 437 µs/sync against 15.5 µs/sync placed. `PartitionGroup` therefore + allgathers the masks over its node-local communicator and `classify_node_mask` **measures** disjointness; + a `NodeMask::PerRank` result collapses `group_count` to 1. Mask *width* cannot substitute for this — "8 + ranks holding 16 cores each" and "8 ranks sharing one 16-core mask" both leave a rank seeing 16 of 128, + and they need opposite placement. Collapse in the wrong direction and every co-located rank pins to the + *same* cores, so `Shared` is the default and the safe error. This regressed once already, when topology + discovery was rewritten onto hwloc, because the guard lives in the placement policy rather than in + discovery: any rework of that layer must re-check it. `cpu_topology_policy_per_rank_slice_starves_without_collapse` + pins the mechanism without needing live hardware. ### Environment Management diff --git a/cpp/monoprop/detail/partition/CpuTopology.cpp b/cpp/monoprop/detail/partition/CpuTopology.cpp index dd019d8e..e5ef0120 100644 --- a/cpp/monoprop/detail/partition/CpuTopology.cpp +++ b/cpp/monoprop/detail/partition/CpuTopology.cpp @@ -80,6 +80,25 @@ auto effective_allowed_cpuset(hwloc_topology_t topo) -> hwloc_cpuset_t { return hwloc_bitmap_dup(hwloc_topology_get_allowed_cpuset(topo)); } +// True when this process may use strictly fewer PUs than the machine has, i.e. something outside +// the process -- a Slurm cgroup, a taskset, an MPI launcher's binding -- has already carved out a +// private share. This is the ONLY way to distinguish "our mask is small because it is our slice" +// from "our mask is the whole node and the caller asked for more cores than exist"; a core-count +// comparison cannot, and conflating the two either pins nothing (former) or double-books cores on +// co-located ranks (latter). +auto topo_is_cpu_confined(hwloc_topology_t topo) -> bool { + const hwloc_cpuset_t allowed = effective_allowed_cpuset(topo); + if (!allowed) { + return false; + } + const hwloc_const_cpuset_t machine = hwloc_topology_get_allowed_cpuset(topo); + /* Strict subset: included in the machine's set and not equal to it. */ + const bool confined = machine != nullptr && hwloc_bitmap_isincluded(allowed, machine) != 0 + && hwloc_bitmap_isequal(allowed, machine) == 0; + hwloc_bitmap_free(allowed); + return confined; +} + } // anonymous namespace /* ── topo_detail::placement_order ─────────────────────────────────────────── */ @@ -218,14 +237,83 @@ auto enumerate_physical_cores() -> std::vector { return cores; } +/* ── process_is_cpu_confined ───────────────────────────────────────────────── */ + +auto process_is_cpu_confined() -> bool { + const auto topo = get_topology(); + return topo != nullptr && topo_is_cpu_confined(topo); +} + +/* ── affinity_mask_words ───────────────────────────────────────────────────── */ + +auto affinity_mask_words(uint64_t *out, size_t nwords) -> bool { + if (out == nullptr || nwords == 0) { + return false; + } + std::fill_n(out, nwords, uint64_t{0}); + const auto topo = get_topology(); + if (!topo) { + return false; + } + const hwloc_cpuset_t allowed = effective_allowed_cpuset(topo); + if (!allowed) { + return false; + } + /* Anything above the window we can exchange is reported as "cannot classify" rather than + * silently truncated: a truncated mask could compare disjoint against a peer it overlaps. */ + const int last = hwloc_bitmap_last(allowed); + const bool representable = last >= 0 && static_cast(last) < nwords * 64; + if (representable) { + for (int pu = hwloc_bitmap_first(allowed); pu >= 0; pu = hwloc_bitmap_next(allowed, pu)) { + out[static_cast(pu) / 64] |= uint64_t{1} << (static_cast(pu) % 64); + } + } + hwloc_bitmap_free(allowed); + return representable; +} + /* ── partition_cpusets ─────────────────────────────────────────────────────── */ -auto partition_cpusets(size_t n, size_t group_index, size_t group_count) -> std::vector { +auto partition_cpusets(size_t n, size_t group_index, size_t group_count, bool mask_is_private) -> std::vector { if (!config::get().partition_pinning) { return {}; } const auto cores = enumerate_physical_cores(); - const auto order = topo_detail::placement_order(cores, n, group_index, group_count); + + /* Do not partition a node the batch system has already partitioned. + * + * enumerate_physical_cores() reports only the cores in THIS process's affinity mask. Under a + * resource manager that hands each rank its own cpuset -- Slurm with cgroups does, which is the + * configuration every benchmark here runs in -- that mask is already this rank's exclusive + * share, so `cores` IS our slice and not the node. Splitting it again by group_count asks for + * group_count x more cores than exist; placement_order correctly refuses and returns {}, so + * NOTHING is pinned, on every rank, silently -- pin_this_thread ignores bind errors by design. + * Measured signature: 8 ranks x 16 partitions with affinity_cpus=16 per rank gave + * distinct_pinned_cpus=0 and voided the run. + * + * The disjointness this function exists to guarantee still holds there: the cgroups are + * disjoint by construction, so placing within our own mask cannot collide with a co-located + * rank. When the mask is the whole node the condition is false and co-located ranks are + * separated here exactly as before. + * + * The discriminator CANNOT be a core count, and cannot be confinement either. + * `cores.size() < group_count * n` is equally the signature of a genuine oversubscription on an + * unconfined node. And "our mask is narrower than the machine" cannot tell "8 ranks holding 16 + * cores each" from "8 ranks SHARING one 16-core mask" -- both leave a rank seeing 16 of 128. + * Collapsing in the shared case points every co-located rank at the same cores, which is worse + * than not pinning: each rank's busy-polling collectives then starve the others' barrier spins. + * Only comparing the co-located ranks' masks answers it, so the caller establishes + * `mask_is_private` by allgathering them (PartitionGroup::discover_node_peers_) and we do not + * guess here. + * + * This is also a FALLBACK rather than a replacement: when the mask is wide enough to hold all + * group_count groups the normal split still runs and still separates co-located ranks. So the + * new arm can only ever turn "nothing pinned" into "something pinned"; it cannot take a working + * placement away. */ + auto order = topo_detail::placement_order(cores, n, group_index, group_count); + if (order.empty() && group_count > 1 && mask_is_private) { + order = topo_detail::placement_order(cores, n, /*group_index=*/0, /*group_count=*/1); + } std::vector sets(order.size()); for (size_t i = 0; i < order.size(); ++i) { diff --git a/cpp/monoprop/detail/partition/CpuTopology.h b/cpp/monoprop/detail/partition/CpuTopology.h index 1b60359e..1fe6f9c2 100644 --- a/cpp/monoprop/detail/partition/CpuTopology.h +++ b/cpp/monoprop/detail/partition/CpuTopology.h @@ -25,6 +25,7 @@ #pragma once #include +#include #include #include "monoprop/detail/EnvConfig.h" @@ -88,6 +89,34 @@ auto placement_order(const std::vector &cores, size_t n, size_t gr */ auto enumerate_physical_cores() -> std::vector; +/*! + * @brief Whether something outside this process has already restricted its usable CPUs. + * + * True when the calling thread's affinity mask is a STRICT subset of the machine's allowed cpuset, + * i.e. a Slurm cgroup, a taskset or an MPI launcher binding has restricted this process. + * + * @warning This is NOT sufficient to decide that the mask is this rank's PRIVATE share. Mask width + * cannot distinguish "8 ranks holding 16 cores each" from "8 ranks sharing one 16-core + * mask" -- both leave a rank seeing 16 of 128 CPUs. Only comparing the co-located ranks' + * masks answers that, which is why partition_cpusets() takes @c mask_is_private from the + * caller (PartitionGroup allgathers the masks over its node communicator) rather than + * inferring it here. + * + * @returns false when hwloc cannot load the topology or the affinity mask is the whole machine. + */ +auto process_is_cpu_confined() -> bool; + +/*! + * @brief This process's effective allowed cpuset, as a bit array of @p nwords 64-bit words. + * + * Exposed so callers can exchange masks between co-located ranks and test them for pairwise + * disjointness -- the only sound basis for deciding whether each rank owns a private share. + * + * @returns false (leaving @p out zeroed) when hwloc cannot load the topology or the mask needs + * more than @p nwords words, which is treated as "cannot classify" and hence not private. + */ +auto affinity_mask_words(uint64_t *out, size_t nwords) -> bool; + /*! * @brief Build placement tokens for one MPI rank's partitions. * @@ -98,10 +127,23 @@ auto enumerate_physical_cores() -> std::vector; * @param n Number of partitions to place. * @param group_index This rank's 0-based index among the co-located ranks on the host. * @param group_count Total number of co-located ranks on the host. + * @param mask_is_private True only when the caller has established that the co-located ranks' + * affinity masks are pairwise DISJOINT, so this rank's mask is its own share of + * the node. See PartitionGroup::discover_node_peers_(), which allgathers the + * masks over its node communicator to decide. * @returns Vector of @p n CpuSet tokens, or empty when @c monoprop_PARTITION_PINNING is disabled, - * hwloc is unavailable, or the host cannot provide @p group_count × @p n distinct cores. + * hwloc is unavailable, or not even @p n distinct cores are visible to this process. + * + * @note With @p mask_is_private, @p group_index / @p group_count are ignored once the normal split + * has failed: the batch system has already partitioned the node (Slurm with per-rank cgroups), + * so subdividing our own share a second time asks for @p group_count × more cores than exist + * and places nothing at all. Disjointness is what makes ignoring them safe. Without it, a set + * of ranks SHARING one narrow mask would every one of them collapse onto the same cores -- + * worse than not pinning, since each rank's busy-polling collectives would then starve the + * others' barrier spins. */ -auto partition_cpusets(size_t n, size_t group_index = 0, size_t group_count = 1) -> std::vector; +auto partition_cpusets(size_t n, size_t group_index = 0, size_t group_count = 1, bool mask_is_private = false) + -> std::vector; /*! * @brief Bind the calling thread to the PU identified by @p set. diff --git a/cpp/monoprop/detail/partition/PartitionGroup.h b/cpp/monoprop/detail/partition/PartitionGroup.h index fc3f64fd..7bf76ad6 100644 --- a/cpp/monoprop/detail/partition/PartitionGroup.h +++ b/cpp/monoprop/detail/partition/PartitionGroup.h @@ -14,8 +14,10 @@ #pragma once +#include #include #include +#include #include #include #include @@ -59,7 +61,7 @@ class PartitionGroup { errs_(static_cast(n_partitions)) { make_transport_(); discover_node_peers_(); - cpusets_ = topo_partition_cpusets(n_, node_rank_, node_size_); + cpusets_ = topo_partition_cpusets(n_, node_rank_, node_size_, node_mask_private_); start_masters_(); // The masters are already running, so a ctor throw must not escape: ~PartitionGroup would never run, // and destroying joinable threads during unwinding calls std::terminate. @@ -79,10 +81,11 @@ class PartitionGroup { parent_(src.parent_), node_rank_(src.node_rank_), node_size_(src.node_size_), + node_mask_private_(src.node_mask_private_), partitions_(static_cast(src.n_)), errs_(static_cast(src.n_)) { make_transport_(); - cpusets_ = topo_partition_cpusets(n_, node_rank_, node_size_); + cpusets_ = topo_partition_cpusets(n_, node_rank_, node_size_, node_mask_private_); start_masters_(); try { // see the primary ctor: a throw past live masters would std::terminate run_on_all([&](int r) { @@ -146,11 +149,12 @@ class PartitionGroup { } // Free-function wrapper so the header compiles on non-Linux (where partition_cpusets returns {}). - static auto topo_partition_cpusets(int n, int group_index, int group_count) + static auto topo_partition_cpusets(int n, int group_index, int group_count, bool mask_is_private) -> std::vector { return monoprop::detail::partition::partition_cpusets(static_cast(n), static_cast(group_index), - static_cast(group_count)); + static_cast(group_count), + mask_is_private); } // Under an MPI parent, find how many ranks share this host and which we are, so each co-located rank @@ -162,11 +166,52 @@ class PartitionGroup { MPI_Comm_split_type(parent_.mpi, MPI_COMM_TYPE_SHARED, 0, MPI_INFO_NULL, &node); MPI_Comm_rank(node, &node_rank_); MPI_Comm_size(node, &node_size_); + classify_node_masks_(node); MPI_Comm_free(&node); } #endif } +#ifdef monoprop_ENABLE_MPI + // Decide whether each co-located rank owns a PRIVATE share of the node's CPUs, by exchanging the + // actual affinity masks and testing them for pairwise disjointness. + // + // This cannot be inferred locally. A rank seeing 16 of the machine's 128 CPUs is equally "Slurm + // gave me my own 16" and "eight of us share the same 16", and the two want opposite placements: + // the first should fill its own mask (otherwise nothing is pinned at all), the second must not + // (or every rank lands on identical cores and their busy-polling collectives starve each other). + // Only the peers' masks separate them, and this is the one place that already has a node-local + // communicator open. One allgather of 512 B per rank, once per group construction. + auto classify_node_masks_(MPI_Comm node) -> void { + node_mask_private_ = false; + if (node_size_ <= 1) { + return; // nobody to collide with; the normal split already handles group_count == 1 + } + std::array mine{}; + // A mask we cannot represent is "cannot classify", never "private": the fallback stays off. + const bool ok = monoprop::detail::partition::affinity_mask_words(mine.data(), kMaskWords); + std::vector all(kMaskWords * static_cast(node_size_), 0); + MPI_Allgather(mine.data(), kMaskWords, MPI_UINT64_T, all.data(), kMaskWords, MPI_UINT64_T, node); + + int local_ok = ok ? 1 : 0; + int all_ok = 0; + MPI_Allreduce(&local_ok, &all_ok, 1, MPI_INT, MPI_MIN, node); + if (all_ok == 0) { + return; + } + for (size_t a = 0; a < static_cast(node_size_); ++a) { + for (size_t b = a + 1; b < static_cast(node_size_); ++b) { + for (size_t w = 0; w < kMaskWords; ++w) { + if ((all[a * kMaskWords + w] & all[b * kMaskWords + w]) != 0) { + return; // two peers share a CPU: not private + } + } + } + } + node_mask_private_ = true; + } +#endif + auto make_transport_() -> void { #ifdef monoprop_ENABLE_MPI if (parent_.kind == mpi::Comm::Kind::Mpi && mpi::size(parent_) > 1) { @@ -250,10 +295,15 @@ class PartitionGroup { } int n_; - mpi::Comm parent_; // enclosing communicator (size R) — decides the transport - int node_rank_ = 0; // this rank's index among the ranks sharing the host - int node_size_ = 1; // how many parent ranks share the host (1 unless MPI R>1) - std::unique_ptr shm_; // set iff R == 1 + mpi::Comm parent_; // enclosing communicator (size R) — decides the transport + int node_rank_ = 0; // this rank's index among the ranks sharing the host + int node_size_ = 1; // how many parent ranks share the host (1 unless MPI R>1) + // True only when the co-located ranks' affinity masks are pairwise disjoint, i.e. each rank owns + // its share of the node. Decided in classify_node_masks_(); copied, never re-derived, by the copy + // ctor, which has no communicator to allgather over. + bool node_mask_private_ = false; + static constexpr size_t kMaskWords = 64; // 4096 CPUs; wider masks are 'cannot classify' + std::unique_ptr shm_; // set iff R == 1 #ifdef monoprop_ENABLE_MPI std::unique_ptr hyb_; // set iff R > 1 #endif diff --git a/cpp/tests/cpu_topology_tests.cpp b/cpp/tests/cpu_topology_tests.cpp index 5cda9980..6c4e60bd 100644 --- a/cpp/tests/cpu_topology_tests.cpp +++ b/cpp/tests/cpu_topology_tests.cpp @@ -93,9 +93,43 @@ BOOST_AUTO_TEST_CASE(cpu_topology_place_co_located_ranks) { // invariant (one rank's busy-polling collectives cannot starve the other's barrier spins). BOOST_CHECK(rank0.front().pu != rank1.front().pu); - // Oversubscription: 2 ranks × cores.size() partitions > total physical cores. - const auto past_end = partition::partition_cpusets(/*n=*/cores.size(), /*group_index=*/1, /*group_count=*/2); - BOOST_CHECK(past_end.empty()); + // 2 ranks × cores.size() partitions asks for more cores than this process can see. What that + // MEANS depends on whether the co-located ranks' masks are disjoint, which no local test can + // answer, so partition_cpusets takes it as an argument and both arms are pinned here: + // + // shared mask → a genuine oversubscription. Refuse: placing anyway would hand both ranks the + // same cores, and each rank's busy-polling collectives would starve the other's + // barrier spins — worse than not pinning at all. + // private mask → the batch system already split the node (Slurm per-rank cgroups) and this is + // our own share. Refusing here is exactly what left every rank unpinned on the + // benchmark nodes, voiding four A/B jobs. Fill our mask. + // + // Passing the flag explicitly is what makes this deterministic on any machine; the earlier + // version branched on the ambient environment and so asserted whichever answer the host gave. + const auto shared_mask = partition::partition_cpusets(/*n=*/cores.size(), + /*group_index=*/1, + /*group_count=*/2, + /*mask_is_private=*/false); + BOOST_CHECK(shared_mask.empty()); + + const auto private_mask = partition::partition_cpusets(/*n=*/cores.size(), + /*group_index=*/1, + /*group_count=*/2, + /*mask_is_private=*/true); + BOOST_REQUIRE_EQUAL(private_mask.size(), cores.size()); + std::set placed; + for (const auto &set : private_mask) { + placed.insert(set.pu); + } + // Distinct PUs, and every one of them a core this process was actually granted. + BOOST_CHECK_EQUAL(placed.size(), cores.size()); + std::set visible; + for (const auto &core : cores) { + visible.insert(core.cpu); + } + for (const int pu : placed) { + BOOST_CHECK(visible.count(pu) == 1); + } } /* ── Policy unit tests (deterministic, no hwloc or live hardware) ─────────── */ From 010e5b483c78202a3f9373e2fe0bd2a1dd02d20f Mon Sep 17 00:00:00 2001 From: Aaron Miller Date: Mon, 17 Aug 2026 11:44:37 +0100 Subject: [PATCH 02/10] =?UTF-8?q?test(partition):=20=F0=9F=A7=AA=20make=20?= =?UTF-8?q?the=20cgroup-collapse=20rule=20reachable=20without=20live=20har?= =?UTF-8?q?dware?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The fix as committed added no hardware-free test case. It extends one live-topology case, which early-returns when fewer than two physical cores are visible or pinning is off -- so on a constrained runner the whole mechanism can go unexercised while the suite reports green. That matters more than usual here: this regressed once already, when topology discovery moved onto hwloc, because the guard lives in the placement policy rather than in discovery. So the rule becomes a free function. masks_are_pairwise_disjoint() is pure bit arithmetic over the array MPI_Allgather already leaves behind -- no communicator, no hwloc, no live hardware -- and classify_node_masks_ keeps only the exchange and the all_ok reduction. Six cases are ported from the competing implementation on perf/multinode-comm-scaling, which spelled the same mechanism as a NodeMask enum over a CpuMask POD; dropping that implementation in favour of this one would otherwise have dropped its tests with it. The empty-mask clause is folded into the predicate rather than left implicit. An all-zero mask is trivially disjoint from everything, so a bare disjointness test answers "private" for a rank that can see no CPU at all -- collapsing group_count in exactly the case where the caller knows least. Not private is the safe error. kMaskWords moves out of PartitionGroup and beside affinity_mask_words as kAffinityMaskWords: the two must agree on the width and a constant living next to only one of them can drift silently. process_is_cpu_confined() and its helper are deleted -- 40 lines of public-header API with no caller, no test, and a Doxygen block warning against its own use. Corrects the AGENTS.md paragraph, which described the OTHER branch's design: classify_node_mask, NodeMask::PerRank and Shared appear nowhere in this tree, and neither did the test it cited. Replaces the 437 -> 15.5 us/sync figure, which is a pair of range midpoints measured on the pre-hwloc tree with a different branch as the arm, with the placement counts the /proc probe takes on both arms (0 vs 16 threads pinned per rank), and records what the record actually says: at 99.4M terms this does not make the workload faster, and pinning did not reduce variance either. Assisted-by: ClaudeCode:claude-opus-5 --- AGENTS.md | 30 ++- cpp/monoprop/detail/partition/CpuTopology.cpp | 55 +++--- cpp/monoprop/detail/partition/CpuTopology.h | 43 ++-- .../detail/partition/PartitionGroup.h | 24 +-- cpp/tests/cpu_topology_tests.cpp | 185 ++++++++++++++++++ 5 files changed, 277 insertions(+), 60 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index a9765033..011d65d4 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -109,16 +109,26 @@ Key files: disjoint slice (`srun --cpu-bind=cores`), the slice *is* the rank's share. Passing the node-wide ranks-per-node through as `group_count` then asks for `group_count × n` cores out of a list that only held `n`, `placement_order` correctly refuses, and every rank silently runs unpinned — which also costs - the two-level barrier its domains, because `cpuset_domains` derives them from the placement. Measured on - Deucalion at 8 ranks × 16 partitions: 437 µs/sync against 15.5 µs/sync placed. `PartitionGroup` therefore - allgathers the masks over its node-local communicator and `classify_node_mask` **measures** disjointness; - a `NodeMask::PerRank` result collapses `group_count` to 1. Mask *width* cannot substitute for this — "8 - ranks holding 16 cores each" and "8 ranks sharing one 16-core mask" both leave a rank seeing 16 of 128, - and they need opposite placement. Collapse in the wrong direction and every co-located rank pins to the - *same* cores, so `Shared` is the default and the safe error. This regressed once already, when topology - discovery was rewritten onto hwloc, because the guard lives in the placement policy rather than in - discovery: any rework of that layer must re-check it. `cpu_topology_policy_per_rank_slice_starves_without_collapse` - pins the mechanism without needing live hardware. + every rank silently runs unpinned. Measured on Deucalion at 8 ranks × 16 partitions: `--cpu-bind=cores` + and `--cpu-bind=threads` both leave **0** threads pinned per rank, against 16 under `--cpu-bind=none`. + `PartitionGroup::classify_node_masks_` therefore allgathers the raw affinity masks over its node-local + communicator and `masks_are_pairwise_disjoint` **measures** disjointness; a private result passes + `mask_is_private` to `partition_cpusets`, which collapses `group_count` to 1. Mask *width* cannot + substitute for this — "8 ranks holding 16 cores each" and "8 ranks sharing one 16-core mask" both leave a + rank seeing 16 of 128, and they need opposite placement. Collapse in the wrong direction and every + co-located rank pins to the *same* cores, so **not private** is the default and the safe error; an empty + mask is not private either, since an all-zero mask is trivially disjoint from everything. This regressed + once already, when topology discovery was rewritten onto hwloc, because the guard lives in the placement + policy rather than in discovery: any rework of that layer must re-check it. + `cpu_topology_policy_per_rank_slice_starves_without_collapse` and `cpu_topology_masks_*` pin the mechanism + without needing live hardware. +- **This is a correctness-of-measurement fix before it is a speed fix.** Unplaced ranks read as a slow code + path and have voided four A/B jobs, so every measurement is more trustworthy with it in. But at 99.4M + terms, 8×16, the branch carrying it is flat or worse on three of four operations — restoring placement + does not make that size faster. Do not quote a µs/sync figure across scales: `barrier_per_sync_us` is + peers idling while partition 0 is inside MPI, so it grows with the work waited *for* (15–22 µs at 0.8M + terms, 274–604 µs at 99M) and two such numbers from different sizes share no axis. Nor does pinning + reduce run-to-run variance: re-measured with 16 threads pinned per rank, spreads went 1.6–2.6× → 1.3–4.6×. ### Environment Management diff --git a/cpp/monoprop/detail/partition/CpuTopology.cpp b/cpp/monoprop/detail/partition/CpuTopology.cpp index e5ef0120..6e27558a 100644 --- a/cpp/monoprop/detail/partition/CpuTopology.cpp +++ b/cpp/monoprop/detail/partition/CpuTopology.cpp @@ -80,25 +80,6 @@ auto effective_allowed_cpuset(hwloc_topology_t topo) -> hwloc_cpuset_t { return hwloc_bitmap_dup(hwloc_topology_get_allowed_cpuset(topo)); } -// True when this process may use strictly fewer PUs than the machine has, i.e. something outside -// the process -- a Slurm cgroup, a taskset, an MPI launcher's binding -- has already carved out a -// private share. This is the ONLY way to distinguish "our mask is small because it is our slice" -// from "our mask is the whole node and the caller asked for more cores than exist"; a core-count -// comparison cannot, and conflating the two either pins nothing (former) or double-books cores on -// co-located ranks (latter). -auto topo_is_cpu_confined(hwloc_topology_t topo) -> bool { - const hwloc_cpuset_t allowed = effective_allowed_cpuset(topo); - if (!allowed) { - return false; - } - const hwloc_const_cpuset_t machine = hwloc_topology_get_allowed_cpuset(topo); - /* Strict subset: included in the machine's set and not equal to it. */ - const bool confined = machine != nullptr && hwloc_bitmap_isincluded(allowed, machine) != 0 - && hwloc_bitmap_isequal(allowed, machine) == 0; - hwloc_bitmap_free(allowed); - return confined; -} - } // anonymous namespace /* ── topo_detail::placement_order ─────────────────────────────────────────── */ @@ -237,13 +218,6 @@ auto enumerate_physical_cores() -> std::vector { return cores; } -/* ── process_is_cpu_confined ───────────────────────────────────────────────── */ - -auto process_is_cpu_confined() -> bool { - const auto topo = get_topology(); - return topo != nullptr && topo_is_cpu_confined(topo); -} - /* ── affinity_mask_words ───────────────────────────────────────────────────── */ auto affinity_mask_words(uint64_t *out, size_t nwords) -> bool { @@ -272,6 +246,35 @@ auto affinity_mask_words(uint64_t *out, size_t nwords) -> bool { return representable; } +/* ── masks_are_pairwise_disjoint ───────────────────────────────────────────── */ + +auto masks_are_pairwise_disjoint(const uint64_t *masks, size_t n, size_t words) -> bool { + if (masks == nullptr || words == 0 || n < 2) { + return false; + } + /* An all-zero mask is disjoint from everything, so a bare disjointness test would answer + * "private" for a rank that can see no CPU at all. Shared is the safe error. */ + for (size_t r = 0; r < n; ++r) { + bool any = false; + for (size_t w = 0; w < words && !any; ++w) { + any = masks[(r * words) + w] != 0; + } + if (!any) { + return false; + } + } + for (size_t a = 0; a < n; ++a) { + for (size_t b = a + 1; b < n; ++b) { + for (size_t w = 0; w < words; ++w) { + if ((masks[(a * words) + w] & masks[(b * words) + w]) != 0) { + return false; // two peers share a CPU: not private + } + } + } + } + return true; +} + /* ── partition_cpusets ─────────────────────────────────────────────────────── */ auto partition_cpusets(size_t n, size_t group_index, size_t group_count, bool mask_is_private) -> std::vector { diff --git a/cpp/monoprop/detail/partition/CpuTopology.h b/cpp/monoprop/detail/partition/CpuTopology.h index 1fe6f9c2..7522ed0d 100644 --- a/cpp/monoprop/detail/partition/CpuTopology.h +++ b/cpp/monoprop/detail/partition/CpuTopology.h @@ -90,21 +90,16 @@ auto placement_order(const std::vector &cores, size_t n, size_t gr auto enumerate_physical_cores() -> std::vector; /*! - * @brief Whether something outside this process has already restricted its usable CPUs. + * @brief Number of 64-bit words used to exchange an affinity mask between co-located ranks. * - * True when the calling thread's affinity mask is a STRICT subset of the machine's allowed cpuset, - * i.e. a Slurm cgroup, a taskset or an MPI launcher binding has restricted this process. + * 64 words is 4096 CPUs. A mask needing more is reported as "cannot classify" by + * affinity_mask_words(), never as private, so the fallback stays off and the old unplaced + * behaviour returns rather than a wrong placement. * - * @warning This is NOT sufficient to decide that the mask is this rank's PRIVATE share. Mask width - * cannot distinguish "8 ranks holding 16 cores each" from "8 ranks sharing one 16-core - * mask" -- both leave a rank seeing 16 of 128 CPUs. Only comparing the co-located ranks' - * masks answers that, which is why partition_cpusets() takes @c mask_is_private from the - * caller (PartitionGroup allgathers the masks over its node communicator) rather than - * inferring it here. - * - * @returns false when hwloc cannot load the topology or the affinity mask is the whole machine. + * Declared here, beside the two functions that must agree on it, rather than privately in the + * caller: a width that lives next to only one of the two can drift silently. */ -auto process_is_cpu_confined() -> bool; +inline constexpr size_t kAffinityMaskWords = 64; /*! * @brief This process's effective allowed cpuset, as a bit array of @p nwords 64-bit words. @@ -117,6 +112,30 @@ auto process_is_cpu_confined() -> bool; */ auto affinity_mask_words(uint64_t *out, size_t nwords) -> bool; +/*! + * @brief Whether @p n masks of @p words words each, laid end to end in @p masks, are pairwise + * disjoint AND none of them is empty. + * + * This is the whole classification rule, and it is pure bit arithmetic over an already-gathered + * array: no communicator, no hwloc, no live hardware. It is a free function precisely so it can be + * tested, because the mechanism it implements has regressed once already -- when topology discovery + * moved onto hwloc -- and the guard lives in the placement policy rather than in discovery. + * + * Mask WIDTH cannot substitute for this. "8 ranks holding 16 cores each" and "8 ranks sharing one + * 16-core mask" both leave a rank seeing 16 of 128 CPUs, and they need opposite placements: the + * first should fill its own mask or nothing is pinned at all, the second must not or every rank + * lands on identical cores and their busy-polling collectives starve each other. + * + * The empty-mask clause is not incidental. An all-zero mask is trivially disjoint from everything, + * so a bare disjointness test would answer "private" for a rank that can see no CPU at all -- + * collapsing group_count in exactly the case where the caller knows least. Shared is the safe + * error, so an empty mask is not private. + * + * @returns false when @p n < 2 (nobody to collide with, and the normal split already handles it), + * when any mask is empty, or when any two masks share a bit. + */ +[[nodiscard]] auto masks_are_pairwise_disjoint(const uint64_t *masks, size_t n, size_t words) -> bool; + /*! * @brief Build placement tokens for one MPI rank's partitions. * diff --git a/cpp/monoprop/detail/partition/PartitionGroup.h b/cpp/monoprop/detail/partition/PartitionGroup.h index 7bf76ad6..2431d247 100644 --- a/cpp/monoprop/detail/partition/PartitionGroup.h +++ b/cpp/monoprop/detail/partition/PartitionGroup.h @@ -187,28 +187,29 @@ class PartitionGroup { if (node_size_ <= 1) { return; // nobody to collide with; the normal split already handles group_count == 1 } + constexpr size_t kMaskWords = monoprop::detail::partition::kAffinityMaskWords; std::array mine{}; // A mask we cannot represent is "cannot classify", never "private": the fallback stays off. const bool ok = monoprop::detail::partition::affinity_mask_words(mine.data(), kMaskWords); std::vector all(kMaskWords * static_cast(node_size_), 0); MPI_Allgather(mine.data(), kMaskWords, MPI_UINT64_T, all.data(), kMaskWords, MPI_UINT64_T, node); + // Reduced before the verdict, not after: `ok` is per-rank, and a rank whose mask is too + // wide to represent must make EVERY peer decide "not private". A verdict computed from a + // buffer some ranks filled and others did not would differ between ranks, and this one + // feeds partition_cpusets on all of them. int local_ok = ok ? 1 : 0; int all_ok = 0; MPI_Allreduce(&local_ok, &all_ok, 1, MPI_INT, MPI_MIN, node); if (all_ok == 0) { return; } - for (size_t a = 0; a < static_cast(node_size_); ++a) { - for (size_t b = a + 1; b < static_cast(node_size_); ++b) { - for (size_t w = 0; w < kMaskWords; ++w) { - if ((all[a * kMaskWords + w] & all[b * kMaskWords + w]) != 0) { - return; // two peers share a CPU: not private - } - } - } - } - node_mask_private_ = true; + // The rule itself is a free function over the gathered array -- pure bit arithmetic, no + // communicator, no hwloc -- so it is reachable from a unit test on any machine. What is + // left here is only the exchange. + node_mask_private_ = monoprop::detail::partition::masks_are_pairwise_disjoint(all.data(), + static_cast(node_size_), + kMaskWords); } #endif @@ -302,8 +303,7 @@ class PartitionGroup { // its share of the node. Decided in classify_node_masks_(); copied, never re-derived, by the copy // ctor, which has no communicator to allgather over. bool node_mask_private_ = false; - static constexpr size_t kMaskWords = 64; // 4096 CPUs; wider masks are 'cannot classify' - std::unique_ptr shm_; // set iff R == 1 + std::unique_ptr shm_; // set iff R == 1 #ifdef monoprop_ENABLE_MPI std::unique_ptr hyb_; // set iff R > 1 #endif diff --git a/cpp/tests/cpu_topology_tests.cpp b/cpp/tests/cpu_topology_tests.cpp index 6c4e60bd..3a73bf45 100644 --- a/cpp/tests/cpu_topology_tests.cpp +++ b/cpp/tests/cpu_topology_tests.cpp @@ -25,6 +25,7 @@ #include #include +#include #include #include @@ -207,3 +208,187 @@ BOOST_AUTO_TEST_CASE(cpu_topology_policy_uneven_domains) { BOOST_CHECK_EQUAL(order[1], 2); BOOST_CHECK_EQUAL(order[2], 4); } + +/* ── The cgroup-placement classification ────────────────────────────────────── + * + * These are ported from the competing implementation of this fix on + * perf/multinode-comm-scaling, which spelled the rule as a `NodeMask` enum over a `CpuMask` POD. + * The mechanism is the same; only the surface differs, so the cases transfer with the enum + * replaced by `masks_are_pairwise_disjoint` over a flat array and by `partition_cpusets`' + * `mask_is_private` flag. + * + * They are worth porting because this branch adds NO hardware-free coverage of its own -- it + * extends one live-topology case, which early-returns on a runner with fewer than two visible + * cores, so the whole fix can be unexercised while the suite reports green. And the mechanism + * has regressed once already, when topology discovery moved onto hwloc, because the guard lives + * in the placement policy rather than in discovery. + */ + +BOOST_AUTO_TEST_CASE(cpu_topology_policy_per_rank_slice_starves_without_collapse) { + // One rank's slice under `srun --cpu-bind=cores`: 2 cores of a 16-core host, one L3 domain. + const std::vector slice = {{6, 0}, {7, 0}}; + + // Told the node-wide truth (8 ranks x 2 partitions), the request cannot be met from a slice. + BOOST_CHECK(placement_order(slice, 2, /*group_index=*/3, /*group_count=*/8).empty()); + + // Collapsed to a single group -- what mask_is_private does -- the same slice places fully. + const auto collapsed = placement_order(slice, 2, /*group_index=*/0, /*group_count=*/1); + BOOST_REQUIRE_EQUAL(collapsed.size(), 2u); + BOOST_CHECK_EQUAL(collapsed[0], 6); + BOOST_CHECK_EQUAL(collapsed[1], 7); +} + +namespace { + +/* Build the flat [n * words] array masks_are_pairwise_disjoint reads, from a list of PU-index + * lists -- the shape MPI_Allgather leaves behind. */ +auto packed_masks(const std::vector> &pus, size_t words) -> std::vector { + std::vector out(pus.size() * words, 0); + for (size_t r = 0; r < pus.size(); ++r) { + for (const size_t pu : pus[r]) { + out[(r * words) + (pu / 64)] |= uint64_t{1} << (pu % 64); + } + } + return out; +} + +} // namespace + +BOOST_AUTO_TEST_CASE(cpu_topology_masks_disjoint_vs_identical) { + constexpr size_t kWords = partition::kAffinityMaskWords; + + const auto disjoint = packed_masks({{0, 1}, {2, 3}}, kWords); + BOOST_CHECK(partition::masks_are_pairwise_disjoint(disjoint.data(), 2, kWords)); + + const auto identical = packed_masks({{0, 1}, {0, 1}}, kWords); + BOOST_CHECK(!partition::masks_are_pairwise_disjoint(identical.data(), 2, kWords)); + + // Partial overlap is pathological; "not private" is the conservative answer, because dividing + // never double-books a core within a rank whereas collapsing points every rank at the same ones. + const auto partial = packed_masks({{0, 1}, {1, 2}}, kWords); + BOOST_CHECK(!partition::masks_are_pairwise_disjoint(partial.data(), 2, kWords)); + + // An unreadable mask arrives empty and must not be read as "disjoint from everything". + const auto with_empty = packed_masks({{0, 1}, {}}, kWords); + BOOST_CHECK(!partition::masks_are_pairwise_disjoint(with_empty.data(), 2, kWords)); + + // A lone rank has nobody to be disjoint from, and the normal split already handles it. + const auto lone = packed_masks({{0, 1}}, kWords); + BOOST_CHECK(!partition::masks_are_pairwise_disjoint(lone.data(), 1, kWords)); + + // More than two peers: one overlapping pair anywhere is enough to make the node shared. + const auto four_ok = packed_masks({{0}, {1}, {2}, {3}}, kWords); + BOOST_CHECK(partition::masks_are_pairwise_disjoint(four_ok.data(), 4, kWords)); + const auto four_bad = packed_masks({{0}, {1}, {2}, {1}}, kWords); + BOOST_CHECK(!partition::masks_are_pairwise_disjoint(four_bad.data(), 4, kWords)); +} + +BOOST_AUTO_TEST_CASE(cpu_topology_masks_span_word_boundaries) { + constexpr size_t kWords = partition::kAffinityMaskWords; + + // Bits in different 64-bit words: a per-word comparison that forgot to loop would call these + // disjoint by luck on the first word and overlapping is what must be detected on the third. + const auto low_high = packed_masks({{5}, {200}}, kWords); + BOOST_CHECK(partition::masks_are_pairwise_disjoint(low_high.data(), 2, kWords)); + + const auto both_high = packed_masks({{200}, {200}}, kWords); + BOOST_CHECK(!partition::masks_are_pairwise_disjoint(both_high.data(), 2, kWords)); + + // Overlap in a later word than the first difference. + const auto late_overlap = packed_masks({{1, 3000}, {2, 3000}}, kWords); + BOOST_CHECK(!partition::masks_are_pairwise_disjoint(late_overlap.data(), 2, kWords)); + + // The truncation guard itself -- a PU index past the exchanged window making + // affinity_mask_words report "cannot classify" -- needs a live hwloc mask and is not + // reachable from here. It is covered only by the all_ok reduction in + // PartitionGroup::classify_node_masks_, which needs a communicator. +} + +BOOST_AUTO_TEST_CASE(cpu_topology_affinity_mask_covers_enumerated_cores) { + // The only check that the mask EXCHANGED and the cores PLACED come from the same view of the + // machine. affinity_mask_words otherwise has no test caller at all. + const auto cores = partition::enumerate_physical_cores(); + if (cores.empty()) { + return; // no topology (hwloc unavailable); nothing to agree with + } + std::vector mine(partition::kAffinityMaskWords, 0); + if (!partition::affinity_mask_words(mine.data(), mine.size())) { + return; // mask too wide to represent, or hwloc unavailable + } + size_t set_bits = 0; + for (const uint64_t w : mine) { + set_bits += static_cast(__builtin_popcountll(w)); + } + BOOST_CHECK(set_bits > 0u); + for (const auto &core : cores) { + const auto pu = static_cast(core.cpu); + BOOST_CHECK((mine[pu / 64] >> (pu % 64)) & 1U); + } +} + +#if defined(__linux__) + +BOOST_AUTO_TEST_CASE(cpu_topology_per_rank_mask_still_places) { + const auto full = partition::enumerate_physical_cores(); + if (full.size() < 2) { + return; // need two cores to confine to + } + + const AffinityGuard guard; + + cpu_set_t confined; + CPU_ZERO(&confined); + CPU_SET(full[0].cpu, &confined); + CPU_SET(full[1].cpu, &confined); + if (sched_setaffinity(0, sizeof(confined), &confined) != 0) { + return; // not permitted here (seccomp, restrictive cgroup); nothing to assert + } + + // n = the whole share, and a group_count as if seven sibling ranks shared the node. This is + // the configuration `srun --cpu-bind=cores` produces, and without the collapse it places + // NOTHING: the request is group_count x n cores out of a list holding n. + const auto sets = + partition::partition_cpusets(/*n=*/2, /*group_index=*/3, /*group_count=*/8, /*mask_is_private=*/true); + BOOST_REQUIRE_EQUAL(sets.size(), 2u); + for (const auto &set : sets) { + // Never pin outside the mask the launcher gave us. + BOOST_CHECK(set.pu == full[0].cpu || set.pu == full[1].cpu); + } + // The two partitions must not land on the same core. + BOOST_CHECK(sets[0].pu != sets[1].pu); +} + +BOOST_AUTO_TEST_CASE(cpu_topology_shared_mask_keeps_co_located_ranks_disjoint) { + const auto full = partition::enumerate_physical_cores(); + if (full.size() < 4) { + return; // need two cores per rank for two ranks + } + + const AffinityGuard guard; + + // A shared mask narrower than the host: four cores that both ranks can see. + cpu_set_t shared; + CPU_ZERO(&shared); + for (size_t i = 0; i < 4; ++i) { + CPU_SET(full[i].cpu, &shared); + } + if (sched_setaffinity(0, sizeof(shared), &shared) != 0) { + return; + } + + const auto rank0 = + partition::partition_cpusets(/*n=*/2, /*group_index=*/0, /*group_count=*/2, /*mask_is_private=*/false); + const auto rank1 = + partition::partition_cpusets(/*n=*/2, /*group_index=*/1, /*group_count=*/2, /*mask_is_private=*/false); + BOOST_REQUIRE_EQUAL(rank0.size(), 2u); + BOOST_REQUIRE_EQUAL(rank1.size(), 2u); + for (const auto &a : rank0) { + for (const auto &b : rank1) { + // Two ranks sharing a core would have each one's busy-polling collectives starve the + // other's barrier spins -- the failure this whole placement path exists to avoid. + BOOST_CHECK(a.pu != b.pu); + } + } +} + +#endif // __linux__ From cd6f2336ea22bfcfca05daa11a5fa2f143aad20f Mon Sep 17 00:00:00 2001 From: Aaron Miller Date: Mon, 17 Aug 2026 15:18:04 +0100 Subject: [PATCH 03/10] =?UTF-8?q?docs(agents):=20=E2=9C=8F=EF=B8=8F=20repa?= =?UTF-8?q?ir=20a=20truncated=20clause=20in=20the=20placement=20rule?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The rewrite in b3d788d left "every rank silently runs unpinned -- which also costs every rank silently runs unpinned": half of the deleted clause and its own replacement, side by side. The clause being deleted was the fix commit's "the two-level barrier loses its domains at the same time, because `cpuset_domains` derives them from the placement". `cpuset_domains` is the OTHER branch's vocabulary -- `git grep` finds it nowhere in this tree -- so the clause goes rather than being repaired, and the consequence stated is the one this branch can actually show: every rank runs unpinned. Assisted-by: ClaudeCode:claude-opus-5 --- AGENTS.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 011d65d4..9a323c85 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -108,9 +108,9 @@ Key files: inside the calling thread's affinity mask, so when a launcher has given each co-located rank its own disjoint slice (`srun --cpu-bind=cores`), the slice *is* the rank's share. Passing the node-wide ranks-per-node through as `group_count` then asks for `group_count × n` cores out of a list that only - held `n`, `placement_order` correctly refuses, and every rank silently runs unpinned — which also costs - every rank silently runs unpinned. Measured on Deucalion at 8 ranks × 16 partitions: `--cpu-bind=cores` - and `--cpu-bind=threads` both leave **0** threads pinned per rank, against 16 under `--cpu-bind=none`. + held `n`, `placement_order` correctly refuses, and every rank silently runs unpinned. Measured on Deucalion + at 8 ranks × 16 partitions: `--cpu-bind=cores` and `--cpu-bind=threads` both leave **0** threads pinned per + rank, against 16 under `--cpu-bind=none`. `PartitionGroup::classify_node_masks_` therefore allgathers the raw affinity masks over its node-local communicator and `masks_are_pairwise_disjoint` **measures** disjointness; a private result passes `mask_is_private` to `partition_cpusets`, which collapses `group_count` to 1. Mask *width* cannot From 9bf4cdbedd8ec232a0e2c9f4e6f52b8a30094ad4 Mon Sep 17 00:00:00 2001 From: Aaron Miller Date: Mon, 17 Aug 2026 15:18:17 +0100 Subject: [PATCH 04/10] =?UTF-8?q?refactor(partition):=20=F0=9F=A6=BA=20ass?= =?UTF-8?q?ert=20the=20affinity-mask=20width=20fits=20an=20MPI=20element?= =?UTF-8?q?=20count?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit kAffinityMaskWords and affinity_mask_words() "must agree", but they cannot be tied by a static_assert: the function takes the width as a runtime `nwords` argument, as does masks_are_pairwise_disjoint()'s `words`, so there is no shape to assert against. Hoisting the constant out of PartitionGroup (b3d788d) is the whole fix -- there is now one definition, and the drift it could have had was a second one. What is assertable is the constraint the exchange imposes on the value: PartitionGroup passes the width to MPI_Allgather as an element count, and MPI counts are `int`. State it where the constant is declared, so a future widening past INT_MAX fails to compile rather than passing a truncated count. No behaviour change: a static_assert emits no code, and the objects prove it. Recompiled with the job-1828023 build's own flags, CpuTopology.cpp.o, Evolution.cpp.o (which reaches this header through MonomialPropagator.h -> PartitionGroup.h) and partition_equivalence_tests.cpp.o are byte-identical with and without this hunk. Assisted-by: ClaudeCode:claude-opus-5 --- cpp/monoprop/detail/partition/CpuTopology.h | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/cpp/monoprop/detail/partition/CpuTopology.h b/cpp/monoprop/detail/partition/CpuTopology.h index 7522ed0d..c1880fc5 100644 --- a/cpp/monoprop/detail/partition/CpuTopology.h +++ b/cpp/monoprop/detail/partition/CpuTopology.h @@ -24,6 +24,7 @@ #pragma once +#include #include #include #include @@ -101,6 +102,16 @@ auto enumerate_physical_cores() -> std::vector; */ inline constexpr size_t kAffinityMaskWords = 64; +/* The agreement between this constant and affinity_mask_words()/masks_are_pairwise_disjoint() is + * NOT assertable: both take the width as a runtime `nwords`/`words` argument, so there is no shape + * to tie. Declaring it here, as the single definition every caller reads, is the whole fix -- the + * previous private copy on PartitionGroup was a second definition that could drift. + * + * What IS a compile-time constraint is the one the exchange imposes: the width is passed to + * MPI_Allgather as an element COUNT, and MPI counts are `int`. */ +static_assert(kAffinityMaskWords > 0 && kAffinityMaskWords <= static_cast(INT_MAX), + "the affinity-mask width is an MPI_Allgather element count, which is an int"); + /*! * @brief This process's effective allowed cpuset, as a bit array of @p nwords 64-bit words. * From bcabd6509e113b32ded92bfb9a8a7c2e81720b19 Mon Sep 17 00:00:00 2001 From: Aaron Miller Date: Mon, 17 Aug 2026 15:24:16 +0100 Subject: [PATCH 05/10] =?UTF-8?q?test(partition):=20=F0=9F=A7=AA=20close?= =?UTF-8?q?=20the=20vacuous-pass=20window=20in=20the=20live=20placement=20?= =?UTF-8?q?cases?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A case that early-returns on a constrained runner reports green while testing nothing, which is the exact defect the ported cases exist to fix -- and three of them had it. cpu_topology_shared_mask_keeps_co_located_ranks_disjoint required FOUR physical cores. A standard GitHub runner has two, so on CI it returned at the first line, every time. Two cores are enough: one per rank still asserts that two co-located ranks do not share one. cpu_topology_per_rank_mask_still_places required two. One is enough -- the request is group_count x n out of a list holding n at any n -- so the window shrinks from "fewer than two cores" to "hwloc loaded no topology at all", which is the condition under which nothing in this file means anything. cpu_topology_affinity_mask_covers_enumerated_cores returned silently when affinity_mask_words() said no. But enumerate_physical_cores() has just succeeded at that point, so hwloc demonstrably works and the only licensed refusal is the truncation guard. It now asserts that some enumerated PU really does sit past kAffinityMaskWords * 64; a mask function that had simply stopped working used to read as "nothing to test". Two premises are checked rather than assumed. confine_to_first() re-enumerates and requires the narrowing to have reached hwloc -- if sched_setaffinity ever stopped moving what effective_allowed_cpuset() sees, both live cases would go on asserting against the unconfined machine and go on passing, which is an early return that does not look like one. And an empty placement is now licensed only by monoprop_PARTITION_PINNING being off; "placed nothing" is the bug this branch fixes and must never pass as a configuration. What remains vacuous, and cannot be fixed here: hwloc loading no topology, and the kernel refusing sched_setaffinity. Both are named at the return. The mechanism itself is covered hardware-free by cpu_topology_policy_per_rank_slice_starves_without_collapse and cpu_topology_masks_*, which have no early return at all. Assisted-by: ClaudeCode:claude-opus-5 --- cpp/tests/cpu_topology_tests.cpp | 124 +++++++++++++++++++++++-------- 1 file changed, 92 insertions(+), 32 deletions(-) diff --git a/cpp/tests/cpu_topology_tests.cpp b/cpp/tests/cpu_topology_tests.cpp index 3a73bf45..b263b993 100644 --- a/cpp/tests/cpu_topology_tests.cpp +++ b/cpp/tests/cpu_topology_tests.cpp @@ -33,6 +33,7 @@ #include #endif +#include "monoprop/detail/EnvConfig.h" // config::get().partition_pinning -- the one licensed empty placement #include "monoprop/detail/partition/CpuTopology.h" namespace partition = monoprop::detail::partition; @@ -309,11 +310,20 @@ BOOST_AUTO_TEST_CASE(cpu_topology_affinity_mask_covers_enumerated_cores) { // machine. affinity_mask_words otherwise has no test caller at all. const auto cores = partition::enumerate_physical_cores(); if (cores.empty()) { - return; // no topology (hwloc unavailable); nothing to agree with + return; // hwloc loaded no topology at all: there is no second view to agree with. } std::vector mine(partition::kAffinityMaskWords, 0); if (!partition::affinity_mask_words(mine.data(), mine.size())) { - return; // mask too wide to represent, or hwloc unavailable + /* NOT a free skip. enumerate_physical_cores just succeeded, so hwloc works here and the + * only licensed reason to refuse is the truncation guard: some allowed PU sits past the + * exchanged window. Assert that, or a mask function that had simply stopped working would + * read as "nothing to test" -- which is the failure mode this whole file exists to close. */ + int highest = 0; + for (const auto &core : cores) { + highest = std::max(highest, core.cpu); + } + BOOST_CHECK_GE(static_cast(highest), partition::kAffinityMaskWords * 64); + return; } size_t set_bits = 0; for (const uint64_t w : mine) { @@ -328,60 +338,110 @@ BOOST_AUTO_TEST_CASE(cpu_topology_affinity_mask_covers_enumerated_cores) { #if defined(__linux__) +namespace { + +/* Confine this thread to the first `k` enumerated cores and assert the premise took hold. + * + * The premise is not free. effective_allowed_cpuset() re-reads hwloc_get_cpubind() on every call, + * so a successful sched_setaffinity DOES narrow what enumerate_physical_cores() reports -- but if + * it ever stopped doing so, the two cases below would keep asserting, against the UNCONFINED + * machine, and keep passing. That is the same defect as an early return, only harder to see, so + * the narrowing is checked rather than assumed. + * + * @returns false when the kernel refused the call (seccomp, a restrictive cgroup), which is the + * one condition here that no assertion can rescue. */ +auto confine_to_first(const std::vector &full, size_t k) -> bool { + cpu_set_t mask; + CPU_ZERO(&mask); + for (size_t i = 0; i < k; ++i) { + CPU_SET(full[i].cpu, &mask); + } + if (sched_setaffinity(0, sizeof(mask), &mask) != 0) { + return false; + } + BOOST_REQUIRE_EQUAL(partition::enumerate_physical_cores().size(), k); + return true; +} + +/* An empty placement is licensed by exactly one thing -- pinning turned off -- and by nothing + * else. Spelled as a check so that "placed nothing", the bug this branch fixes, can never be + * mistaken for a configuration. */ +auto empty_placement_is_licensed() -> bool { + if (monoprop::config::get().partition_pinning) { + return false; + } + BOOST_TEST_MESSAGE("monoprop_PARTITION_PINNING is off; partition_cpusets places nothing"); + return true; +} + +} // namespace + BOOST_AUTO_TEST_CASE(cpu_topology_per_rank_mask_still_places) { const auto full = partition::enumerate_physical_cores(); - if (full.size() < 2) { - return; // need two cores to confine to + if (full.empty()) { + return; // hwloc loaded no topology: there is no mask to confine to. } + /* ONE core is enough to exercise the collapse -- the request is group_count x n out of a list + * holding n either way. Requiring two would let this pass vacuously on a single-core runner, + * which is exactly the property that made the live case it was ported alongside worthless. */ + const size_t k = std::min(full.size(), 2); const AffinityGuard guard; - - cpu_set_t confined; - CPU_ZERO(&confined); - CPU_SET(full[0].cpu, &confined); - CPU_SET(full[1].cpu, &confined); - if (sched_setaffinity(0, sizeof(confined), &confined) != 0) { - return; // not permitted here (seccomp, restrictive cgroup); nothing to assert + if (!confine_to_first(full, k)) { + return; // the kernel refused the affinity call; nothing below is reachable } // n = the whole share, and a group_count as if seven sibling ranks shared the node. This is // the configuration `srun --cpu-bind=cores` produces, and without the collapse it places // NOTHING: the request is group_count x n cores out of a list holding n. const auto sets = - partition::partition_cpusets(/*n=*/2, /*group_index=*/3, /*group_count=*/8, /*mask_is_private=*/true); - BOOST_REQUIRE_EQUAL(sets.size(), 2u); + partition::partition_cpusets(/*n=*/k, /*group_index=*/3, /*group_count=*/8, /*mask_is_private=*/true); + if (sets.empty() && empty_placement_is_licensed()) { + return; + } + BOOST_REQUIRE_EQUAL(sets.size(), k); for (const auto &set : sets) { // Never pin outside the mask the launcher gave us. - BOOST_CHECK(set.pu == full[0].cpu || set.pu == full[1].cpu); + bool inside = false; + for (size_t i = 0; i < k; ++i) { + inside = inside || set.pu == full[i].cpu; + } + BOOST_CHECK(inside); + } + if (k == 2) { + // The two partitions must not land on the same core. + BOOST_CHECK(sets[0].pu != sets[1].pu); } - // The two partitions must not land on the same core. - BOOST_CHECK(sets[0].pu != sets[1].pu); } BOOST_AUTO_TEST_CASE(cpu_topology_shared_mask_keeps_co_located_ranks_disjoint) { const auto full = partition::enumerate_physical_cores(); - if (full.size() < 4) { - return; // need two cores per rank for two ranks + if (full.size() < 2) { + return; // two ranks cannot hold disjoint cores when there is only one } + /* Two cores are enough: one per rank. The ported version asked for four, which a two-physical- + * core CI runner does not have -- it would have returned here and reported green. */ + const size_t per_rank = full.size() >= 4 ? 2 : 1; + const size_t shared = 2 * per_rank; const AffinityGuard guard; - - // A shared mask narrower than the host: four cores that both ranks can see. - cpu_set_t shared; - CPU_ZERO(&shared); - for (size_t i = 0; i < 4; ++i) { - CPU_SET(full[i].cpu, &shared); + if (!confine_to_first(full, shared)) { + return; // the kernel refused the affinity call; nothing below is reachable } - if (sched_setaffinity(0, sizeof(shared), &shared) != 0) { + + const auto rank0 = partition::partition_cpusets(/*n=*/per_rank, + /*group_index=*/0, + /*group_count=*/2, + /*mask_is_private=*/false); + const auto rank1 = partition::partition_cpusets(/*n=*/per_rank, + /*group_index=*/1, + /*group_count=*/2, + /*mask_is_private=*/false); + if (rank0.empty() && rank1.empty() && empty_placement_is_licensed()) { return; } - - const auto rank0 = - partition::partition_cpusets(/*n=*/2, /*group_index=*/0, /*group_count=*/2, /*mask_is_private=*/false); - const auto rank1 = - partition::partition_cpusets(/*n=*/2, /*group_index=*/1, /*group_count=*/2, /*mask_is_private=*/false); - BOOST_REQUIRE_EQUAL(rank0.size(), 2u); - BOOST_REQUIRE_EQUAL(rank1.size(), 2u); + BOOST_REQUIRE_EQUAL(rank0.size(), per_rank); + BOOST_REQUIRE_EQUAL(rank1.size(), per_rank); for (const auto &a : rank0) { for (const auto &b : rank1) { // Two ranks sharing a core would have each one's busy-polling collectives starve the From d4361c190a9c3d6b92d3070d60e029e9946cea67 Mon Sep 17 00:00:00 2001 From: Aaron Miller Date: Wed, 19 Aug 2026 22:52:45 +0100 Subject: [PATCH 06/10] =?UTF-8?q?refactor(partition):=20=E2=99=BB=EF=B8=8F?= =?UTF-8?q?=20drop=20the=20redundant=20mask=20reduction=20and=20placement?= =?UTF-8?q?=20retry?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The MPI_Allreduce over the per-rank affinity_mask_words result cannot change a verdict: refusal leaves the mask zeroed, and masks_are_pairwise_disjoint already rejects an all-zero row, so every peer computes "not private" from the allgathered buffer alone. The private-mask collapse becomes unconditional rather than a retry after the normal split has failed, which removes the second call to placement_order. A rank whose mask is private owns all of it, so it now takes the head of its own interleaved order instead of a group_index-offset slice of it even when the split would have succeeded. --- cpp/monoprop/detail/partition/CpuTopology.cpp | 38 +++---------------- cpp/monoprop/detail/partition/CpuTopology.h | 8 +--- .../detail/partition/PartitionGroup.h | 15 +------- 3 files changed, 8 insertions(+), 53 deletions(-) diff --git a/cpp/monoprop/detail/partition/CpuTopology.cpp b/cpp/monoprop/detail/partition/CpuTopology.cpp index 6e27558a..f48cdd41 100644 --- a/cpp/monoprop/detail/partition/CpuTopology.cpp +++ b/cpp/monoprop/detail/partition/CpuTopology.cpp @@ -283,40 +283,12 @@ auto partition_cpusets(size_t n, size_t group_index, size_t group_count, bool ma } const auto cores = enumerate_physical_cores(); - /* Do not partition a node the batch system has already partitioned. - * - * enumerate_physical_cores() reports only the cores in THIS process's affinity mask. Under a - * resource manager that hands each rank its own cpuset -- Slurm with cgroups does, which is the - * configuration every benchmark here runs in -- that mask is already this rank's exclusive - * share, so `cores` IS our slice and not the node. Splitting it again by group_count asks for - * group_count x more cores than exist; placement_order correctly refuses and returns {}, so - * NOTHING is pinned, on every rank, silently -- pin_this_thread ignores bind errors by design. - * Measured signature: 8 ranks x 16 partitions with affinity_cpus=16 per rank gave - * distinct_pinned_cpus=0 and voided the run. - * - * The disjointness this function exists to guarantee still holds there: the cgroups are - * disjoint by construction, so placing within our own mask cannot collide with a co-located - * rank. When the mask is the whole node the condition is false and co-located ranks are - * separated here exactly as before. - * - * The discriminator CANNOT be a core count, and cannot be confinement either. - * `cores.size() < group_count * n` is equally the signature of a genuine oversubscription on an - * unconfined node. And "our mask is narrower than the machine" cannot tell "8 ranks holding 16 - * cores each" from "8 ranks SHARING one 16-core mask" -- both leave a rank seeing 16 of 128. - * Collapsing in the shared case points every co-located rank at the same cores, which is worse - * than not pinning: each rank's busy-polling collectives then starve the others' barrier spins. - * Only comparing the co-located ranks' masks answers it, so the caller establishes - * `mask_is_private` by allgathering them (PartitionGroup::discover_node_peers_) and we do not - * guess here. - * - * This is also a FALLBACK rather than a replacement: when the mask is wide enough to hold all - * group_count groups the normal split still runs and still separates co-located ranks. So the - * new arm can only ever turn "nothing pinned" into "something pinned"; it cannot take a working - * placement away. */ - auto order = topo_detail::placement_order(cores, n, group_index, group_count); - if (order.empty() && group_count > 1 && mask_is_private) { - order = topo_detail::placement_order(cores, n, /*group_index=*/0, /*group_count=*/1); + // A private mask IS this rank's share: the launcher already separated co-located ranks. + if (mask_is_private) { + group_index = 0; + group_count = 1; } + const auto order = topo_detail::placement_order(cores, n, group_index, group_count); std::vector sets(order.size()); for (size_t i = 0; i < order.size(); ++i) { diff --git a/cpp/monoprop/detail/partition/CpuTopology.h b/cpp/monoprop/detail/partition/CpuTopology.h index c1880fc5..dee880d0 100644 --- a/cpp/monoprop/detail/partition/CpuTopology.h +++ b/cpp/monoprop/detail/partition/CpuTopology.h @@ -164,13 +164,7 @@ auto affinity_mask_words(uint64_t *out, size_t nwords) -> bool; * @returns Vector of @p n CpuSet tokens, or empty when @c monoprop_PARTITION_PINNING is disabled, * hwloc is unavailable, or not even @p n distinct cores are visible to this process. * - * @note With @p mask_is_private, @p group_index / @p group_count are ignored once the normal split - * has failed: the batch system has already partitioned the node (Slurm with per-rank cgroups), - * so subdividing our own share a second time asks for @p group_count × more cores than exist - * and places nothing at all. Disjointness is what makes ignoring them safe. Without it, a set - * of ranks SHARING one narrow mask would every one of them collapse onto the same cores -- - * worse than not pinning, since each rank's busy-polling collectives would then starve the - * others' barrier spins. + * @note Under @p mask_is_private the group split is skipped: our share is already this rank's alone. */ auto partition_cpusets(size_t n, size_t group_index = 0, size_t group_count = 1, bool mask_is_private = false) -> std::vector; diff --git a/cpp/monoprop/detail/partition/PartitionGroup.h b/cpp/monoprop/detail/partition/PartitionGroup.h index 2431d247..3934581f 100644 --- a/cpp/monoprop/detail/partition/PartitionGroup.h +++ b/cpp/monoprop/detail/partition/PartitionGroup.h @@ -189,21 +189,10 @@ class PartitionGroup { } constexpr size_t kMaskWords = monoprop::detail::partition::kAffinityMaskWords; std::array mine{}; - // A mask we cannot represent is "cannot classify", never "private": the fallback stays off. - const bool ok = monoprop::detail::partition::affinity_mask_words(mine.data(), kMaskWords); + // Refusal zeroes `mine` and an all-zero row is never private, so no reduction of the verdict is needed. + monoprop::detail::partition::affinity_mask_words(mine.data(), kMaskWords); std::vector all(kMaskWords * static_cast(node_size_), 0); MPI_Allgather(mine.data(), kMaskWords, MPI_UINT64_T, all.data(), kMaskWords, MPI_UINT64_T, node); - - // Reduced before the verdict, not after: `ok` is per-rank, and a rank whose mask is too - // wide to represent must make EVERY peer decide "not private". A verdict computed from a - // buffer some ranks filled and others did not would differ between ranks, and this one - // feeds partition_cpusets on all of them. - int local_ok = ok ? 1 : 0; - int all_ok = 0; - MPI_Allreduce(&local_ok, &all_ok, 1, MPI_INT, MPI_MIN, node); - if (all_ok == 0) { - return; - } // The rule itself is a free function over the gathered array -- pure bit arithmetic, no // communicator, no hwloc -- so it is reachable from a unit test on any machine. What is // left here is only the exchange. From b21e206070615c1bec81ff8526b88b879401b5d3 Mon Sep 17 00:00:00 2001 From: Aaron Miller Date: Wed, 19 Aug 2026 22:53:50 +0100 Subject: [PATCH 07/10] =?UTF-8?q?fix(partition):=20=F0=9F=90=9B=20refuse?= =?UTF-8?q?=20a=20zero=20group=20count=20and=20say=20so=20when=20placement?= =?UTF-8?q?=20is=20impossible?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit group_count == 0 never advanced the interleave arm's stride, so `mine` grew without bound. It joins the existing oversubscription guard. An empty placement is the failure this branch exists to fix and it was returned silently. One warning per process now names the core count and the request it could not meet. --- cpp/monoprop/detail/partition/CpuTopology.cpp | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/cpp/monoprop/detail/partition/CpuTopology.cpp b/cpp/monoprop/detail/partition/CpuTopology.cpp index f48cdd41..62b021a9 100644 --- a/cpp/monoprop/detail/partition/CpuTopology.cpp +++ b/cpp/monoprop/detail/partition/CpuTopology.cpp @@ -15,7 +15,10 @@ #include "monoprop/detail/partition/CpuTopology.h" #include +#include #include +#include +#include #include #include @@ -88,7 +91,7 @@ namespace topo_detail { auto placement_order(const std::vector &cores, size_t n, size_t group_index, size_t group_count) -> std::vector { - if (cores.empty() || group_count * n > cores.size()) { + if (cores.empty() || group_count == 0 || group_count * n > cores.size()) { return {}; } @@ -290,6 +293,19 @@ auto partition_cpusets(size_t n, size_t group_index, size_t group_count, bool ma } const auto order = topo_detail::placement_order(cores, n, group_index, group_count); + if (order.empty()) { + static std::once_flag warned; + std::call_once(warned, [&] { + std::print(stderr, + "monoprop: partition pinning requested but not possible " + "({} cores visible, {} groups x {} partitions); threads run unpinned.\n", + cores.size(), + group_count, + n); + std::fflush(stderr); + }); + } + std::vector sets(order.size()); for (size_t i = 0; i < order.size(); ++i) { sets[i] = CpuSet{order[i]}; From b3ccd2895a65f6c886812def4f0c302cc61b09d9 Mon Sep 17 00:00:00 2001 From: Aaron Miller Date: Wed, 19 Aug 2026 22:55:29 +0100 Subject: [PATCH 08/10] =?UTF-8?q?test(partition):=20=F0=9F=A7=AA=20close?= =?UTF-8?q?=20two=20spurious=20failures=20and=20pin=20the=20unconditional?= =?UTF-8?q?=20collapse?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The truncation-guard assertion compared kAffinityMaskWords*64 against the core representatives, which are the LOWEST allowed sibling of each core, while affinity_mask_words refuses on the HIGHEST allowed PU. A core holding PUs {100, 4196} refuses with a representative of 100. It now re-reads the mask through a wider window and asserts against its own highest bit. empty_placement_is_licensed() moves out of the __linux__ block so the two live smoke cases can use it; both hard-failed under monoprop_PARTITION_PINNING=0. The new policy case covers a private mask whose split WOULD have fit, which the fallback form never reached. --- cpp/tests/cpu_topology_tests.cpp | 64 ++++++++++++++++++++++++-------- 1 file changed, 48 insertions(+), 16 deletions(-) diff --git a/cpp/tests/cpu_topology_tests.cpp b/cpp/tests/cpu_topology_tests.cpp index b263b993..190ad453 100644 --- a/cpp/tests/cpu_topology_tests.cpp +++ b/cpp/tests/cpu_topology_tests.cpp @@ -49,6 +49,21 @@ struct AffinityGuard { #endif }; +namespace { + +/* An empty placement is licensed by exactly one thing -- pinning turned off -- and by nothing + * else. Spelled as a check so that "placed nothing", the bug this branch fixes, can never be + * mistaken for a configuration. */ +auto empty_placement_is_licensed() -> bool { + if (monoprop::config::get().partition_pinning) { + return false; + } + BOOST_TEST_MESSAGE("monoprop_PARTITION_PINNING is off; partition_cpusets places nothing"); + return true; +} + +} // namespace + /* ── Live smoke tests ─────────────────────────────────────────────────────── */ BOOST_AUTO_TEST_CASE(cpu_topology_enumerate_and_place) { @@ -64,7 +79,7 @@ BOOST_AUTO_TEST_CASE(cpu_topology_enumerate_and_place) { // guard restores affinity on scope exit } // When topology discovery succeeds, a non-empty core list must produce a non-empty placement. - if (!cores.empty()) { + if (!cores.empty() && !(one.empty() && empty_placement_is_licensed())) { BOOST_CHECK_EQUAL(one.size(), 1u); } @@ -118,6 +133,9 @@ BOOST_AUTO_TEST_CASE(cpu_topology_place_co_located_ranks) { /*group_index=*/1, /*group_count=*/2, /*mask_is_private=*/true); + if (private_mask.empty() && empty_placement_is_licensed()) { + return; + } BOOST_REQUIRE_EQUAL(private_mask.size(), cores.size()); std::set placed; for (const auto &set : private_mask) { @@ -239,6 +257,24 @@ BOOST_AUTO_TEST_CASE(cpu_topology_policy_per_rank_slice_starves_without_collapse BOOST_CHECK_EQUAL(collapsed[1], 7); } +BOOST_AUTO_TEST_CASE(cpu_topology_policy_private_mask_collapses_even_when_the_split_would_fit) { + // 4 cores over 2 L3 domains: 2 ranks x 2 partitions fits exactly, so the collapse here is not a + // fallback after a refusal -- it changes which cores a private-mask rank is given. + const std::vector cores = {{0, 0}, {2, 1}, {4, 0}, {6, 1}}; + + // Rank 1's share under the split: domain 1 only. + const auto split = placement_order(cores, 2, /*group_index=*/1, /*group_count=*/2); + BOOST_REQUIRE_EQUAL(split.size(), 2u); + BOOST_CHECK_EQUAL(split[0], 2); + BOOST_CHECK_EQUAL(split[1], 6); + + // What mask_is_private now passes instead: the head of this rank's own interleave over both domains. + const auto collapsed = placement_order(cores, 2, /*group_index=*/0, /*group_count=*/1); + BOOST_REQUIRE_EQUAL(collapsed.size(), 2u); + BOOST_CHECK_EQUAL(collapsed[0], 0); + BOOST_CHECK_EQUAL(collapsed[1], 2); +} + namespace { /* Build the flat [n * words] array masks_are_pairwise_disjoint reads, from a list of PU-index @@ -318,11 +354,18 @@ BOOST_AUTO_TEST_CASE(cpu_topology_affinity_mask_covers_enumerated_cores) { * only licensed reason to refuse is the truncation guard: some allowed PU sits past the * exchanged window. Assert that, or a mask function that had simply stopped working would * read as "nothing to test" -- which is the failure mode this whole file exists to close. */ - int highest = 0; - for (const auto &core : cores) { - highest = std::max(highest, core.cpu); + // Refusal keys on the HIGHEST allowed PU while core.cpu is the LOWEST sibling of each core, + // so a re-read through a wider window is the only sound witness. + std::vector wide(partition::kAffinityMaskWords * 64, 0); + BOOST_REQUIRE(partition::affinity_mask_words(wide.data(), wide.size())); + size_t highest = 0; + for (size_t w = wide.size(); w-- > 0;) { + if (wide[w] != 0) { + highest = (w * 64) + static_cast(63 - __builtin_clzll(wide[w])); + break; + } } - BOOST_CHECK_GE(static_cast(highest), partition::kAffinityMaskWords * 64); + BOOST_CHECK_GE(highest, partition::kAffinityMaskWords * 64); return; } size_t set_bits = 0; @@ -363,17 +406,6 @@ auto confine_to_first(const std::vector &full, size_t k return true; } -/* An empty placement is licensed by exactly one thing -- pinning turned off -- and by nothing - * else. Spelled as a check so that "placed nothing", the bug this branch fixes, can never be - * mistaken for a configuration. */ -auto empty_placement_is_licensed() -> bool { - if (monoprop::config::get().partition_pinning) { - return false; - } - BOOST_TEST_MESSAGE("monoprop_PARTITION_PINNING is off; partition_cpusets places nothing"); - return true; -} - } // namespace BOOST_AUTO_TEST_CASE(cpu_topology_per_rank_mask_still_places) { From 3662fe12e0c034bbd762b6b75efb8193ff603406 Mon Sep 17 00:00:00 2001 From: Aaron Miller Date: Wed, 19 Aug 2026 23:00:52 +0100 Subject: [PATCH 09/10] =?UTF-8?q?style(partition):=20=F0=9F=8E=A8=20cut=20?= =?UTF-8?q?the=20placement=20comments=20to=20what=20the=20code=20cannot=20?= =?UTF-8?q?say?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The AGENTS.md bullets restated the header and the PR body; both are dropped. What survives elsewhere is the empty-mask rule, what the synthetic core literals stand for physically, and why two cores rather than four are enough on a CI runner. --- AGENTS.md | 25 ----- cpp/monoprop/detail/partition/CpuTopology.cpp | 6 +- cpp/monoprop/detail/partition/CpuTopology.h | 57 ++--------- .../detail/partition/PartitionGroup.h | 24 +---- cpp/tests/cpu_topology_tests.cpp | 94 +++---------------- 5 files changed, 28 insertions(+), 178 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 9a323c85..35bcd09e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -104,31 +104,6 @@ Key files: for the mutating/collecting paths, which run on the partitions' own pinned masters; `sum_partitions_`, `fold_partitions_`, `first_partition_` for reads off quiescent partitions) rather than hand-rolling a `run_on_all` loop — the declarations record which helper is legal where. -- **Placement must not divide an already-divided machine.** `enumerate_physical_cores` reports only cores - inside the calling thread's affinity mask, so when a launcher has given each co-located rank its own - disjoint slice (`srun --cpu-bind=cores`), the slice *is* the rank's share. Passing the node-wide - ranks-per-node through as `group_count` then asks for `group_count × n` cores out of a list that only - held `n`, `placement_order` correctly refuses, and every rank silently runs unpinned. Measured on Deucalion - at 8 ranks × 16 partitions: `--cpu-bind=cores` and `--cpu-bind=threads` both leave **0** threads pinned per - rank, against 16 under `--cpu-bind=none`. - `PartitionGroup::classify_node_masks_` therefore allgathers the raw affinity masks over its node-local - communicator and `masks_are_pairwise_disjoint` **measures** disjointness; a private result passes - `mask_is_private` to `partition_cpusets`, which collapses `group_count` to 1. Mask *width* cannot - substitute for this — "8 ranks holding 16 cores each" and "8 ranks sharing one 16-core mask" both leave a - rank seeing 16 of 128, and they need opposite placement. Collapse in the wrong direction and every - co-located rank pins to the *same* cores, so **not private** is the default and the safe error; an empty - mask is not private either, since an all-zero mask is trivially disjoint from everything. This regressed - once already, when topology discovery was rewritten onto hwloc, because the guard lives in the placement - policy rather than in discovery: any rework of that layer must re-check it. - `cpu_topology_policy_per_rank_slice_starves_without_collapse` and `cpu_topology_masks_*` pin the mechanism - without needing live hardware. -- **This is a correctness-of-measurement fix before it is a speed fix.** Unplaced ranks read as a slow code - path and have voided four A/B jobs, so every measurement is more trustworthy with it in. But at 99.4M - terms, 8×16, the branch carrying it is flat or worse on three of four operations — restoring placement - does not make that size faster. Do not quote a µs/sync figure across scales: `barrier_per_sync_us` is - peers idling while partition 0 is inside MPI, so it grows with the work waited *for* (15–22 µs at 0.8M - terms, 274–604 µs at 99M) and two such numbers from different sizes share no axis. Nor does pinning - reduce run-to-run variance: re-measured with 16 threads pinned per rank, spreads went 1.6–2.6× → 1.3–4.6×. ### Environment Management diff --git a/cpp/monoprop/detail/partition/CpuTopology.cpp b/cpp/monoprop/detail/partition/CpuTopology.cpp index 62b021a9..6c0387f3 100644 --- a/cpp/monoprop/detail/partition/CpuTopology.cpp +++ b/cpp/monoprop/detail/partition/CpuTopology.cpp @@ -236,8 +236,7 @@ auto affinity_mask_words(uint64_t *out, size_t nwords) -> bool { if (!allowed) { return false; } - /* Anything above the window we can exchange is reported as "cannot classify" rather than - * silently truncated: a truncated mask could compare disjoint against a peer it overlaps. */ + // Refused rather than truncated: a truncated mask could compare disjoint against a peer it overlaps. const int last = hwloc_bitmap_last(allowed); const bool representable = last >= 0 && static_cast(last) < nwords * 64; if (representable) { @@ -255,8 +254,7 @@ auto masks_are_pairwise_disjoint(const uint64_t *masks, size_t n, size_t words) if (masks == nullptr || words == 0 || n < 2) { return false; } - /* An all-zero mask is disjoint from everything, so a bare disjointness test would answer - * "private" for a rank that can see no CPU at all. Shared is the safe error. */ + // An all-zero mask is disjoint from everything, so empty is rejected before the pairwise test. for (size_t r = 0; r < n; ++r) { bool any = false; for (size_t w = 0; w < words && !any; ++w) { diff --git a/cpp/monoprop/detail/partition/CpuTopology.h b/cpp/monoprop/detail/partition/CpuTopology.h index dee880d0..52ec0640 100644 --- a/cpp/monoprop/detail/partition/CpuTopology.h +++ b/cpp/monoprop/detail/partition/CpuTopology.h @@ -90,60 +90,17 @@ auto placement_order(const std::vector &cores, size_t n, size_t gr */ auto enumerate_physical_cores() -> std::vector; -/*! - * @brief Number of 64-bit words used to exchange an affinity mask between co-located ranks. - * - * 64 words is 4096 CPUs. A mask needing more is reported as "cannot classify" by - * affinity_mask_words(), never as private, so the fallback stays off and the old unplaced - * behaviour returns rather than a wrong placement. - * - * Declared here, beside the two functions that must agree on it, rather than privately in the - * caller: a width that lives next to only one of the two can drift silently. - */ +//! Affinity-mask exchange width, in 64-bit words. A mask needing more is "cannot classify", never private. inline constexpr size_t kAffinityMaskWords = 64; -/* The agreement between this constant and affinity_mask_words()/masks_are_pairwise_disjoint() is - * NOT assertable: both take the width as a runtime `nwords`/`words` argument, so there is no shape - * to tie. Declaring it here, as the single definition every caller reads, is the whole fix -- the - * previous private copy on PartitionGroup was a second definition that could drift. - * - * What IS a compile-time constraint is the one the exchange imposes: the width is passed to - * MPI_Allgather as an element COUNT, and MPI counts are `int`. */ static_assert(kAffinityMaskWords > 0 && kAffinityMaskWords <= static_cast(INT_MAX), "the affinity-mask width is an MPI_Allgather element count, which is an int"); -/*! - * @brief This process's effective allowed cpuset, as a bit array of @p nwords 64-bit words. - * - * Exposed so callers can exchange masks between co-located ranks and test them for pairwise - * disjointness -- the only sound basis for deciding whether each rank owns a private share. - * - * @returns false (leaving @p out zeroed) when hwloc cannot load the topology or the mask needs - * more than @p nwords words, which is treated as "cannot classify" and hence not private. - */ +//! This process's allowed cpuset as @p nwords 64-bit words; false with @p out zeroed when it does not fit. auto affinity_mask_words(uint64_t *out, size_t nwords) -> bool; -/*! - * @brief Whether @p n masks of @p words words each, laid end to end in @p masks, are pairwise - * disjoint AND none of them is empty. - * - * This is the whole classification rule, and it is pure bit arithmetic over an already-gathered - * array: no communicator, no hwloc, no live hardware. It is a free function precisely so it can be - * tested, because the mechanism it implements has regressed once already -- when topology discovery - * moved onto hwloc -- and the guard lives in the placement policy rather than in discovery. - * - * Mask WIDTH cannot substitute for this. "8 ranks holding 16 cores each" and "8 ranks sharing one - * 16-core mask" both leave a rank seeing 16 of 128 CPUs, and they need opposite placements: the - * first should fill its own mask or nothing is pinned at all, the second must not or every rank - * lands on identical cores and their busy-polling collectives starve each other. - * - * The empty-mask clause is not incidental. An all-zero mask is trivially disjoint from everything, - * so a bare disjointness test would answer "private" for a rank that can see no CPU at all -- - * collapsing group_count in exactly the case where the caller knows least. Shared is the safe - * error, so an empty mask is not private. - * - * @returns false when @p n < 2 (nobody to collide with, and the normal split already handles it), - * when any mask is empty, or when any two masks share a bit. +/*! @brief Whether the @p n masks of @p words words laid end to end in @p masks are pairwise disjoint. + * False for @p n < 2 and for any empty mask: all-zero is disjoint from everything, and shared is the safe error. */ [[nodiscard]] auto masks_are_pairwise_disjoint(const uint64_t *masks, size_t n, size_t words) -> bool; @@ -157,10 +114,8 @@ auto affinity_mask_words(uint64_t *out, size_t nwords) -> bool; * @param n Number of partitions to place. * @param group_index This rank's 0-based index among the co-located ranks on the host. * @param group_count Total number of co-located ranks on the host. - * @param mask_is_private True only when the caller has established that the co-located ranks' - * affinity masks are pairwise DISJOINT, so this rank's mask is its own share of - * the node. See PartitionGroup::discover_node_peers_(), which allgathers the - * masks over its node communicator to decide. + * @param mask_is_private True only when the co-located ranks' affinity masks have been measured + * pairwise DISJOINT (PartitionGroup::classify_node_masks_), so this mask is our share. * @returns Vector of @p n CpuSet tokens, or empty when @c monoprop_PARTITION_PINNING is disabled, * hwloc is unavailable, or not even @p n distinct cores are visible to this process. * diff --git a/cpp/monoprop/detail/partition/PartitionGroup.h b/cpp/monoprop/detail/partition/PartitionGroup.h index 3934581f..169a0020 100644 --- a/cpp/monoprop/detail/partition/PartitionGroup.h +++ b/cpp/monoprop/detail/partition/PartitionGroup.h @@ -173,15 +173,7 @@ class PartitionGroup { } #ifdef monoprop_ENABLE_MPI - // Decide whether each co-located rank owns a PRIVATE share of the node's CPUs, by exchanging the - // actual affinity masks and testing them for pairwise disjointness. - // - // This cannot be inferred locally. A rank seeing 16 of the machine's 128 CPUs is equally "Slurm - // gave me my own 16" and "eight of us share the same 16", and the two want opposite placements: - // the first should fill its own mask (otherwise nothing is pinned at all), the second must not - // (or every rank lands on identical cores and their busy-polling collectives starve each other). - // Only the peers' masks separate them, and this is the one place that already has a node-local - // communicator open. One allgather of 512 B per rank, once per group construction. + // A rank seeing 16 of 128 CPUs is equally "my own 16" and "eight of us share these 16": only the masks tell. auto classify_node_masks_(MPI_Comm node) -> void { node_mask_private_ = false; if (node_size_ <= 1) { @@ -193,9 +185,6 @@ class PartitionGroup { monoprop::detail::partition::affinity_mask_words(mine.data(), kMaskWords); std::vector all(kMaskWords * static_cast(node_size_), 0); MPI_Allgather(mine.data(), kMaskWords, MPI_UINT64_T, all.data(), kMaskWords, MPI_UINT64_T, node); - // The rule itself is a free function over the gathered array -- pure bit arithmetic, no - // communicator, no hwloc -- so it is reachable from a unit test on any machine. What is - // left here is only the exchange. node_mask_private_ = monoprop::detail::partition::masks_are_pairwise_disjoint(all.data(), static_cast(node_size_), kMaskWords); @@ -285,13 +274,10 @@ class PartitionGroup { } int n_; - mpi::Comm parent_; // enclosing communicator (size R) — decides the transport - int node_rank_ = 0; // this rank's index among the ranks sharing the host - int node_size_ = 1; // how many parent ranks share the host (1 unless MPI R>1) - // True only when the co-located ranks' affinity masks are pairwise disjoint, i.e. each rank owns - // its share of the node. Decided in classify_node_masks_(); copied, never re-derived, by the copy - // ctor, which has no communicator to allgather over. - bool node_mask_private_ = false; + mpi::Comm parent_; // enclosing communicator (size R) — decides the transport + int node_rank_ = 0; // this rank's index among the ranks sharing the host + int node_size_ = 1; // how many parent ranks share the host (1 unless MPI R>1) + bool node_mask_private_ = false; // set by classify_node_masks_; copied, never re-derived, by the copy ctor std::unique_ptr shm_; // set iff R == 1 #ifdef monoprop_ENABLE_MPI std::unique_ptr hyb_; // set iff R > 1 diff --git a/cpp/tests/cpu_topology_tests.cpp b/cpp/tests/cpu_topology_tests.cpp index 190ad453..93dd165c 100644 --- a/cpp/tests/cpu_topology_tests.cpp +++ b/cpp/tests/cpu_topology_tests.cpp @@ -51,9 +51,7 @@ struct AffinityGuard { namespace { -/* An empty placement is licensed by exactly one thing -- pinning turned off -- and by nothing - * else. Spelled as a check so that "placed nothing", the bug this branch fixes, can never be - * mistaken for a configuration. */ +// An empty placement is licensed by pinning being off and by nothing else; "placed nothing" is the bug. auto empty_placement_is_licensed() -> bool { if (monoprop::config::get().partition_pinning) { return false; @@ -110,19 +108,7 @@ BOOST_AUTO_TEST_CASE(cpu_topology_place_co_located_ranks) { // invariant (one rank's busy-polling collectives cannot starve the other's barrier spins). BOOST_CHECK(rank0.front().pu != rank1.front().pu); - // 2 ranks × cores.size() partitions asks for more cores than this process can see. What that - // MEANS depends on whether the co-located ranks' masks are disjoint, which no local test can - // answer, so partition_cpusets takes it as an argument and both arms are pinned here: - // - // shared mask → a genuine oversubscription. Refuse: placing anyway would hand both ranks the - // same cores, and each rank's busy-polling collectives would starve the other's - // barrier spins — worse than not pinning at all. - // private mask → the batch system already split the node (Slurm per-rank cgroups) and this is - // our own share. Refusing here is exactly what left every rank unpinned on the - // benchmark nodes, voiding four A/B jobs. Fill our mask. - // - // Passing the flag explicitly is what makes this deterministic on any machine; the earlier - // version branched on the ambient environment and so asserted whichever answer the host gave. + // Both arms passed explicitly so neither depends on the host: refuse the shared one, fill the private. const auto shared_mask = partition::partition_cpusets(/*n=*/cores.size(), /*group_index=*/1, /*group_count=*/2, @@ -141,7 +127,6 @@ BOOST_AUTO_TEST_CASE(cpu_topology_place_co_located_ranks) { for (const auto &set : private_mask) { placed.insert(set.pu); } - // Distinct PUs, and every one of them a core this process was actually granted. BOOST_CHECK_EQUAL(placed.size(), cores.size()); std::set visible; for (const auto &core : cores) { @@ -228,26 +213,12 @@ BOOST_AUTO_TEST_CASE(cpu_topology_policy_uneven_domains) { BOOST_CHECK_EQUAL(order[2], 4); } -/* ── The cgroup-placement classification ────────────────────────────────────── - * - * These are ported from the competing implementation of this fix on - * perf/multinode-comm-scaling, which spelled the rule as a `NodeMask` enum over a `CpuMask` POD. - * The mechanism is the same; only the surface differs, so the cases transfer with the enum - * replaced by `masks_are_pairwise_disjoint` over a flat array and by `partition_cpusets`' - * `mask_is_private` flag. - * - * They are worth porting because this branch adds NO hardware-free coverage of its own -- it - * extends one live-topology case, which early-returns on a runner with fewer than two visible - * cores, so the whole fix can be unexercised while the suite reports green. And the mechanism - * has regressed once already, when topology discovery moved onto hwloc, because the guard lives - * in the placement policy rather than in discovery. - */ +/* ── The cgroup-placement classification ──────────────────────────────────── */ BOOST_AUTO_TEST_CASE(cpu_topology_policy_per_rank_slice_starves_without_collapse) { // One rank's slice under `srun --cpu-bind=cores`: 2 cores of a 16-core host, one L3 domain. const std::vector slice = {{6, 0}, {7, 0}}; - // Told the node-wide truth (8 ranks x 2 partitions), the request cannot be met from a slice. BOOST_CHECK(placement_order(slice, 2, /*group_index=*/3, /*group_count=*/8).empty()); // Collapsed to a single group -- what mask_is_private does -- the same slice places fully. @@ -258,17 +229,15 @@ BOOST_AUTO_TEST_CASE(cpu_topology_policy_per_rank_slice_starves_without_collapse } BOOST_AUTO_TEST_CASE(cpu_topology_policy_private_mask_collapses_even_when_the_split_would_fit) { - // 4 cores over 2 L3 domains: 2 ranks x 2 partitions fits exactly, so the collapse here is not a - // fallback after a refusal -- it changes which cores a private-mask rank is given. + // 2 ranks x 2 partitions fits these 4 cores, so the collapse is not a fallback: it moves rank 1's cores. const std::vector cores = {{0, 0}, {2, 1}, {4, 0}, {6, 1}}; - // Rank 1's share under the split: domain 1 only. const auto split = placement_order(cores, 2, /*group_index=*/1, /*group_count=*/2); BOOST_REQUIRE_EQUAL(split.size(), 2u); BOOST_CHECK_EQUAL(split[0], 2); BOOST_CHECK_EQUAL(split[1], 6); - // What mask_is_private now passes instead: the head of this rank's own interleave over both domains. + // What mask_is_private now passes: the head of this rank's own interleave over both domains. const auto collapsed = placement_order(cores, 2, /*group_index=*/0, /*group_count=*/1); BOOST_REQUIRE_EQUAL(collapsed.size(), 2u); BOOST_CHECK_EQUAL(collapsed[0], 0); @@ -277,8 +246,7 @@ BOOST_AUTO_TEST_CASE(cpu_topology_policy_private_mask_collapses_even_when_the_sp namespace { -/* Build the flat [n * words] array masks_are_pairwise_disjoint reads, from a list of PU-index - * lists -- the shape MPI_Allgather leaves behind. */ +// The flat [n * words] array MPI_Allgather leaves behind, built from per-rank PU-index lists. auto packed_masks(const std::vector> &pus, size_t words) -> std::vector { std::vector out(pus.size() * words, 0); for (size_t r = 0; r < pus.size(); ++r) { @@ -300,8 +268,7 @@ BOOST_AUTO_TEST_CASE(cpu_topology_masks_disjoint_vs_identical) { const auto identical = packed_masks({{0, 1}, {0, 1}}, kWords); BOOST_CHECK(!partition::masks_are_pairwise_disjoint(identical.data(), 2, kWords)); - // Partial overlap is pathological; "not private" is the conservative answer, because dividing - // never double-books a core within a rank whereas collapsing points every rank at the same ones. + // Partial overlap: "not private" is conservative, since collapsing points every rank at the same cores. const auto partial = packed_masks({{0, 1}, {1, 2}}, kWords); BOOST_CHECK(!partition::masks_are_pairwise_disjoint(partial.data(), 2, kWords)); @@ -309,11 +276,9 @@ BOOST_AUTO_TEST_CASE(cpu_topology_masks_disjoint_vs_identical) { const auto with_empty = packed_masks({{0, 1}, {}}, kWords); BOOST_CHECK(!partition::masks_are_pairwise_disjoint(with_empty.data(), 2, kWords)); - // A lone rank has nobody to be disjoint from, and the normal split already handles it. const auto lone = packed_masks({{0, 1}}, kWords); BOOST_CHECK(!partition::masks_are_pairwise_disjoint(lone.data(), 1, kWords)); - // More than two peers: one overlapping pair anywhere is enough to make the node shared. const auto four_ok = packed_masks({{0}, {1}, {2}, {3}}, kWords); BOOST_CHECK(partition::masks_are_pairwise_disjoint(four_ok.data(), 4, kWords)); const auto four_bad = packed_masks({{0}, {1}, {2}, {1}}, kWords); @@ -323,39 +288,26 @@ BOOST_AUTO_TEST_CASE(cpu_topology_masks_disjoint_vs_identical) { BOOST_AUTO_TEST_CASE(cpu_topology_masks_span_word_boundaries) { constexpr size_t kWords = partition::kAffinityMaskWords; - // Bits in different 64-bit words: a per-word comparison that forgot to loop would call these - // disjoint by luck on the first word and overlapping is what must be detected on the third. + // A per-word comparison that forgot to loop would answer from word 0 alone. const auto low_high = packed_masks({{5}, {200}}, kWords); BOOST_CHECK(partition::masks_are_pairwise_disjoint(low_high.data(), 2, kWords)); const auto both_high = packed_masks({{200}, {200}}, kWords); BOOST_CHECK(!partition::masks_are_pairwise_disjoint(both_high.data(), 2, kWords)); - // Overlap in a later word than the first difference. const auto late_overlap = packed_masks({{1, 3000}, {2, 3000}}, kWords); BOOST_CHECK(!partition::masks_are_pairwise_disjoint(late_overlap.data(), 2, kWords)); - - // The truncation guard itself -- a PU index past the exchanged window making - // affinity_mask_words report "cannot classify" -- needs a live hwloc mask and is not - // reachable from here. It is covered only by the all_ok reduction in - // PartitionGroup::classify_node_masks_, which needs a communicator. } BOOST_AUTO_TEST_CASE(cpu_topology_affinity_mask_covers_enumerated_cores) { - // The only check that the mask EXCHANGED and the cores PLACED come from the same view of the - // machine. affinity_mask_words otherwise has no test caller at all. + // The only check that the mask EXCHANGED and the cores PLACED come from one view of the machine. const auto cores = partition::enumerate_physical_cores(); if (cores.empty()) { return; // hwloc loaded no topology at all: there is no second view to agree with. } std::vector mine(partition::kAffinityMaskWords, 0); if (!partition::affinity_mask_words(mine.data(), mine.size())) { - /* NOT a free skip. enumerate_physical_cores just succeeded, so hwloc works here and the - * only licensed reason to refuse is the truncation guard: some allowed PU sits past the - * exchanged window. Assert that, or a mask function that had simply stopped working would - * read as "nothing to test" -- which is the failure mode this whole file exists to close. */ - // Refusal keys on the HIGHEST allowed PU while core.cpu is the LOWEST sibling of each core, - // so a re-read through a wider window is the only sound witness. + // Not a skip: refusal keys on the HIGHEST allowed PU, and core.cpu is each core's LOWEST sibling. std::vector wide(partition::kAffinityMaskWords * 64, 0); BOOST_REQUIRE(partition::affinity_mask_words(wide.data(), wide.size())); size_t highest = 0; @@ -383,16 +335,7 @@ BOOST_AUTO_TEST_CASE(cpu_topology_affinity_mask_covers_enumerated_cores) { namespace { -/* Confine this thread to the first `k` enumerated cores and assert the premise took hold. - * - * The premise is not free. effective_allowed_cpuset() re-reads hwloc_get_cpubind() on every call, - * so a successful sched_setaffinity DOES narrow what enumerate_physical_cores() reports -- but if - * it ever stopped doing so, the two cases below would keep asserting, against the UNCONFINED - * machine, and keep passing. That is the same defect as an early return, only harder to see, so - * the narrowing is checked rather than assumed. - * - * @returns false when the kernel refused the call (seccomp, a restrictive cgroup), which is the - * one condition here that no assertion can rescue. */ +// Confine to the first `k` cores, asserting the narrowing took hold; false only if the kernel refused. auto confine_to_first(const std::vector &full, size_t k) -> bool { cpu_set_t mask; CPU_ZERO(&mask); @@ -413,9 +356,7 @@ BOOST_AUTO_TEST_CASE(cpu_topology_per_rank_mask_still_places) { if (full.empty()) { return; // hwloc loaded no topology: there is no mask to confine to. } - /* ONE core is enough to exercise the collapse -- the request is group_count x n out of a list - * holding n either way. Requiring two would let this pass vacuously on a single-core runner, - * which is exactly the property that made the live case it was ported alongside worthless. */ + // ONE core exercises the collapse, so a single-core runner does not turn this into a free pass. const size_t k = std::min(full.size(), 2); const AffinityGuard guard; @@ -423,9 +364,7 @@ BOOST_AUTO_TEST_CASE(cpu_topology_per_rank_mask_still_places) { return; // the kernel refused the affinity call; nothing below is reachable } - // n = the whole share, and a group_count as if seven sibling ranks shared the node. This is - // the configuration `srun --cpu-bind=cores` produces, and without the collapse it places - // NOTHING: the request is group_count x n cores out of a list holding n. + // What `srun --cpu-bind=cores` produces: our whole share, told there are eight sibling ranks. const auto sets = partition::partition_cpusets(/*n=*/k, /*group_index=*/3, /*group_count=*/8, /*mask_is_private=*/true); if (sets.empty() && empty_placement_is_licensed()) { @@ -441,7 +380,6 @@ BOOST_AUTO_TEST_CASE(cpu_topology_per_rank_mask_still_places) { BOOST_CHECK(inside); } if (k == 2) { - // The two partitions must not land on the same core. BOOST_CHECK(sets[0].pu != sets[1].pu); } } @@ -451,8 +389,7 @@ BOOST_AUTO_TEST_CASE(cpu_topology_shared_mask_keeps_co_located_ranks_disjoint) { if (full.size() < 2) { return; // two ranks cannot hold disjoint cores when there is only one } - /* Two cores are enough: one per rank. The ported version asked for four, which a two-physical- - * core CI runner does not have -- it would have returned here and reported green. */ + // Two cores, one per rank: a two-physical-core CI runner cannot supply the four the port asked for. const size_t per_rank = full.size() >= 4 ? 2 : 1; const size_t shared = 2 * per_rank; @@ -476,8 +413,7 @@ BOOST_AUTO_TEST_CASE(cpu_topology_shared_mask_keeps_co_located_ranks_disjoint) { BOOST_REQUIRE_EQUAL(rank1.size(), per_rank); for (const auto &a : rank0) { for (const auto &b : rank1) { - // Two ranks sharing a core would have each one's busy-polling collectives starve the - // other's barrier spins -- the failure this whole placement path exists to avoid. + // Two ranks on one core starve each other: busy-polling collectives against barrier spins. BOOST_CHECK(a.pu != b.pu); } } From 655ea3c91fcd5cabe77dfe58be8423f93c3407a3 Mon Sep 17 00:00:00 2001 From: Aaron Miller Date: Wed, 19 Aug 2026 23:02:39 +0100 Subject: [PATCH 10/10] =?UTF-8?q?docs(partition):=20=E2=9C=8F=EF=B8=8F=20s?= =?UTF-8?q?tate=20both=20refusal=20widths=20in=20the=20@returns=20clause?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The private-mask arm needs n cores, the shared arm group_count x n; the clause named only the former. Co-Authored-By: Claude Opus 5 --- cpp/monoprop/detail/partition/CpuTopology.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/monoprop/detail/partition/CpuTopology.h b/cpp/monoprop/detail/partition/CpuTopology.h index 52ec0640..1928d2d1 100644 --- a/cpp/monoprop/detail/partition/CpuTopology.h +++ b/cpp/monoprop/detail/partition/CpuTopology.h @@ -117,7 +117,7 @@ auto affinity_mask_words(uint64_t *out, size_t nwords) -> bool; * @param mask_is_private True only when the co-located ranks' affinity masks have been measured * pairwise DISJOINT (PartitionGroup::classify_node_masks_), so this mask is our share. * @returns Vector of @p n CpuSet tokens, or empty when @c monoprop_PARTITION_PINNING is disabled, - * hwloc is unavailable, or not even @p n distinct cores are visible to this process. + * hwloc is unavailable, or fewer than @p group_count x @p n cores are visible (@p n when private). * * @note Under @p mask_is_private the group split is skipped: our share is already this rank's alone. */