feat: sm multiproc shared bucket#75
Open
coldzerofear wants to merge 24 commits into
Open
Conversation
…ucket
Multiple processes in one container each hold a private g_dev_hot[] token
bucket, so their concurrent launches can stack and briefly exceed the aggregate
SM limit by ~N x. Document the design for making the bucket container-shared:
- Move g_dev_hot[]'s cur_cuda_cores + controller integral state into a
MAP_SHARED region under /tmp (reusing the vmem region pattern, so it is
container-scoped and cross-container-isolated).
- The consumer CAS in rate_limiter is unchanged -- CAS is address-space
agnostic, so a static->shared move makes it a cross-process atomic decrement
for free.
- Replenishment uses a per-cycle CAS election on last_refill_ns (no leader, no
failure detection, self-healing) so N watchers do not over-supply N x.
- Keep last_launch_ns process-private (gap detection is per-process).
- Rewrite the anti-jitter bypass from a SET to an accumulate so it cannot wipe
concurrent consumer decrements (the subtlest correctness point).
No lock, no launch serialization -- strictly cheaper and safer than HAMi's
lock-serialization, and (unlike HAMi Event mode) aggregate-correct so the
sleeping-coordination is unnecessary. Env-gated (default off). Staged: measure
first (single-process containers get zero benefit), then the shared bucket, then
optional local token batching. Design only -- no code changes; multi-process
concurrency needs on-GPU validation.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Two review points, both of which changed the design.
Stale-state reset (was 4.5): the draft's generation concept was solving a
problem that does not exist. The shared dir is not the container's own /tmp --
the device plugin mounts it from <host_manager_dir>/<pod-uid>_<cont-name>, so
cross-pod inheritance is already impossible by construction. What does survive
(a container restart within a pod) is share/cur_cuda_cores/up_limit/is/
avg_sys_free -- all self-correcting feedback quantities that reconverge in a few
control cycles, not a ledger like vmem where a stale entry never heals. The
draft conflated the two.
Meanwhile generation does not catch the one real hazard: the host dir outlives a
container restart while the .so is mounted per-version, so a library upgrade can
map a new library onto an old struct. Replaced with a magic/layout_version/
region_size/device_count header guard plus a device fingerprint, which makes
first-create and upgrade-reinit the same path. ns st_ino {mnt,cgroup} (already
implemented at util.c:546) is kept only as an optional best-effort reset, noted
as such because nsfs inums come from an IDA and are reused -- a collision only
costs one skipped reset.
Atomics (new 4.10): volatile provides no atomicity or ordering -- today's
safety is entirely the CAS macro, so volatile is decorative and misleading;
the shared region drops it. _Atomic is rejected too: a non-lock-free _Atomic
degrades to libatomic's address-keyed lock table, which is per-process and would
silently break across mapped-shared memory, and _Atomic may alter size/align on
a struct that is an on-disk cross-process ABI. Use plain fixed-width types +
__atomic_* builtins with explicit per-site memory order, matching the existing
idiom (hook.h already hard-requires GCC/Clang + glibc).
Design only -- still no code changes, still gated on the stage-0 measurement.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Mount a dedicated per-container dir for the shared region instead of using the
container's own /tmp (which may be shadowed by another hostPath mount or be
read-only), and let the plugins wipe the cache file before each container start.
Named it sm_node (/tmp/.sm_node/sm_node.config), symmetric with vmem_node: the
latter is the cross-process state for memory isolation, this is the same for
compute. Naming it by mechanism (shm_node, vgpu_shm) was rejected because
vmem_node is itself a shared-memory region, so a mechanism name is ambiguous.
The key finding is that this mechanism is not new: PreStartContainer already
does exactly this for vmem_node.config ("Clean up old cache files before each
startup", vnum_plugin.go:1094, backed by PreStartRequired:true so kubelet calls
it before every container start including restarts), and the NRI CreateContainer
hook does the same (nri/plugin.go:387). sm_node.config just joins those two
existing cleanup blocks. Since the region is therefore always freshly zeroed at
attach, this deletes the ns st_ino probing from the previous revision outright --
it was reimplementing an existing and strictly more reliable guarantee.
Coverage is not total: DRA without NRI injects mounts via a CDI spec generated
at NodePrepareResources (per-claim, at admission), so no plugin code runs on a
container restart and there is no cleanup hook. Documented rather than patched,
because per 4.5.4 the residue is all self-correcting feedback state; only layout
mismatch is non-self-healing, and the magic/layout_version guard survives as the
second line of defense for exactly that path.
Split stage 1 into 1a (Go: mounts + cleanup, a pure no-op addition until the
library uses the dir, so it can land first and turn the control-plane assumption
into a verified fact) and 1b (library). Scope header now covers the Go packages.
Design only -- no code changes yet.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…erhead Three review points. Rebuild, don't error out. The draft said "reinit_region()" without saying how, which mattered because the existing vmem region gets this wrong: loader.c:1399 returns 1 on a size mismatch, so a library upgrade that changes the struct makes the container unusable on any path the control plane cannot clean. sm_node now specifies: a frozen 16-byte header ABI (magic/layout_version/region_size/ device_count, offsets pinned by _Static_assert and never allowed to move), a file size that is a permanent constant decoupled from sizeof(region) so a version bump never resizes, and in-place memset rebuild with magic published last under release order, making rebuild idempotent and interruptible. Any unrecoverable error degrades to the per-process bucket instead of failing -- multi-process isolation is an optimization, not a correctness precondition. In-place rebuild is safe without unlink/rename because a layout mismatch implies the file is from a previous container life: the .so mounts at a fixed container path, so every process in one container life loads one library version, and no live reader can hold the old layout. Avoiding rename also avoids its race, where two processes map different inodes and the shared bucket silently degrades into two private ones -- a worse failure than the one it would prevent. Scoped the rule, because two of the three size-mismatch sites must NOT change: mmap_file_to_config_path and mmap_file_to_util_path are MAP_PRIVATE/PROT_READ consumers of control-plane output that the library cannot rebuild, and resource_data_t's check is deliberately fatal -- CheckResourceDataSize under the Reschedule gate exists so the controller reschedules pods that cannot start. The rule is: rebuild regions the library owns; keep erroring on read-only inputs. Compatibility and overhead. Added a section listing what may be used (all already in the repo) against what is banned (memfd_create, O_TMPFILE, renameat2, statx), and reused lock.c's ofd_fcntl, which already falls back from OFD to classic POSIX locks at runtime, so serializing init needs no new primitive and raises no kernel floor. The init lock is justified explicitly: it is once per process under pthread_once, kernel-released on death, and never touched by the hot path -- unlike the HAMi shm mutex it does not deadlock a container when a holder dies. Overhead section states the hot path stays at one CAS and forbids adding any check to rate_limiter; the only real new cost is shared-cacheline bouncing, deferred to stage 2 and gated on profiling. Dropped the three-state initialized field (the init lock subsumes it). Design only -- no code changes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…rafted Checked the design against the controllers instead of assuming, and the earlier "only the token bucket moves, the algorithms are untouched" claim does not hold. There are three controllers, not two (delta/aimd/auto), and the per-cycle election changes which PROCESS runs the control step each cycle, which breaks any cross-cycle state the winner alone advances. delta is a pure function -- no cross-cycle state of its own -- so the election is transparent to it and it needs nothing. aimd holds g_aimd_md_cooldown, whose declaration states the invariant it relies on: "Watcher-thread-only access... No volatile / atomics needed." The election voids exactly that. With each process holding a private cooldown at 0, N consecutive winners each fire MD before any cooldown engages, cutting share by md_divisor^N (3^4 = 81x for N=4) -- which is precisely the "MD avalanche" the cooldown was added to prevent, per the comment at cuda_hook.c:846. Fixed by moving md_cooldown into the shared region, where the election serializes it and the semantics match single-process (the cooldown counts global cycles, which is its documented time-based intent). auto routes through the debounced exclusivity FSM, which carries the same thread-ownership assumption over three fields; lost_exclusivity_pending is the worst of them, a one-shot flag that a non-winner would leave armed until it finally wins, firing a stale reset cycles later. And all three controllers share throttled_since_watch, the bypass gate: with it private, a winner reads its own flag as 0 while another process is being throttled, admits the bypass, and the bypass SET wipes tokens the throttled process is still deducting. Sharing it turns its meaning from "did this process throttle" into "did anyone in the container throttle", which is what a shared bucket should have been asking. So the whole cross-cycle control block moves, not just the bucket. The algorithms themselves still change by zero lines -- only where their state lives -- but the change surface is larger than earlier revisions promised, so the risk is raised accordingly. Struct extended (72B, still one cacheline); per-cycle scratch (top_results, sys_frees, the exclusivity memo) stays private and is justified. Added section 11 listing what needs a decision, with the stage-0 measurement and the controller-coverage choice called out as blocking 1b. Design only -- no code changes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
All nine decisions folded in; the doc is now a定稿 rather than a proposal. Stage 0 was a gate; it is now an acceptance baseline. The reasoning stands on its own -- multiple processes contending for one GPU necessarily over-shoot, and notebook containers (one process per kernel, several kernels at once, kernels started and restarted at will) are the canonical case. So section 7 stops asking "should we do this" and starts asking "did it work and did it break anything", with concrete criteria: aggregate util converges toward the single-process curve, no regression with the switch off, no consecutive-cycle MD_FIRED (direct evidence md_cooldown really is shared), the degrade path runs, the rebuild path runs, and Go-side memory metrics match reality. All three controllers stay in scope, so the struct keeps md_cooldown and the exclusivity FSM. Recorded the consequence: those fields' comments currently say "watcher-thread-only / no atomics needed", which becomes false and actively misleading once they are shared -- the comments must move with the fields. vmem_node is in, and I withdrew my objection to it. I had argued vmem is a ledger so rebuilding loses accounting, but applying this design's own safety argument kills that: a layout mismatch implies the file is from a previous container life, so every record in it belongs to a dead PID. Rebuilding loses nothing real, while erroring out (today's loader.c:1399) makes the container unstartable. The decision was right. That inclusion has a consequence worth stating plainly: it creates the design's only cross-language ABI. The Go side mirrors the C struct byte-for-byte AND derives fcntl record-lock offsets from it. C needs no change there because offsetof absorbs the new header automatically -- but Go's getVmemoryLockOffset computes the offset within the devices array and must now add the array's base. Miss it and Go and C lock non-overlapping byte ranges: mutual exclusion silently evaporates and metrics go wrong with no error anywhere. It is the one change in this design that fails silently, so it is now a top risk, C+Go are pinned to one PR, and it has an explicit acceptance check. The fixed reserve turns out to matter more for vmem than for sm_node: it is not just headroom, it means never resizing, which rules out shrinking the file under the manager's live mmap and SIGBUS-ing it. Also documented that upgrade skew degrades to "metrics briefly missing until the container restarts" in both directions, that this is not new (today's size check already does it), and that moving the create-time memset under the OFD lock is a net robustness gain over today's unlocked one. Design only -- no code changes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…safe The init/rebuild path already passed wait=1 (F_OFD_SETLKW, blocking) but never justified it. Making it explicit per review: blocking is the point -- a late process should sleep in the kernel while the first one builds the region, not spin-retry and burn CPU. Added 4.4.2a contrasting it with lock_gpu_device's non-blocking spin-with-timeout: that one guards a hot, contended device lock whose holder runs real allocation work and could stall, so it needs a timeout ceiling; the init critical section is only ftruncate+memset+a few field writes on a few-KB file, has no call that can block indefinitely, so the holder cannot hang and no timeout is needed (adding one would just reintroduce the spin logic). Reconciled with the never-fatal rule: blocking on "another process is building" is a normal short wait, not the failure-degrade path (that's for open/mmap failure); and if the holder dies mid-section the kernel releases the lock so the waiter wakes and takes over an idempotent rebuild. Noted this matches the repo: ofd_fcntl(fd,1,...) blocking is already used for the vmem/sm-util record locks, ofd_fcntl(fd,0,...) non-blocking for the device spin lock. Also fixed two stale "stage 2" references that meant token-batching (now stage 3 after vmem_node took the stage-2 slot). Design only -- no code changes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… file/dir Clarify a boundary that was implicit: the init lock is a byte-range record lock placed on sm_node.config itself (one fd builds, locks, and mmaps the region), so there is no separate .lock file -- zero extra inodes, matching the existing vmem_node.config pattern. Record why the parent directory cannot be the lock target: fcntl F_WRLCK needs a write-opened fd and a directory cannot be opened O_RDWR (EISDIR), so ofd_fcntl can only ever take a shared read lock on it; flock(2) could lock a dir fd but is a new primitive the design deliberately avoids and mixing it with fcntl is a footgun. Whole-file lock is fine because sm_node has no per-device record locks at runtime (no Go reader needing a consistent snapshot). Design only -- no code changes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ket is not a ledger Re-verified every premise against main @ 2f234ef. The 37 commits merged in touch memory accounting, dlsym/cuGetProcAddress routing and NVLink topology -- all orthogonal to this design. The SM controller region is untouched, so no technical judgement in §1-§9/§11 changes. Recalibrated all 38 code references and recorded the survey in a new §0. Three things the sync did surface, each one a trap for whoever implements this: §10.6 -- vmem_node's "size mismatch means the container cannot start" is TWO layers, not one. Past the return 1 inside mmap_file_to_vmem_node, the caller carries a LOGGER(FATAL) that is what actually kills the process; changing only the inner layer changes nothing when open/mmap hard-fails. The region is also feature-gated now (default off), so acceptance criterion §7.2.6 passes vacuously unless the gate is turned on -- which is worse than failing. §4.5.5 -- main grew three stale-reclaim mechanisms for vmem_node (PID liveness sweep, per-cycle watcher check, atexit/signal cleanup). sm_node needs none of them, and copying them would be actively wrong: the per-cycle one takes a per-device write record lock, straight through the "no locks on the watcher path" constraint. The judgement is the ledger/self-correcting asymmetry §4.5.4 already draws, and it is pinned to sm_node having no per-PID field -- add one and all three conclusions lapse. §5.1 -- the repo now has a second switch idiom (resource_data_t field behind a control-plane feature gate). Picking a side explicitly: env, because widening resource_data_t trips the CheckResourceDataSize reschedule gate, and paying a vgpu.config ABI break for a default-off optimisation is not a trade. The upgrade path stays open -- both env injection points already exist, so the control plane can take over the switch without the library changing a line. Also confirmed the DRA path versions the .so the same way the device plugin does, which is what §4.5.4's in-place rebuild argument rests on; only the device-plugin half had been checked before. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…lugin path The cleanup joined configDirPath, naming <cont-dir>/config/vmem_node/vmem_node.config while Allocate mounts, and the metrics lister reads back, <cont-dir>/vmem_node/vmem_node.config -- vmem_node is a sibling of config/, not a child. The path named here has never existed, so os.RemoveAll returned nil and the container started on top of the previous incarnation's ledger. Introduced in 583df02 by reusing the pidsConfigPath line above it, where configDirPath IS correct. The NRI CreateContainer hook does the same cleanup and gets it right, via strings.TrimSuffix(inj.ConfigDir, util.Config), which is why only the device-plugin path is affected. Consequence of the stale ledger: records left by the previous container's processes stay charged. PIDs from a dead container do not exist in the new one, so the liveness sweep in rm_vmem_node_by_non_existent_device_pid does eventually evict them -- but only once a watcher cycle reaches that device, and until then the container runs against a memory limit that is already partly consumed by ghosts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…aths Purely additive: the library does not read this directory yet, so nothing observable changes. Landing it first is what lets stage 1b assume the directory is there -- and lets us verify on real nodes that it actually is, before any library code depends on it. Six provisioning sites, one per path that can start a vGPU container: device plugin EnsureDir + response.Mounts (Allocate) DRA / CDI GetPartitionMountContainerEdits DRA / NRI GetNRIPartitionInjection both DRA paths ensurePartitionDirectories NRI observe mountDestsOfInterest (logging only) Plus the two pre-start cleanups, so the library always attaches to a fresh zero region: PreStartContainer and the NRI CreateContainer hook. Both cleanup paths are built from the same base expression as the corresponding mount -- contDir and basePath, never configDirPath. That is the discipline the bug fixed in 78e8efc came from: sm_node/ is a sibling of config/, and joining the wrong base yields a path that never exists, which RemoveAll reports as success. Extended the two existing mount tests rather than adding new ones: the request-scoped test now requires the sm_node host path to be present, and the claim-common test requires it to be absent. Allocation state is per-container, so a claim-scoped sm_node mount would silently merge the buckets of every container sharing the claim -- the exact failure the partition split exists to prevent. Doc: recorded decisions 12-15 (vmem region 320KiB, no region merge, stage 3 deferred, FATAL downgrade split out), and corrected §4.5.2, which had asserted this cleanup was already working on both paths. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A container whose vmem_node directory is missing, read-only, shadowed by the workload's own /tmp, or paired with an older plugin currently gets LOGGER(FATAL) and dies. None of those conditions say the process cannot run correctly -- only that the extra accounting is unavailable. Two things had to hold before this was safe to change, and both do: g_device_vmem == NULL is already a supported state everywhere. Every dereference sits behind a NULL guard, including the three call sites of the two rm_vmem_node_by_* helpers that carry no guard of their own. The stronger evidence is that the VMemoryNode feature gate defaults to off, so NULL is the state most nodes already run in -- degrading lands in the default configuration, not in an untested one. Memory limiting survives. The check is used + vmem_used + request_size > total_memory and `used` comes from NVML, independent of this region. Losing the region zeroes vmem_used only, and that term exists solely to cover oversold/UVA allocations NVML cannot observe. The limit stays enforced; it stops counting what it can no longer see. Had the limit depended on the ledger outright, FATAL would have been the safer behaviour and this change would be wrong. Details worth not getting wrong: the degrade path must NOT unlock init_config_mutex -- the FATAL it replaces never reached DONE, whereas this does, and DONE unlocks. ret is cleared because a missing optional region is not a failure to load the configuration. The atexit/sigaction registration moves into the success branch: those handlers hand this process's ledger records back at exit, so with no ledger there is nothing to hand back and installing them would change the container's signal disposition for nothing. Leaves the neighbouring sm_watcher FATAL alone -- different region, different argument, not this change's business. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
N processes in one container each held their own g_dev_hot[].cur_cuda_cores,
so each could independently decide "tokens available, go" at the same instant.
The aggregate limit was a statistical average rather than an invariant. This
puts the bucket in a MAP_SHARED region so it becomes a physical one.
The consumer side barely changes, which is the whole point: a CAS is a CPU
instruction and does not care which address space the word lives in, so
rate_limiter keeps its exact deduction loop and just resolves the target
pointer once per launch. Hot-path cost is unchanged -- one CAS, one
predictable branch, no validation (the region is checked once at map time).
Refill is the part that needed real work. Every process runs a watcher, and if
they all refilled the container would be supplied N times over -- looser than
the bug being fixed. Each cycle a device is claimed by one process via CAS on
last_refill_ns. No leader, no liveness probe, no failover: a winner that dies
mid-cycle just misses one refill, and the next cycle's staleness check hands
the device to somebody else.
The controller state has to move with it. The election hands a device to a
different PROCESS each cycle, so any per-process integrator would advance at
1/N rate and fracture into N divergent controllers. AIMD's md_cooldown is the
sharp case: without it every winner sees its own never-advanced cooldown of 0,
MD re-fires each cycle and share collapses by md_divisor^N -- precisely the
avalanche the cooldown was added to prevent. Same for the exclusivity FSM
(lost_excl_pending is one-shot and would hang set in every non-winner) and for
throttled_since_watch, whose meaning correctly widens from "did this process
throttle" to "did anyone in this container throttle".
State moves by snapshot-in/publish-out around the winner's block rather than
by rewriting each access site, so the control algorithm stays textually
identical -- only where its state lives changes. share carries the
acquire/release pairing for the whole block.
Two things the design did not anticipate, found while implementing:
- up_limit must be seeded when the region is BUILT, not when a watcher
thread starts. Seeding it per-watcher lets a late-joining process reset
the container's already-converged limit every time it starts.
- change_token still clamps against the per-process g_total_cuda_cores.
Reading the shared copy would introduce an ordering hazard where a
transiently-zero ceiling clamps the bucket to 0 and throttles everything.
Every process derives the same value from the same device, so local is
both safe and correct; the shared field is observability only.
The anti-jitter bypass changes from a store to a delta applied through
change_token, because a store erases tokens other processes deducted between
the read and the write -- tokens conjured back into the bucket. The per-process
branch keeps the literal store: off must mean bit-for-bit the old behaviour.
Adds test_sm_node_shared, which needs no CUDA and no GPU, so the two
properties that cannot be established by reading code can run in ordinary CI:
3.2M concurrent cross-process deductions lose nothing, and 8 processes
hammering the election yield ~10 refills per 900ms rather than 8x that. Both
were checked against a negative control -- the first version of the deduction
test passed WITHOUT atomics, because 8 short-lived children never actually
overlapped; it needs a start barrier to contend at all.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
gap_effective_dc() took its soft-mode duty-cycle target from up_limits[], which is process-private. Once the bucket is shared the controller runs in one process per cycle, so a given process refreshes that array only on the cycles it wins -- roughly 1 in N, and a process that keeps losing can be arbitrarily far behind. The GAP path then throttles against a limit the container moved away from several cycles ago. Bounded today because every process wins its share of the elections. It stops being bounded the moment refill ownership becomes sticky rather than per-cycle, which is the direction this is heading: a standby would never refresh up_limits[] at all and would throttle on its start-up value forever. Fixing it now, since the read is wrong either way. Relaxed load, matching the lock-free plain read it replaces -- a duty-cycle target that is one cycle stale was always acceptable here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every process ran its own watcher calling nvmlDeviceGetComputeRunningProcesses
plus nvmlDeviceGetProcessUtilization every ~100ms per device. The local-driver
path already carries a comment saying frequent calls to that API legitimately
return NOT_FOUND -- so N processes multiply exactly the call rate the driver
handles worst, and N is largest in the notebook containers this design targets.
One process now samples per device and publishes the result; the rest read it.
Sampling ownership and refill admission are deliberately DIFFERENT mechanisms:
last_refill_ns CAS HARD. Bounds how often the bucket may be topped up.
Always in force.
byte-range lock SOFT. Decides who pays for NVML. If it cannot be taken,
is lost, or is somehow held twice, the worst outcome is
redundant sampling.
Correctness never rests on the lock, and that is the point rather than
defensiveness. A lock can be held by a process that is alive but wedged, which
no lock can express; with the CAS still gating refill, a standby that sees the
published sample go stale can sample and refill itself without risking
over-supply. Lose the lock file entirely and this degrades to the previous
per-cycle election.
Standby cost per cycle is one non-blocking fcntl plus one shared read, and no
NVML call. Takeover latency is at most one cycle -- identical to the per-cycle
election it replaces, so nothing regresses.
Locks are per-device (l_start = host_index, l_len = 1) because processes can
have disjoint CUDA_VISIBLE_DEVICES and there is no single owner for all
devices. This works because host_index is resolved by UUID, not by the
process's own device ordering -- the same property the shared region's
indexing already depends on.
The lock lives in its OWN file. The init lock may share sm_node.config because
it is dropped inside one function; this one is held for the process lifetime,
which inverts the trade-off: classic POSIX record locks (the documented
fallback for kernels without OFD) are dropped when the process closes ANY
descriptor for the file, and map_sm_node_region closes that very file during
init. It must also never be deleted while containers run -- unlink+recreate
gives a new inode, and locks on different inodes are not mutually exclusive.
fork() needed explicit handling, and both failure modes are easy to miss. An
OFD lock belongs to the open file description, which fork SHARES: the child
inherits ownership, and since child_after_fork resets g_init_set the child
respawns its own watchers, so both would sample. Worse in the other direction,
the description outlives the parent as long as the child holds the descriptor
-- so the lock survives the owner's death and no standby can ever take over,
silently voiding the "kernel releases it when the holder dies" property this
design leans on. child_after_fork now closes the inherited fd and clears
ownership; initialization() reopens under a guard SEPARATE from the region's,
since a forked child keeps the mapping but must not keep the descriptor.
Publishing the sample is not optional: without it every standby's top_results
would go permanently stale and N-1 processes would report bogus utilization
metrics -- a new bug introduced by centralising. Region layout goes to v2 for
the s_* fields and leader_pid; still exactly one cache line per device.
test_sm_node_shared gains two cases. One checks exclusion, per-device
independence, and takeover on owner death. The other ASSERTS THE FORK HAZARD
IS REAL -- it confirms a lock outliving its dead owner via an inherited
descriptor -- so the close in child_after_fork cannot later be removed as
redundant cleanup.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ot to fix Two things a newcomer cannot recover from the code alone. §5.0 is the evidence behind acceptance criterion §7.2.2. Every shared behaviour funnels through g_sm_node, so the audit is finite: all ten functional uses, each with the branch it takes when the switch is off. It also states the constraint that keeps the table true -- new shared behaviour must keep g_sm_node as the single gate, because §7.2.2 measures throughput and will not notice a code path that quietly bypasses it. §9.1 lists the boundaries that are real, deliberate, and will otherwise be rediscovered as bugs: mixed mode weakening the aggregate limit (the price of degrading instead of dying), soft mode's elastic headroom becoming container-wide rather than per-process, a leader whose sampling keeps failing falling back to N-way sampling, and the pre-existing stickiness of top_results.valid. Each with why it is not being fixed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…using to start C and Go in one commit, deliberately. getVmemoryLockOffset is computed by hand on the Go side, and if it disagrees with C the two take fcntl locks on non-overlapping byte ranges: each believes it holds the lock, mutual exclusion is gone, and the manager reports torn reads as valid metrics. Nothing errors. Splitting this across commits would guarantee a revision where the two sides disagree. The region gains the same 16-byte frozen header sm_node has, so a library that meets a file written by an older one can tell, and rebuild in place rather than returning 1 -- which, with the caller's FATAL (downgraded separately in f31457c), made upgrading the library enough to stop containers starting on any path without a pre-start cleanup hook. Discarding the ledger costs nothing there: a layout mismatch proves the file predates this container, so every record in it belongs to a process that no longer exists. File size becomes a fixed 320KiB, decoupled from sizeof. That is not just headroom: the manager keeps this file mmap'd, so a library that shrank the struct and truncated the file would leave the manager's mapping past EOF, and touching it is SIGBUS. A size that never changes deletes the failure class. Also removes the TOCTOU the old two-branch open had, where two processes could both conclude they created the file and both memset the mapping. Two things found while implementing. The init lock must take ONE header byte, not the whole file. This region, unlike sm_node, has per-device record locks (lowest offset 16516) and a host-side Go reader that takes them. A whole-file write lock would contend with every one of them, to exclude something that is only ever another initialiser. Byte 0 sits in the frozen header and is never a per-device lock target. The pre-existing TestGetVmemoryLockOffset encoded the headerless layout -- it reconstructed the C macro as i*stride + lockByte, correct only while the Devices base was zero. It failed immediately, which is the point: it measured the exact 128 bytes this change moves. Updated to keep its original intent (guarding the per-device stride regression) and added TestVMemoryLayoutMatchesC, which pins four values as literals measured from a C build. Deriving them from the Go structs would only prove Go agrees with itself. The negative control for that test was worthless on the first attempt: deleting `base +` made it fail with [build failed], the compiler objecting to an unused variable, so the test never ran. With base := 0 instead it fails properly, off by exactly 128. Same lesson as the concurrency test earlier -- a control that goes red can be red for a reason that has nothing to do with what it checks. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
VMemNodeFileSize, VMemNodeMagic and VMemNodeLayoutVersion are as much a cross-language contract as the offsets, and they fail the same silent way. If C changed the reserved file size or bumped the magic and this side did not, NewMmapDeviceVMemory would reject every region handed to it and per-container memory metrics would simply stop appearing -- no error logged above V(4), just absence. Asserting them against the hook.h literals turns that into a failing test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… the glob Two independent claims decided who refilled: the sampling lock (sticky) and the last_refill_ns CAS (re-contested every cycle). Symmetric thresholds meant the winner was decided purely by watcher phase, so a standby could take the refill and run the controller on the sample the owner published a cycle earlier, while the owner's fresh sample waited its turn. Never wrong -- the sample is a 1-second NVML average against 200-400ms of actuation lag -- but it made "which sample fed this refill" depend on process start order, which is a poor thing to reconstruct from a trace. The owner now claims at 1x the period and standbys at 2x, so steady state is "whoever sampled also refills, immediately" and standbys go back to being a backstop. Takeover is untouched: an owner that dies releases its lock, and a standby picks it up in sm_sampling_claim earlier in the same cycle, claiming at 1x from that moment. The predicate cannot be `g_sm_sampling_mine ? 1x : 2x`. With no lock file available nobody is the owner, so that form puts EVERY process on 2x and halves the container's refill rate -- the bucket starves, and it looks like nothing until throughput drops. It has to be `g_sm_lock_fd < 0 || g_sm_sampling_mine`, falling back to the symmetric per-cycle election when ownership does not exist as a concept. Also fixes a build break of my own making: test/CMakeLists.txt does file(GLOB "test_*.c") and compiles the results against the CUDA includes only, so a test in that directory including hook.h fails to configure with -DBUILD_TESTS=ON. Moved to test/nogpu/, which the non-recursive glob cannot see, and left a note at the glob so the next GPU-less test does not land in the same trap. Test [5] covers both failure modes, and needed two attempts to be worth anything. The owner is deliberately given the LATEST start phase: with the earliest phase it won on timing alone and the test passed even with the asymmetry deleted. It now fails both when the asymmetry is removed (a standby wins) and when everything runs at 2x (the rate collapses), each verified against a modified build. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The limit was fixed at three ~100ms periods, which assumed the watcher's per-device period is ~100ms. It is not. When per-device processing overruns its slot the loop stops sleeping to the absolute-time grid and falls back to a 10ms floor, so an iteration costs (processing + 10ms) and a device is revisited only every dev_count iterations. Slow NVML on a four-device batch puts a device's period past 400ms. Against a 270ms limit that is not an edge case, it is every cycle: standbys would permanently judge the sample stale, all of them would resume calling nvmlDeviceGetProcessUtilization, and the added load would slow the owner further -- a feedback loop whose fixed point is "centralisation delivers nothing", reached exactly when the machine is already struggling. The question a standby needs answered is whether the owner has STOPPED, not whether it is as fast as someone assumed. So the owner now publishes the interval it actually achieves and standbys scale to it: three of the owner's own intervals, floored at the old value so a fast owner stays responsive, and capped at 5s so one absurd interval -- a suspend, a clock artefact, seconds of scheduling starvation -- cannot park the limit somewhere it never fires again. The interval is not recorded across a change of ownership, where the gap measures the takeover rather than the cadence. Region layout goes to v3; the field fits the existing padding, so a device is still exactly one cache line. Test [6] asserts a slow-but-alive owner is trusted and a stopped one is still caught at either cadence, and states the regression directly: a limit fixed for a 100ms cadence rejects a 440ms owner, the adaptive one accepts it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…illiseconds
The previous version adapted the staleness limit but bolted a 270ms floor and a
5s ceiling onto it, two numbers with no derivation behind them. Replaced with a
single named base -- the watcher's DESIGN per-device period -- and everything
stated as a multiple of it:
nominal = 100ms, derived, not chosen: the loop sleeps 100ms/dev_count
before each device and visits dev_count devices per pass, so
a device is revisited every 100ms whatever the batch size.
owner cadence = clamp(published interval, 1x nominal, 10x nominal)
owner refill = 0.9x nominal
standby refill = 2 cadence beats ("the owner missed a beat")
staleness = 3 cadence beats ("the owner missed two")
The lower bound is nominal because an owner cannot beat the design cadence, and
because a not-yet-published interval reads as 0 and lands in the same branch --
one rule covers both the floor and the cold start, where treating an unknown
cadence as infinitely fast would leave standbys hair-trigger during startup. The
upper bound is 10x nominal because past that the watcher is missing its own
contract by an order of magnitude, and continuing to scale would let one
pathological interval park the limits where they never fire again.
Fixes the same latent flaw in a second place: the standby refill threshold was
also fixed, at 2x90ms. An owner legitimately cycling at 440ms would have had its
refills taken every 180ms, so "whoever samples also refills" silently held only
for fast owners. It is now measured in the same beats.
Separately, make run_tests_with_env.sh survive a failing nvidia-smi. Under
`set -o errexit` with `pipefail`, a non-zero exit from the UUID query killed the
script before the fallback a few lines below could run -- the fallback was dead
code, and the failure surfaced as a bare "Error 18" from make with nothing to
act on. It now reports the exit status, decodes the common ones (18 is
NVML_ERROR_LIB_RM_VERSION_MISMATCH: kernel module and userspace NVML disagree,
usually a driver upgrade without reloading nvidia.ko), and proceeds to the
fallback as intended.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…t as a skip Two problems, and the second is why the first stayed quiet. The mirror had drifted. Adding the frozen header to device_vmemory_t shifted devices[] down by a cache line and decoupled the file size from sizeof(), so this test's copy of the layout no longer described the region: it compared the file against sizeof(ledger_t) and found 327680 where it wanted 262272. The mirror now carries the same header, checks the file against the region's own permanent size, and validates magic/version/region_size/device_count -- which also covers what a size check cannot, a layout that changes without changing size, and makes a region caught mid-rebuild read as "not this layout" rather than as data. The reason that surfaced as SKIP rather than FAIL is a semantic bug worth fixing on its own. ledger_open() returned without setting g_ledger on a size mismatch, which is indistinguishable from the file being absent, and can_assert() reports absence as "needs VMEMORY_NODE_ENABLED=1" and skips. So "the feature is off", a legitimate configuration, and "this test can no longer read the region", a real defect, produced the same green-looking outcome. An ABI change turned eight assertions into skips and nothing in the summary demanded attention. The two states are now distinct: g_ledger_broken means present-but-unreadable and can_assert returns 1 for it, which the runner counts as a failure. The call sites propagate the value instead of collapsing everything to -1 (`if (can_assert() != 0) return -1` mapped a failure onto a skip). Verified by extracting the ledger logic and exercising it directly: absent skips (255), the old 262272 size fails, a zeroed header fails, a valid header runs, and a version bumped on its own still fails -- the last being the case the size check alone would miss. The mirror's sizeof, devices offset, per-device stride, file size, magic and version were each compared against hook.h. Also corrects test_utils.h, which stated that run_tests_with_env.sh sets VGPU_TEST_STRICT=1. That export is commented out, so the suite does not run strict and a skip is only as visible as whoever reads the summary. Left the export alone -- turning global strictness on is a separate decision -- and recorded the rule instead: report a defect as a failure where it is detected, reserve skip for a precondition that is genuinely absent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…es left behind BUILD_SM_NODE_TESTS defaulted to OFF and no target turned it on, so test_sm_node_shared was never built or executed by `make test`. Six checks that need no GPU -- cross-process CAS, the refill election, sampling ownership, the fork hazard, owner priority, adaptive staleness -- sat there reachable only by someone who already knew the cmake incantation. A test nobody invokes is a test that quietly rots, which is the failure mode this suite exists to prevent. build-tests now configures it, `make test-nogpu` runs it alone, and `make test` runs it before the CUDA suite: it takes seconds, needs no driver, and failing fast on an ABI or concurrency regression beats discovering it after the GPU tests have run. Verified that BUILD_TESTS=ON and BUILD_SM_NODE_TESTS=ON configure together on a host with no CUDA toolkit, where test/ self-skips and the GPU-less target still builds. Comments: two references to sm_sample_stale_limit outlived the rename to sm_owner_cadence_ns, and the test file's header still described two checks when there are seven. Both corrected -- a stale comment about a threshold function is exactly what sends the next reader looking for a symbol that is not there. Checked while reviewing and found clean: top_results.checktime is set and consumed inside a single get_used_gpu_utilization call and never read by the control block, so a standby loading a published sample without it carries no stale state. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.