From 3063ff35d64f18acf3d33f52839d427e283479c0 Mon Sep 17 00:00:00 2001 From: diego Date: Mon, 24 Aug 2026 21:38:26 -0300 Subject: [PATCH] gpu: fire the sampled probe before the batched add (#67) A sampled launch and its batched gpu_launch_v1 record are twins - same correlation, one carrying the launch, the other only the CPU stack the consumer staples onto it. The consumer joins them in either order, but the orders are not equally safe. Sampled first parks the stack in pendingStacks, where only the twin can claim it and any number of unrelated batches may pass. Batched first holds the launch in deferredLaunches, which the next batch of any other kind releases stackless - deliberately, since the timeline wants launches promptly - leaving the stack to park with nothing to join. Both producers added the launch to its batch and only then fired the sampled probe, so a launch that both filled the batch and was sampled put its own batched record on the wire inside that add(), with the exec batch of the same iteration landing between the twins. On the privileged gate: 58 sampled, 57 attached, 1 parked forever, and the accounting identity held exactly, so nothing was lost silently - only attributed. At the old fixed sampler stride the collision was arithmetically unreachable: batches hold 32 records, the sampler took one in 8, and a multiple of 8 is never 31 mod 32. #50's jittered stride draws gaps from [4,12], so a sampled ordinal eventually lands on a batch boundary. #50 did not cause this; it removed the arithmetic that was hiding it. Fire the probe before the batched add() in both shims. A record cannot be in a batch before add() puts it there, so no flush - on the launching thread, on the drain thread, or on a CUPTI worker - can carry the twin past the sampled probe, and sampled-first holds unconditionally. The records are unchanged and fully built before the probe fires: no ABI change, no sampler change, no consumer change. In the adapter this closes a thread race rather than a same-thread ordering bug: the exec batch is flushed by the CUPTI worker and the drain timer, never by on_launch, so the window between add() and the probe was small but real. The CUDA path measured PendingStacks:0 in every run because it kept missing that window, not because it could not hit it. Rejected: holding the deferred queue across a non-launch batch hands every exec to the sink ahead of its own launch for 32 launches per batch, and needs a number no arrival pattern justifies. Attaching late stacks via LaunchCache means re-emitting a launch the sink already has, breaking "one launch in, one launch out". deferredLaunches stays for foreign producers that may still emit batched-first; what it cannot be is lossless. Tests, all unprivileged: shim/stub/probe_order_test.cc drives the real producer and reads the wire ORDER of its probe fires by patching the sites with int3 and reading the trapped registers - the same bytes bpf_probe_read_user copies. At period 1 the batch-filling launch is sampled by construction, so it fails on the old producer on every machine: "2 of 64 sampled launches had their BATCHED record emitted before their sampled twin". At period 8 over 2048 launches it reported 7 of 253, the same order as the 24 of 250 the demo run recorded. TestBothShimsFireTheSampledProbeBeforeTheBatchedAdd pins the order against both shims' source, which is the only unprivileged guard the CUPTI adapter can have. TestStackSurvivesASplittingBatchWhenTheSampledRecordLeads is the positive counterpart of the parking test, in the consumer's own terms. The gate now asserts PendingStacks == 0 and StacksAttached == wantSampled at rest, and logs the whole identity line. The assertion that fails today is unchanged. --- .superpowers/sdd/issue-67-report.md | 297 +++++++++++++++++++++++++ gpuprobe/batch_size_test.go | 38 ++++ gpuprobe/consumer_test.go | 119 +++++++--- gpuprobe/gate_test.go | 29 +++ gpuprobe/sampledstacks.go | 78 ++++--- shim/Makefile | 17 +- shim/nvidia/cupti_adapter.cc | 24 +- shim/stub/probe_order_test.cc | 329 ++++++++++++++++++++++++++++ shim/stub/stub.cc | 45 +++- 9 files changed, 893 insertions(+), 83 deletions(-) create mode 100644 .superpowers/sdd/issue-67-report.md create mode 100644 shim/stub/probe_order_test.cc diff --git a/.superpowers/sdd/issue-67-report.md b/.superpowers/sdd/issue-67-report.md new file mode 100644 index 00000000..b3577768 --- /dev/null +++ b/.superpowers/sdd/issue-67-report.md @@ -0,0 +1,297 @@ +# Issue #67 — a sampled stack is lost when its launch fills the batch + +Branch `fix/sampled-stack-join-race`, one commit, closes #67. + +## Cannot verify + +**I have `CapEff: 0`. I did not run the privileged gate, and I have no GPU.** +Nothing below that describes `TestStubDrivesThePipelineToPprofWithoutAGPU` +running, or the CUDA path running, is an observation. The numbers I predict +for both are derived, and they are stated so they can be falsified by one run. + +What I *did* run is in "Verification" at the end: the whole unprivileged +suite, plus a new test that reproduces the defect's mechanism with no +capabilities at all and fails against unfixed code. + +## The fix + +Both producers now fire the unbatched `gpu_launch_sampled_v1` probe **before** +the batched `add()`, instead of after it. + +- `shim/stub/stub.cc` — `gpu_launch_sampled_v1_emit(...)` moved above + `lb.add(l)`. The `gpu_launch_v1` record is still fully built first, so the + sampled record still copies `l.kernel_id` from it; only the two probe fires + swapped places. `sampler.should_sample()` remains the left operand of the + `&&`, so the schedule is still advanced on every launch whether or not a + consumer is attached. +- `shim/nvidia/cupti_adapter.cc` — the same move above `g_lb->add(l)`, after + `l.correlation = correlate_launch(...)` has run, since the sampled record + reuses that correlation. + +No ABI change: `sample_period`, `launch_seq` and every record layout are +untouched, the probe set is untouched, and the consumer is untouched. + +### Why this closes it rather than narrowing it + +The two records are twins — same correlation, one carrying the launch, the +other carrying only the CPU stack the consumer staples onto it. The consumer +joins them in either order, but the orders are not equally safe: + +- **sampled first** — the stack parks in `pendingStacks`, where nothing but + the twin can claim it, and any number of unrelated batches may pass. +- **batched first** — the launch waits in `deferredLaunches`, and the first + batch of any other kind releases it stackless. The stack then parks with + nothing to join. + +Batched-first was reachable because the launch record entered the batch before +the sampled probe fired: a launch that both *filled* the batch and was +*sampled* put its own batched record on the wire inside that `add()`, with the +exec batch of the same loop iteration landing between the twins. + +With the probe first, batched-first is **unreachable, not rare**. A record +cannot be in a batch before `add()` puts it there, so no flush — on the +launching thread, on the drain thread, or on a CUPTI worker — can carry the +twin past a probe that has already fired. The guarantee holds without any +assumption about threading or about flush timing, which is what distinguishes +it from a narrowing. + +### The four questions asked before implementing + +1. **Does anything depend on the batched record being queued first?** No. + `Consumer.applyBatch` registers a PID with the walker on *any* batch + (`c.unwind.note(b.PID)`), sequence numbers are per-probe and unaffected by + cross-probe order, and `attachSampledStackLocked` reaches nothing the + launch path establishes. In practice the first record either producer emits + was already the unbatched `gpu_kernel_name_v1`, and the launch batch does + not flush until 32 records — so the sampled record was already ahead of the + first launch batch on every run. This changes only which of the two twins + leads, and only at a batch boundary. +2. **Can the sampled record reference state the batched path establishes?** + Only within the same call: the stub's `sl.kernel_id` reads `l.kernel_id`, + and the adapter's `s.correlation` reads `l.correlation` from + `correlate_launch`. Both are *struct fills*, not `add()`. Both stay above + the probe; `add()` is what moved. +3. **Sampled probe fires, process dies before the batch flushes.** Same + exposure as before, in exactly the same set of cases: the stack parks and + is counted in `PendingStacks`, the launch is lost with the unflushed + partial batch, and the producer's `launch_dropped` counts it. That was + already true for the ~31 launches that can sit in a partial batch behind + any sampled record — the reorder does not widen the window, because the + twin it concerns is emitted in the same `add()` that would have flushed it. + Both producers flush on the normal exit path (`perfagent_stub_run` before + lingering, `at_exit_handler` for the adapter); only a `SIGKILL` loses the + partial batch, before and after. +4. **Is the same reorder correct for the CUPTI adapter?** Yes, and it is + slightly *more* necessary there than the issue implies. In the stub the + collision is same-thread and deterministic. In the adapter the exec batch + is flushed by the CUPTI worker thread and by the drain timer, never by + `on_launch`, so the interleave was a genuine thread race: `g_lb->add(l)` + flushes a batch ending in launch N, and before the sampled probe two + instructions later another thread flushes `g_eb`. Small window, real + window, nothing making it impossible — which is why "unaffected in every + run measured" is the right description of the CUDA path and "structurally + safe" was not. Post-reorder it is structurally safe. + +## What I rejected + +- **Hold the deferred queue across a non-launch batch.** This is the one the + comment in `sampledstacks.go` calls deliberate, and it is right to. In the + stub the exec batch for correlations 1..32 follows the launch batch for + 1..32 immediately; holding launches past it means every exec is handed to + the sink before its own launch, for 32 launches per batch, for the whole + run. It also needs a number — hold for one batch? two? — and a kernel-name + batch or a second exec batch arriving first defeats whatever number is + chosen. It buys a rare attribution with a systematic delay and a heuristic. +- **Attach late stacks via `LaunchCache`.** The consumer talks to a + `gpu.EventSink` interface and does not own a `LaunchCache` — `Timeline` does. + Reaching it means either widening the sink interface with a mutate-in-place + method, or re-emitting the launch (`Timeline.EmitLaunch` → `cache.Put` + replaces on correlation, so it would technically work). Both break "one + launch in, one launch out", which is asserted in + `TestSampledStackArrivingFirstAttachesToTheBatchedLaunch` and relied on by + every counting sink; re-emission also races `Snapshot`. The parent called + this "reaching back into already-emitted state" and that is exactly what it + is. +- **A consumer-side fix in general.** I could not find one that is lossless + without either delaying launches or re-emitting them, and I now think none + exists: once the launch has gone to the sink there is nothing left to attach + to. This is why the deliverable does **not** contain the consumer-hostile- + order test the brief asked for — such a test can only pass if the consumer + changes, and I am arguing the consumer should not. See "Testing without + privilege" for what I wrote instead, which tests the same mechanism and + fails against unfixed code just as required. + +`deferredLaunches` stays exactly as it is. The ABI is public, and an older or +third-party producer may still emit batched-first; for those the queue is the +difference between "usually joins" and "never joins". What it cannot be is +lossless, and `TestStackParksUnattachedWhenAnotherBatchSplitsTheTwins` now +documents that as a statement about foreign producers rather than about ours. + +## Testing without privilege + +`shim/stub/probe_order_test.cc` — new, wired into `make -C shim test`. + +It uses the trick `core/probe_args_test.cc` already established: read the +binary's own `.note.stapsdt` to find the probe sites, patch their one-byte +nops with `int3` (which is all a uprobe does), and read `%rdi`/`%rsi` out of +the trapped context in the `SIGTRAP` handler — the same bytes +`bpf_probe_read_user` copies in `bpf/gpu_usdt.bpf.c`. No CAP_BPF, no consumer, +no GPU. + +It links the real `stub/stub.cc` (with `PERFAGENT_STUB_NO_MAIN`), arms all +four semaphores, calls `perfagent_stub_run`, stamps every probe fire with a +global tick, and asserts: **for every sampled launch, the sampled record's +tick is strictly less than the tick of the launch batch carrying that same +correlation.** Two passes: + +- `period=1 launches=64` — every launch is sampled, so the launch that fills + the 32-record batch is sampled *by construction*. This pass fails on + unfixed code on every machine, every run; it does not depend on the + sampler's schedule. +- `period=8 launches=2048` — the shipped configuration, with enough sample + points for the jittered schedule to land on a batch boundary the way the + gate did. One-directional: it can detect a violation, never invent one. + +Both passes refuse to pass vacuously (a full batch must have flushed, and at +least one sampled launch must have been seen). + +### Pre-fix failure output (verbatim, against `origin/main`'s producer) + +``` +stub: launches=64 observed=64 sampled=64 period=1 seed=0x9e3779b97f4a7c15 launch_dropped=0 exec_dropped=0 enroll=disabled +period=1 launches=64: 2 of 64 sampled launches had their BATCHED record emitted before their sampled twin (first: correlation 32, batched at tick 32, sampled at tick 33). + The consumer holds that launch in deferredLaunches, the exec batch of the same loop iteration releases it stackless, and the stack parks in pendingStacks with nothing to join (issue #67). + Fire the sampled probe BEFORE the batched add(). +stub: launches=2048 observed=2048 sampled=253 period=8 seed=0x9e3779b97f4a7c15 launch_dropped=0 exec_dropped=0 enroll=disabled +period=8 launches=2048: 7 of 253 sampled launches had their BATCHED record emitted before their sampled twin (first: correlation 32, batched at tick 4, sampled at tick 5). + The consumer holds that launch in deferredLaunches, the exec batch of the same loop iteration releases it stackless, and the stack parks in pendingStacks with nothing to join (issue #67). + Fire the sampled probe BEFORE the batched add(). +EXIT=1 +``` + +7 of 253 at 2048 launches is the same order as the demo run's 24 of 250 at +2000 that `sampledstacks.go` recorded, reproduced unprivileged. Note that +correlation 32 — ordinal 31, the first batch boundary — *is* in the jittered +schedule at period 8, which is precisely what the fixed stride made +impossible. + +### Post-fix + +``` +probe_order_test: period=1 launches=64 ok - 64 sampled launches, all emitted before their batched twin; 64 launch records in 2 full batches +probe_order_test: period=8 launches=2048 ok - 253 sampled launches, all emitted before their batched twin; 2048 launch records in 64 full batches +``` + +### Two more guards + +- `TestBothShimsFireTheSampledProbeBeforeTheBatchedAdd` + (`gpuprobe/batch_size_test.go`) pins the emit-before-add order against the + *source* of both shims. It is a regex over C++ and it is a weak instrument; + it is there because the CUPTI adapter cannot be driven without a CUDA + process and a GPU, so for that file it is the only unprivileged guard there + is. Pre-fix output: + + ``` + --- FAIL: TestBothShimsFireTheSampledProbeBeforeTheBatchedAdd (0.00s) + Error: "5176" is not less than "4285" + Messages: ../shim/stub/stub.cc: the launch is added to its batch before the + sampled probe fires. A launch that fills the batch then reaches the consumer + ahead of its own stack, is released stackless by the next exec batch, and the + stack parks in PendingStacks forever (issue #67) + ``` + +- `TestStackSurvivesASplittingBatchWhenTheSampledRecordLeads` + (`gpuprobe/consumer_test.go`) is the positive counterpart of + `TestStackParksUnattachedWhenAnotherBatchSplitsTheTwins`: the same batch + boundary and the same splitting exec batch, in the order the fixed producers + emit. It asserts the stack attaches and `PendingStacks == 0`, in the + consumer's own vocabulary. + +## Gate changes + +`gpuprobe/gate_test.go`, in `TestStubDrivesThePipelineToPprofWithoutAGPU`: + +- `assert.Zero(t, stats.PendingStacks, ...)` — the assertion #67 asks for. + Checked at rest, after `Run` has returned; `PendingStacks` is not drained by + `Flush`, so this is exactly "a resolved stack with no launch left to join". +- `assert.Equal(t, uint64(wantSampled), stats.StacksAttached, ...)` — the same + fact from the counter side. The existing timeline-side assertion + (`sampledLaunches == wantSampled`, the one that fails today) is **unchanged** + and stays. +- A `t.Logf("stack attach: ...")` printing the full identity line, so + `58 = 57 + 0 + 0 + 1` is visible whether or not the assertions pass. + +Nothing was weakened. `wantSampled` is still 58, the sampler is untouched, and +`#50` is untouched. + +## Predicted gate output — derived, not observed + +`sudo -E ... go test -run TestStubDrivesThePipelineToPprofWithoutAGPU ./gpuprobe/` +should print and assert: + +``` +stack attach: sampled=58 resolved=58 attached=58 evicted=0 profiler-only=0 pending=0 missing=0 uncorrelated=0 +``` + +- `Stats.SampledLaunches` = **58** — unchanged; the sampler, its seed and its + schedule are untouched, and the stub's own `sampled=58` still agrees. +- `Stats.StacksAttached` = **58** (was 57). +- `Stats.PendingStacks` = **0** (was 1). +- `sampledLaunches` counted over `snap.Executions` = **58**, so the assertion + that fails today (`Not equal: expected 58, actual 57`) passes. + +Derivation: every sampled record now precedes its batched twin, so every +resolved stack parks and every parked stack is taken by the launch batch that +follows. At most one batch-window's worth of stacks (~32/8 ≈ 4) is parked at +any instant, far under `SampledStackCapacity`, so `StacksEvicted` stays 0. The +stub flushes both batches before it lingers and the gate sleeps 500ms after +the producer exits, so every launch batch is consumed before `Stats()` is +read — nothing is legitimately still in flight. + +Everything else on the gate is unchanged and should read as it does today: +`500 executions, all exact; 500 launches, all matched; cache 500 live`, +`dwarf=58 fp-only=0 reached-root=58`, both kernels represented in +`stacks per kernel`, `launch_dropped=0 exec_dropped=0 enroll=confirmed`. + +**If `PendingStacks` reads non-zero after this**, the cause is not the twin +order — `probe_order_test` rules that out unprivileged — and the log line +names the remaining suspects directly. + +## Predicted CUDA-path numbers — derived, not observed + +The adapter change is the mirror of the stub's and can only remove a join +failure, never add one. So the nine measured runs' numbers must not move: + +- `PendingStacks` = **0**, as before. +- `StacksAttached == SampledLaunches` = **505 == 505** at 4000 launches, + period 8 (the sampler pin in `shim/core/sampler_test.cc` prints exactly + `4000 launches at period 8 -> 505 sampled`). +- `launch_batch_dropped`, `exec_batch_dropped`, `exec_no_clock`, + `exec_no_time`, `cupti_dropped` — unchanged; nothing on those paths was + touched. +- Join quality, clock fit, kernel names, correlation epochs — unchanged. + +The one thing that could move is timing: the uprobe trap for a sampled launch +now happens a few instructions earlier inside `on_launch`, before the batch +mutex rather than after it. That shortens the interval during which +`g_lb`'s mutex is held on the sampling thread, if anything; it does not add +work. + +`make -C shim nvidia` builds clean against CUDA 13.3 here. That is a compile, +not a run. + +## Verification (all run, all green) + +``` +make -C shim OK +make -C shim test OK (includes the new probe_order_test) +make -C shim check-fpless OK +make -C shim nvidia OK (CUDA 13.3; compile only) +go build ./... && go vet ./... OK +go test ./gpu/ ./gpuprobe/ ./internal/... -count=1 all ok +go test ./gpu/ ./gpuprobe/ -race -count=4 all ok +golangci-lint run --timeout=5m 0 issues +``` + +`TestStubDrivesThePipelineToPprofWithoutAGPU` **skipped** in every one of +those runs, for want of `cap_bpf`/`cap_perfmon`/`cap_checkpoint_restore`. diff --git a/gpuprobe/batch_size_test.go b/gpuprobe/batch_size_test.go index c3e83d22..c156d37f 100644 --- a/gpuprobe/batch_size_test.go +++ b/gpuprobe/batch_size_test.go @@ -4,6 +4,7 @@ import ( "os" "regexp" "strconv" + "strings" "testing" "github.com/stretchr/testify/require" @@ -124,6 +125,43 @@ func TestSampledProbesAreNotBatchedInTheStub(t *testing.T) { } } +// Issue #67, for the producer no unprivileged test can execute. +// +// The sampled record and the batched gpu_launch_v1 record for one launch are +// twins, and the consumer's join is only safe when the sampled one leads: +// batched-first leaves the launch in deferredLaunches, where the next batch +// of any other kind releases it stackless, and the stack then parks with +// nothing to join (sampledstacks.go). Both shims therefore fire the sampled +// probe BEFORE the batched add(). +// +// For shim/stub/stub.cc that order is proven rather than asserted - +// shim/stub/probe_order_test.cc patches the probe sites with int3 and reads +// the wire order back, and it fails on the pre-#67 producer. The CUPTI +// adapter cannot be driven without a CUDA process and a GPU, so the same +// fact is pinned against its source here. A regex over C++ is a weak +// instrument; it is here because the alternative for this file is nothing at +// all, and because a reorder is exactly the kind of edit that looks harmless +// in review. +func TestBothShimsFireTheSampledProbeBeforeTheBatchedAdd(t *testing.T) { + for _, tc := range []struct{ file, add string }{ + {"../shim/stub/stub.cc", "lb.add(l);"}, + {"../shim/nvidia/cupti_adapter.cc", "g_lb->add(l);"}, + } { + b, err := os.ReadFile(tc.file) + require.NoError(t, err) + src := string(b) + emit := strings.Index(src, "gpu_launch_sampled_v1_emit(&") + batched := strings.Index(src, tc.add) + require.Positivef(t, emit, "%s: no unbatched sampled-launch emit found", tc.file) + require.Positivef(t, batched, "%s: no %q found", tc.file, tc.add) + require.Lessf(t, emit, batched, + "%s: the launch is added to its batch before the sampled probe fires. "+ + "A launch that fills the batch then reaches the consumer ahead of its own "+ + "stack, is released stackless by the next exec batch, and the stack parks "+ + "in PendingStacks forever (issue #67)", tc.file) + } +} + // The sampled-launch cap is 1, and not because of bytes: the batch header // holds one stack id, so a batch of N sampled launches would attribute one // captured stack to N unrelated launches. Its byte budget would allow 54. diff --git a/gpuprobe/consumer_test.go b/gpuprobe/consumer_test.go index d17f82c1..109404bc 100644 --- a/gpuprobe/consumer_test.go +++ b/gpuprobe/consumer_test.go @@ -1616,11 +1616,11 @@ func TestSampledStackArrivingFirstAttachesToTheBatchedLaunch(t *testing.T) { assert.True(t, sm.wasDeleted(5), "a consumed stackmap entry must be freed") } -// The other arrival order, which happens whenever the launch that fills the -// shim's batch is also the one the sampler picked: the flush is queued -// inside the add() that precedes the sampler check, so the batched record -// reaches the ringbuf first. The launch waits for the twin already on its -// way, then goes out once - with its stack. +// The other arrival order. No shim in this repository produces it any more - +// they fire the sampled probe before the batched add(), see issue #67 and +// sampledstacks.go - but the ABI is public and a foreign or older producer +// may still emit batched-first, so the consumer must join it. The launch +// waits for the twin, then goes out once, with its stack. func TestBatchedLaunchArrivingFirstWaitsForItsStack(t *testing.T) { sink := &recordingSink{} c, sm, _ := stackConsumer(t, sink, Config{}) @@ -1753,36 +1753,35 @@ func TestHeldLaunchDoesNotTakeAnotherProcessesStack(t *testing.T) { "the pid-4242 launch was released before its own stack arrived: no stack is correct, a borrowed one is not") } -// The measured cause of the unattached stacks in cmd/gpu-stub-profile's -// 2000-launch run (PendingStacks:24 of 250 captured), reproduced here with -// no privileges and no attach - just the batch order the stub actually -// produces. +// What a batched-first producer costs, and the reason no shim here is one. // -// The stub adds to the launch batch, then to the exec batch, then fires the -// unbatched sampled probe. When one launch is both the record that FILLS -// the launch batch and the one the sampler picks, all three land on the -// ringbuf in that order: launch batch, exec batch, sampled record. The -// launch is held for its twin; the exec batch releases it stackless (which -// is correct - the timeline needs launches promptly); and the twin then -// arrives with nowhere to go and parks forever. +// Three records land in this order: launch batch, exec batch, sampled +// record. The launch is held for its twin; the exec batch releases it +// stackless (which is correct - the timeline wants launches promptly); the +// twin then arrives with nowhere to go and parks forever. // -// With 32-record batches and a period of 8 those two conditions are -// disjoint by arithmetic - the batch fills at launch i = 0 mod 32 and the -// sampler picks i = 1 mod 8 - which is why a short run never shows it. What -// breaks the arithmetic is the stub's Drainer: it flushes both partial -// batches every 100ms, which RE-PHASES the batch boundary to wherever the -// loop happened to be. Roughly one tick in eight leaves the new boundary on -// a sampled launch, and from then until the next tick every one of the -// remaining fills - one per 32 launches - loses its stack this way. That is -// the whole rate dependence: a 500-launch run has about one tick and -// usually shows none, a 2000-launch run has several and showed 24. +// This WAS the shipped stub. It added to the launch batch, then to the exec +// batch, then fired the unbatched sampled probe, so a launch that both +// FILLED the batch and was sampled produced exactly this sequence: 58 +// sampled, 57 attached, 1 parked, on the privileged gate (issue #67). Issue +// #50's jittered stride is what made a sampled ordinal able to land on a +// batch boundary at all - at the old fixed stride of 8 the collision was +// arithmetically impossible against 32-record batches. // -// It costs attribution only. The launch ships, the execution ships, the GPU -// time is measured and projects as unattributed, and the stack is counted -// in PendingStacks rather than vanishing. Fixing it would mean holding -// launches past the next batch, which trades a rare attribution gain for a -// systematic delay in launch delivery that the timeline's join depends on - -// a worse trade, so this is documented and counted, not "fixed". +// The fix is in the producer: both shims now fire the sampled probe BEFORE +// the batched add(), which makes sampled-first unconditional, and +// shim/stub/probe_order_test.cc pins that order without any privilege. This +// test therefore no longer describes our producers. It stays because the +// ABI is public and a foreign or older producer may still emit in this +// order, and because what the consumer does then must be a documented, +// counted outcome rather than a surprise: the launch ships, the execution +// ships, the GPU time is measured and projects as unattributed, and the +// stack is counted in PendingStacks rather than vanishing. +// +// It is not fixable on this side without giving something up. Holding the +// launch past the exec batch delays every launch systematically to buy back +// a rare attribution, and attaching the stack after the fact means +// re-emitting a launch the sink has already been given. func TestStackParksUnattachedWhenAnotherBatchSplitsTheTwins(t *testing.T) { sink := &recordingSink{} c, sm, _ := stackConsumer(t, sink, Config{}) @@ -1818,6 +1817,62 @@ func TestStackParksUnattachedWhenAnotherBatchSplitsTheTwins(t *testing.T) { assert.Zero(t, st.StacksEvicted, "nothing was pushed out; it is still parked") } +// The same batch boundary, in the order the fixed producers actually emit +// (issue #67): the sampled record for the launch that fills the batch goes +// out BEFORE the batched add(), so it reaches the ringbuf ahead of the batch +// that carries its twin. +// +// Correlation 7 is the record that fills the launch batch here, and the exec +// batch of that same producer loop iteration follows immediately - the batch +// that released the launch stackless in the test above. The stack is parked +// rather than held, so it is not the deferred queue's to release, and the +// join survives the exec batch untouched. That is the whole difference the +// reorder buys, stated in the consumer's own vocabulary rather than the +// producer's; shim/stub/probe_order_test.cc is what pins the producer to +// this order. +func TestStackSurvivesASplittingBatchWhenTheSampledRecordLeads(t *testing.T) { + sink := &recordingSink{} + c, sm, _ := stackConsumer(t, sink, Config{}) + sm.put(1, 0x1000) + + // Sampled first: the producer fires this probe before the add() that + // flushes the batch below. + apply(t, c, sampledBatchWith(4242, 7, 1, 8)) + require.Equal(t, 1, c.Stats().PendingStacks, "the stack waits for its twin") + + // The batch the twin fills, then the exec batch of the same iteration. + apply(t, c, launchBatchWith(4242, 5, 6, 7)) + execs := make([]byte, batchHdrSize+gpuabi.SizeExec) + putU32(execs[0:], kindExec) + putU32(execs[4:], 1) + putU32(execs[16:], 4242) + putU64(execs[24:], gpuabi.SizeExec) + putU64(execs[batchHdrSize:], 7) + apply(t, c, execs) + c.Flush() + + st := c.Stats() + assert.Equal(t, uint64(1), st.StacksResolved) + assert.Equal(t, uint64(1), st.StacksAttached, + "the launch that filled the batch must still carry its stack") + assert.Zero(t, st.PendingStacks, + "nothing may be left parked: the exec batch cannot take a stack, only a held launch") + assert.Zero(t, st.StacksEvicted) + require.Len(t, sink.launches, 3) + // By correlation, not by position: a launch that collects a parked stack + // is emitted on the spot while its batch-mates are still held, so + // correlation 7 overtakes 5 and 6 here. admitLaunchLocked documents that + // reordering and bounds it to one batch. + byCorr := map[string][]string{} + for _, l := range sink.launches { + byCorr[l.Correlation.Value] = frameNames(l.Launch.CPUStack) + } + assert.Equal(t, []string{"fn_1000"}, byCorr["7"], + "correlation 7 is the sampled one and must carry its own stack") + assert.Empty(t, byCorr["5"], "an unsampled batch-mate must stay stackless") + assert.Empty(t, byCorr["6"], "an unsampled batch-mate must stay stackless") +} + // A held launch is waiting for a record that would be the next ringbuf // sample. Anything else arriving ends the wait: launches must not sit // behind an exec batch, because the timeline joins executions against diff --git a/gpuprobe/gate_test.go b/gpuprobe/gate_test.go index e25fbfed..76d890c3 100644 --- a/gpuprobe/gate_test.go +++ b/gpuprobe/gate_test.go @@ -331,6 +331,35 @@ func TestStubDrivesThePipelineToPprofWithoutAGPU(t *testing.T) { assert.Zero(t, stats.StackWalkScratchFailed, "a per-CPU scratch lookup at key 0 cannot fail on a loaded program") assert.Zero(t, stats.StacksEvicted, "the parked-stack side table must never overflow at this launch rate and capacity") + // Issue #67, and the assertion that keeps it from regressing silently. + // Every sampled record now reaches the ringbuf before the batched twin it + // belongs to (shim/stub/stub.cc fires the probe before the batched add(), + // pinned unprivileged by shim/stub/probe_order_test.cc), so every parked + // stack has a launch still to come and the run is flushed by the time + // this reads. A stack left parked at rest is a stack whose twin was + // emitted before it and released stackless: attribution lost, silently + // except for this gauge. It read 1 of 58 on main at 75ecc513. + // + // Checked before StacksAttached below because it is the more specific + // failure: a shortfall in attached stacks could come from a dozen places, + // a non-zero PendingStacks names one. + assert.Zero(t, stats.PendingStacks, + "a resolved stack is still parked with no launch to join, at rest and after Flush: sampled=%d resolved=%d attached=%d evicted=%d profiler-only=%d uncorrelated=%d", + stats.SampledLaunches, stats.StacksResolved, stats.StacksAttached, + stats.StacksEvicted, stats.StacksProfilerOnly, stats.StacksUncorrelated) + assert.Equal(t, uint64(wantSampled), stats.StacksAttached, + "every sampled launch must reach the timeline carrying its own stack; resolved=%d pending=%d evicted=%d profiler-only=%d uncorrelated=%d missing=%d", + stats.StacksResolved, stats.PendingStacks, stats.StacksEvicted, + stats.StacksProfilerOnly, stats.StacksUncorrelated, stats.StacksMissing) + // The whole join, on one line, in the shape the accounting identity above + // StacksResolved is written in: resolved = attached + evicted + + // profiler-only + pending. Printed whether or not the assertions pass, + // because "58 = 57 + 0 + 0 + 1" is what told issue #67 apart from silent + // loss in the first place. + t.Logf("stack attach: sampled=%d resolved=%d attached=%d evicted=%d profiler-only=%d pending=%d missing=%d uncorrelated=%d", + stats.SampledLaunches, stats.StacksResolved, stats.StacksAttached, + stats.StacksEvicted, stats.StacksProfilerOnly, stats.PendingStacks, + stats.StacksMissing, stats.StacksUncorrelated) assert.Zero(t, stats.StackLookupFailed, "every resolved stack's gpu_stacks entry must be readable back exactly once") assert.Zero(t, stats.StackDeleteFailed, "every gpu_stacks entry read must also be deletable") diff --git a/gpuprobe/sampledstacks.go b/gpuprobe/sampledstacks.go index 050c6fe2..4327d1fc 100644 --- a/gpuprobe/sampledstacks.go +++ b/gpuprobe/sampledstacks.go @@ -16,46 +16,54 @@ import ( // launch that arrives on the *batched* probe, and only that launch is // emitted. // -// The two halves can arrive in either order, and both happen: +// The two halves can arrive in either order, and the consumer handles both: // -// - Sampled first (the common case). The shim's launch batch flushes only -// when it fills, so the batched record for launch N usually reaches the -// ringbuf long after the unbatched sampled record for the same launch. -// The resolved stack waits in pendingStacks until its twin shows up. -// - Batched first. When launch N is the record that fills the batch, the -// flush happens inside the same add() call that precedes the sampler -// check, so the batch - whose last record is launch N - is queued before -// N's own sampled record. The launch waits in deferredLaunches, briefly, -// for the twin that is already on its way. +// - Sampled first. The resolved stack waits in pendingStacks until its +// twin shows up. Nothing can take it but the twin, and any number of +// unrelated batches may pass in the meantime. +// - Batched first. The launch waits in deferredLaunches for a twin that +// had better be the very next thing off the ringbuf, because the first +// batch of any other kind releases the whole queue (Consumer.applyBatch, +// deliberately: the timeline wants launches promptly). // -// The batched-first case has a known, counted, rate-dependent way of losing -// the join, and it is the cause of the PendingStacks the demo run reports -// (24 of 250 captures at 2000 launches; none at 500). The producer queues -// the launch batch, then the EXEC batch, then the sampled probe. So when one -// launch is both the record that fills the launch batch and the one the -// sampler picks, the exec batch lands between the twins - and any batch that -// is not a sampled launch releases the deferred queue (Consumer.applyBatch, -// deliberately: the timeline needs launches promptly). The launch goes out -// stackless and its stack, arriving next, parks with nothing to join. +// Only the first order is safe, and issue #67 is the measurement of that. +// The shim used to add the launch to its batch and only then fire the +// sampled probe, so a launch that both FILLED the batch and was sampled put +// its own batched record on the wire first - with the exec batch of the same +// loop iteration landing between the twins. The exec batch released the +// launch stackless and the stack, arriving next, parked with nothing to +// join: 58 sampled, 57 attached, 1 in PendingStacks on the privileged gate. // -// It looks arithmetically impossible in the stub, which is why it went -// unexplained: batches hold 32 records and the sampler takes one in 8, so -// "fills the batch" (i = 0 mod 32) and "is sampled" (i = 1 mod 8) cannot -// both hold. What breaks that is the producer's periodic drain tick, which -// flushes a PARTIAL batch every 100ms and so re-phases the batch boundary to -// wherever the launch loop had got to. About one tick in eight leaves the -// new boundary sitting on a sampled launch, and until the next tick moves it -// again every remaining fill - one per 32 launches - loses its stack this -// way. Hence rate-dependent: more launches, more ticks, more chances to land -// in phase and more fills to spend there. +// At the old fixed sampler stride that collision was arithmetically +// unreachable - batches hold 32 records and the sampler took one in 8, and a +// multiple of 8 is never 31 mod 32 - which is why the join looked sound for +// a whole phase. Issue #50's jittered stride draws each gap from [4,12], so +// a sampled ordinal eventually lands exactly on a batch boundary. #50 did +// not cause this; it removed the arithmetic that was hiding it. // -// The cost is attribution, never a record: the launch ships, the execution +// The fix is in the producer, not here: shim/stub/stub.cc and +// shim/nvidia/cupti_adapter.cc fire the sampled probe BEFORE the batched +// add(). A record cannot be in a batch before add() puts it there, so no +// flush - on the launching thread or on the drain thread - can carry the +// twin past the sampled probe, and sampled-first holds unconditionally. +// shim/stub/probe_order_test.cc pins that order by patching the probe sites +// with int3 and reading the wire order back, with no privilege and no +// consumer; it fails on the pre-#67 producer at every sampled launch that +// fills a batch. +// +// deferredLaunches stays, because this consumer does not only see shims this +// repository builds: the ABI is public (spec §6), a vendor bridge or an +// older shim may still emit batched-first, and for those the queue is the +// difference between "usually joins" and "never joins". What it cannot do is +// make batched-first lossless - the launch has to go out before the next +// exec batch, and once it has gone out there is nothing left to attach to +// without re-emitting an event the sink has already been given. So the cost +// there is attribution and never a record: the launch ships, the execution // ships, the GPU time is measured and projects as unattributed, and the -// orphaned stack is visible in Stats.PendingStacks. The alternative - -// holding launches past the next batch - trades a rare attribution gain for -// a systematic delay in launch delivery that the timeline's join depends on. -// Reproduced without any privilege by -// TestStackParksUnattachedWhenAnotherBatchSplitsTheTwins. +// orphaned stack is counted in Stats.PendingStacks rather than vanishing. +// That path is held to its documented behaviour by +// TestStackParksUnattachedWhenAnotherBatchSplitsTheTwins, which is now a +// statement about foreign producers rather than about ours. // // Both stores are bounded, and both count what they push out. An unbounded // map on either side is a leak driven by a profiled application's launch diff --git a/shim/Makefile b/shim/Makefile index 2077db84..d563eb13 100644 --- a/shim/Makefile +++ b/shim/Makefile @@ -177,17 +177,22 @@ nvidia/testdata/cuda_workload: nvidia/testdata/cuda_workload.cu $(NVCC) -O2 -g -lineinfo -arch=$(CUDA_ARCH) \ -Xcompiler -fno-omit-frame-pointer -Xcompiler -rdynamic -o $@ $< -# core/probe_args_test.cc is the one test here built with -O2, and that is not -# a detail: it checks that the record a probe points at has actually been -# written to memory when the probe fires, which only an optimizer can get -# wrong. At -O0 it cannot fail, so the flag is part of the test. +# core/probe_args_test.cc and stub/probe_order_test.cc are the two tests here +# built with -O2, and that is not a detail. probe_args_test checks that the +# record a probe points at has actually been written to memory when the probe +# fires, which only an optimizer can get wrong; at -O0 it cannot fail, so the +# flag is part of the test. probe_order_test drives the real producer +# (stub/stub.cc, with PERFAGENT_STUB_NO_MAIN) and reads the wire ORDER of its +# probe fires by patching their nops with int3 - so it has to be the same +# optimized build the gate runs, not a debug one that the compiler laid out +# differently. # # core/usdt_abi_test.c is built with a C compiler on purpose: usdt_abi.h is the # ABI both the C++ shim and any C vendor adapter include, and its # _Static_asserts are the test. Compiling it only as C++ leaves the C11 spelling # unguarded -- which is how the header's validity broke earlier in this branch, # when a C++ translation unit included it for the first time. -test: core/batch_test.cc core/clock_test.cc core/drain_test.cc core/enroll_test.cc core/usdt_abi_test.c core/sampler_test.cc core/probe_args_test.cc $(CORE_SRC) +test: core/batch_test.cc core/clock_test.cc core/drain_test.cc core/enroll_test.cc core/usdt_abi_test.c core/sampler_test.cc core/probe_args_test.cc stub/probe_order_test.cc stub/stub.cc $(CORE_SRC) $(CXX) -std=c++17 -pthread -I core -o /tmp/batch_test core/batch_test.cc core/batch.cc && /tmp/batch_test $(CXX) -std=c++17 -I core -o /tmp/clock_test core/clock_test.cc core/clock.cc && /tmp/clock_test $(CXX) -std=c++17 -pthread -I core -o /tmp/drain_test core/drain_test.cc core/drain.cc && /tmp/drain_test @@ -195,6 +200,8 @@ test: core/batch_test.cc core/clock_test.cc core/drain_test.cc core/enroll_test. $(CC) -std=c11 -Wall -Werror -I core -o /tmp/usdt_abi_test core/usdt_abi_test.c && /tmp/usdt_abi_test $(CXX) -std=c++17 -pthread -I core -o /tmp/sampler_test core/sampler_test.cc core/sampler.cc && /tmp/sampler_test $(CXX) -std=c++17 -O2 -Wall -Werror -I core -o /tmp/probe_args_test core/probe_args_test.cc && /tmp/probe_args_test + $(CXX) -std=c++17 -O2 -Wall -Werror -pthread -I core -DPERFAGENT_STUB_NO_MAIN \ + -o /tmp/probe_order_test stub/probe_order_test.cc stub/stub.cc $(CORE_SRC) && /tmp/probe_order_test # Not a prerequisite of `test`: a TSan build is ~10x slower and `test` is the # fast path. batch_test.cc's concurrent case asserts emitted + dropped == kAdds, diff --git a/shim/nvidia/cupti_adapter.cc b/shim/nvidia/cupti_adapter.cc index 07531224..c116d85d 100644 --- a/shim/nvidia/cupti_adapter.cc +++ b/shim/nvidia/cupti_adapter.cc @@ -272,8 +272,28 @@ void on_launch(const CUpti_CallbackData *cb) { l.context_id = cb->contextUid; l.time_ns = now; l.tid = current_tid(); - g_lb->add(l); + // The sampled probe fires BEFORE the launch reaches its batch (issue + // #67), and the record is fully built above so this reorder changes only + // when the two probes fire, never what they carry. + // + // The two records are twins - same correlation, one carrying the launch, + // the other only the CPU stack the consumer staples onto it - and the + // consumer's two join paths are not equally safe. Sampled first parks the + // stack in pendingStacks, where only the twin can claim it. Batched first + // holds the launch in deferredLaunches, which the next batch of any other + // kind releases stackless, leaving the stack with nothing to join + // (gpuprobe/sampledstacks.go). + // + // With the add() first, a launch that FILLED the batch put its own + // batched record on the wire inside that add(), before this probe. In the + // stub that collision is same-thread and was measured on the gate; here + // it is a thread race - the exec batch is flushed by the CUPTI worker and + // the drain timer, not by this callback - so the window between add() and + // this probe was small but real, and nothing made it impossible. Firing + // here closes it: a record cannot be in a batch before add() puts it + // there, so no flush on any thread can carry the twin past this probe. + // // Unbatched, one record per fire: this probe is the whole reason the // consumer can attribute GPU time to a CPU stack. if (sample && gpu_launch_sampled_v1_enabled()) { @@ -288,6 +308,8 @@ void on_launch(const CUpti_CallbackData *cb) { s.launch_seq = ordinal; gpu_launch_sampled_v1_emit(&s, 1, g_sampled_seq.fetch_add(1, std::memory_order_relaxed)); } + + g_lb->add(l); } void CUPTIAPI on_callback(void *, CUpti_CallbackDomain domain, CUpti_CallbackId cbid, diff --git a/shim/stub/probe_order_test.cc b/shim/stub/probe_order_test.cc new file mode 100644 index 00000000..2bcd41c0 --- /dev/null +++ b/shim/stub/probe_order_test.cc @@ -0,0 +1,329 @@ +// Proves the producer emits a sampled launch's record BEFORE the batched +// gpu_launch_v1 record for that same launch. +// +// Why that order is the thing under test (issue #67) +// -------------------------------------------------- +// The two records are twins: gpu_launch_sampled_v1 carries no launch of its +// own, only the CPU stack the consumer must staple onto the batched +// gpu_launch_v1 with the same correlation (gpuprobe/sampledstacks.go). The +// consumer can join them in either order, but the two orders are not equally +// safe: +// +// - sampled first: the stack parks in pendingStacks and nothing but the +// twin can claim it. Any number of unrelated batches may arrive in +// between; the join still happens. +// - batched first: the launch is held in deferredLaunches, and the FIRST +// batch of any other kind releases it stackless - deliberately, because +// the timeline wants launches promptly. The stack then arrives with +// nothing to join and parks forever. +// +// The producer used to add the launch to its batch and only then fire the +// sampled probe, so a launch that both FILLED the batch and was sampled put +// the batched record on the wire first - with the exec batch of that same +// loop iteration landing between the twins. Measured on the privileged gate: +// 58 sampled, 57 attached, 1 parked forever. At the old fixed sampler stride +// this was arithmetically unreachable at 500 launches (a multiple of 8 is +// never 31 mod 32); the jittered stride of issue #50 made it reachable. +// +// Firing the sampled probe first makes sampled-first unconditional: a record +// cannot be in a batch before add() puts it there, so no flush - on this +// thread or the drain thread - can carry the twin past the sampled record. +// +// How this test sees the wire order without any privilege +// ------------------------------------------------------- +// The same trick core/probe_args_test.cc uses: read our own .note.stapsdt to +// find the probe sites, patch their one-byte nops with int3 (which is all a +// uprobe does), and read the arguments out of the trapped context in the +// SIGTRAP handler - exactly what bpf_probe_read_user does in +// bpf/gpu_usdt.bpf.c. Every probe fire is stamped with a global tick, so the +// order the consumer would see is reconstructed exactly. No CAP_BPF, no +// consumer, no GPU. +#include "usdt_abi.h" + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#if !defined(__x86_64__) +int main() { + // The probe macro binds rdi/rsi/rdx by name, so reading the arguments out + // of a trapped context is as architecture-specific as the ABI itself. + fprintf(stderr, "probe_order_test: skipped, not x86-64\n"); + return 0; +} +#else + +#include + +// The producer under test. stub/stub.cc is compiled into this binary with +// PERFAGENT_STUB_NO_MAIN, so this is the very code the gate runs. +extern "C" void perfagent_stub_run(unsigned launches, unsigned period_us, + unsigned sample_period); + +// stub.cc's probe semaphores. Hidden visibility, so this only links because +// the producer and the test are one binary - which is the point: nothing here +// asks the producer to behave differently for a test. +extern "C" unsigned short perfagent_gpu_launch_v1_semaphore + __attribute__((visibility("hidden"))); +extern "C" unsigned short perfagent_gpu_exec_v1_semaphore + __attribute__((visibility("hidden"))); +extern "C" unsigned short perfagent_gpu_launch_sampled_v1_semaphore + __attribute__((visibility("hidden"))); +extern "C" unsigned short perfagent_gpu_kernel_name_v1_semaphore + __attribute__((visibility("hidden"))); + +// The address every stapsdt note is relative to; comparing it with the note's +// own base field yields the load bias, PIE or not. +extern "C" char perfagent_stapsdt_base_sym __asm__("_.stapsdt.base"); + +// ------------------------------------------------------------ the recorder + +// Correlations in stub.cc are 1..launches, so a flat array indexed by +// correlation is exact and needs no hashing inside a signal handler. +static constexpr unsigned kMaxCorrelation = 4096; + +// Tick of the probe fire that carried each correlation. Zero means "not seen +// yet". Written from the launch thread and from the Drainer's thread, at +// distinct indices in the common case and idempotently otherwise, through +// atomics because a signal handler racing another thread is still a race. +static unsigned long g_launch_at[kMaxCorrelation]; +static unsigned long g_sampled_at[kMaxCorrelation]; +static unsigned long g_tick; +static unsigned long g_full_batches; // launch batches that flushed at N==32 +static unsigned long g_launch_records; +static unsigned long g_sampled_records; +static unsigned long g_overflow; // a correlation past kMaxCorrelation + +enum { kProbeLaunch, kProbeSampled, kProbeCount }; +static uintptr_t g_probe_site[kProbeCount]; + +static void note_first(unsigned long *slot, unsigned long tick) { + unsigned long zero = 0; + // First fire wins. A batch is flushed once, so this only ever matters if + // a correlation were somehow emitted twice - in which case the EARLIER + // record is the one the consumer joins against, and the later one must + // not overwrite it. + __atomic_compare_exchange_n(slot, &zero, tick, false, __ATOMIC_SEQ_CST, + __ATOMIC_SEQ_CST); +} + +static void on_trap(int, siginfo_t *, void *ucv) { + ucontext_t *uc = (ucontext_t *)ucv; + // int3 leaves %rip one byte past the patched nop, which is where the nop + // itself would have left it: returning from the handler resumes correctly + // with no single-stepping and no byte to restore. + const uintptr_t site = (uintptr_t)uc->uc_mcontext.gregs[REG_RIP] - 1; + const void *ptr = (const void *)uc->uc_mcontext.gregs[REG_RDI]; + const unsigned long count = (unsigned long)uc->uc_mcontext.gregs[REG_RSI]; + const unsigned long tick = __atomic_add_fetch(&g_tick, 1, __ATOMIC_SEQ_CST); + + if (site == g_probe_site[kProbeLaunch]) { + const struct gpu_launch_v1 *recs = (const struct gpu_launch_v1 *)ptr; + if (count == 32) __atomic_add_fetch(&g_full_batches, 1, __ATOMIC_SEQ_CST); + for (unsigned long i = 0; i < count; i++) { + const uint64_t corr = recs[i].correlation; + __atomic_add_fetch(&g_launch_records, 1, __ATOMIC_SEQ_CST); + if (corr < kMaxCorrelation) note_first(&g_launch_at[corr], tick); + else __atomic_add_fetch(&g_overflow, 1, __ATOMIC_SEQ_CST); + } + return; + } + if (site == g_probe_site[kProbeSampled]) { + const struct gpu_launch_sampled_v1 *rec = + (const struct gpu_launch_sampled_v1 *)ptr; + const uint64_t corr = rec->correlation; + __atomic_add_fetch(&g_sampled_records, 1, __ATOMIC_SEQ_CST); + if (corr < kMaxCorrelation) note_first(&g_sampled_at[corr], tick); + else __atomic_add_fetch(&g_overflow, 1, __ATOMIC_SEQ_CST); + } +} + +// --------------------------------------------------------------- the notes + +// probe_address returns the run-time address of the named probe's nop, read +// out of this binary's own .note.stapsdt - the same note internal/usdt parses +// and the same one the kernel is pointed at. +static uintptr_t probe_address(const char *want) { + int fd = open("/proc/self/exe", O_RDONLY); + assert(fd >= 0 && "open /proc/self/exe"); + struct stat st; + assert(fstat(fd, &st) == 0); + const uint8_t *m = (const uint8_t *)mmap(nullptr, (size_t)st.st_size, PROT_READ, + MAP_PRIVATE, fd, 0); + assert(m != MAP_FAILED); + close(fd); + + const Elf64_Ehdr *eh = (const Elf64_Ehdr *)m; + const Elf64_Shdr *sh = (const Elf64_Shdr *)(m + eh->e_shoff); + const char *shstr = (const char *)(m + sh[eh->e_shstrndx].sh_offset); + + uintptr_t found = 0; + for (unsigned i = 0; i < eh->e_shnum && !found; i++) { + if (sh[i].sh_type != SHT_NOTE) continue; + if (strcmp(shstr + sh[i].sh_name, ".note.stapsdt") != 0) continue; + const uint8_t *p = m + sh[i].sh_offset; + const uint8_t *end = p + sh[i].sh_size; + while (p + sizeof(Elf64_Nhdr) <= end) { + const Elf64_Nhdr *nh = (const Elf64_Nhdr *)p; + const uint8_t *desc = p + sizeof(Elf64_Nhdr) + ((nh->n_namesz + 3) & ~3u); + uint64_t pc, base; + memcpy(&pc, desc, 8); + memcpy(&base, desc + 8, 8); + const char *provider = (const char *)(desc + 24); + const char *name = provider + strlen(provider) + 1; + if (strcmp(provider, "perfagent") == 0 && strcmp(name, want) == 0) { + found = (uintptr_t)pc + + ((uintptr_t)&perfagent_stapsdt_base_sym - (uintptr_t)base); + break; + } + p = desc + ((nh->n_descsz + 3) & ~3u); + } + } + munmap((void *)m, (size_t)st.st_size); + return found; +} + +// Become the uprobe: replace the probe's one-byte nop with int3. The mapping +// is private, so this is a copy-on-write of our own text. +static void patch(uintptr_t probe) { + const long pagesize = sysconf(_SC_PAGESIZE); + void *page = (void *)(probe & ~(uintptr_t)(pagesize - 1)); + assert(mprotect(page, (size_t)pagesize * 2, PROT_READ | PROT_WRITE | PROT_EXEC) == 0); + assert(*(volatile uint8_t *)probe == 0x90 && "probe site is not the expected nop"); + *(volatile uint8_t *)probe = 0xCC; + assert(mprotect(page, (size_t)pagesize * 2, PROT_READ | PROT_EXEC) == 0); +} + +// ---------------------------------------------------------------- the pass + +static void reset() { + memset((void *)g_launch_at, 0, sizeof(g_launch_at)); + memset((void *)g_sampled_at, 0, sizeof(g_sampled_at)); + g_tick = 0; + g_full_batches = 0; + g_launch_records = 0; + g_sampled_records = 0; + g_overflow = 0; +} + +// Runs the producer and checks the one invariant the consumer's join rests +// on: for every launch the sampler picked, the sampled record reached the +// wire strictly before the batched record carrying that same correlation. +static int run_pass(const char *what, unsigned launches, unsigned sample_period) { + assert(launches < kMaxCorrelation); + reset(); + perfagent_stub_run(launches, 0, sample_period); + + int bad = 0; + unsigned checked = 0, first_bad = 0; + for (unsigned corr = 1; corr <= launches; corr++) { + const unsigned long s = g_sampled_at[corr]; + if (!s) continue; // this launch was not sampled + checked++; + const unsigned long l = g_launch_at[corr]; + if (l == 0) { + fprintf(stderr, "%s: correlation %u was sampled but its batched " + "launch record never reached the wire\n", what, corr); + bad++; + continue; + } + if (s > l) { + if (!first_bad) first_bad = corr; + bad++; + } + } + + if (bad) { + fprintf(stderr, + "%s: %d of %u sampled launches had their BATCHED record emitted " + "before their sampled twin (first: correlation %u, batched at " + "tick %lu, sampled at tick %lu).\n" + " The consumer holds that launch in deferredLaunches, the exec " + "batch of the same loop iteration releases it stackless, and the " + "stack parks in pendingStacks with nothing to join (issue #67).\n" + " Fire the sampled probe BEFORE the batched add().\n", + what, bad, checked, first_bad, + g_launch_at[first_bad], g_sampled_at[first_bad]); + return 1; + } + + // Neither half may pass vacuously. Without a full batch the collision + // this test exists for cannot occur at all, and without sampled records + // there is nothing to order. + if (!g_full_batches) { + fprintf(stderr, "%s: no launch batch ever flushed full, so the " + "batch-boundary collision was never reachable\n", what); + return 1; + } + if (!checked) { + fprintf(stderr, "%s: no sampled launch was observed at all\n", what); + return 1; + } + if (g_overflow) { + fprintf(stderr, "%s: %lu records carried a correlation past the table\n", + what, g_overflow); + return 1; + } + printf("probe_order_test: %s ok - %u sampled launches, all emitted before " + "their batched twin; %lu launch records in %lu full batches\n", + what, checked, g_launch_records, g_full_batches); + return 0; +} + +int main() { + // The rendezvous is a consumer-side service and there is no consumer + // here; disabling it outright keeps this test from spending its budget + // discovering that (shim/core/enroll.h). + setenv("PERFAGENT_GPU_ENROLL_TIMEOUT_MS", "0", 1); + + g_probe_site[kProbeLaunch] = probe_address("gpu_launch_v1"); + g_probe_site[kProbeSampled] = probe_address("gpu_launch_sampled_v1"); + if (!g_probe_site[kProbeLaunch] || !g_probe_site[kProbeSampled]) { + fprintf(stderr, "probe_order_test: missing a perfagent probe note\n"); + return 1; + } + + struct sigaction sa {}; + sa.sa_sigaction = on_trap; + sa.sa_flags = SA_SIGINFO; + sigemptyset(&sa.sa_mask); + assert(sigaction(SIGTRAP, &sa, nullptr) == 0); + + patch(g_probe_site[kProbeLaunch]); + patch(g_probe_site[kProbeSampled]); + + // Arm every semaphore, not just the two that are patched: the producer + // takes a different path when a probe is unattached (Batch::add counts + // and discards), and this test must exercise the attached one. The exec + // and kernel-name probes fire their unpatched nops and cost nothing, + // while still putting the exec batch on the wire between the twins, + // which is the record that releases the deferred queue. + perfagent_gpu_launch_v1_semaphore = 1; + perfagent_gpu_exec_v1_semaphore = 1; + perfagent_gpu_launch_sampled_v1_semaphore = 1; + perfagent_gpu_kernel_name_v1_semaphore = 1; + + int bad = 0; + // Pass 1, deterministic. At sample_period 1 every launch is sampled, so + // the launch that fills the 32-record batch is sampled BY CONSTRUCTION + // and the collision is not left to the sampler's schedule. This is the + // pass that must fail on unfixed code, on every machine, every run. + bad |= run_pass("period=1 launches=64", 64, 1); + // Pass 2, the shipped configuration. The gate runs 500 launches at + // period 8; 2048 gives the jittered schedule enough sample points to + // land on a batch boundary the way the privileged gate did. It can only + // ever detect a violation, never invent one, so a run in which the + // schedule happens to miss every boundary still passes - pass 1 is what + // makes the test deterministic. + bad |= run_pass("period=8 launches=2048", 2048, 8); + return bad; +} +#endif diff --git a/shim/stub/stub.cc b/shim/stub/stub.cc index 6d96a1e1..cb3bdb17 100644 --- a/shim/stub/stub.cc +++ b/shim/stub/stub.cc @@ -110,17 +110,31 @@ perfagent_stub_run(unsigned launches, unsigned period_us, unsigned sample_period l.context_id = 1; l.time_ns = now; l.tid = current_tid(); - lb.add(l); - - gpu_exec_v1 e{}; - e.correlation = i; - e.kernel_id = l.kernel_id; - e.queue_id = 1; - e.device_id = 0; - e.start_ns = now + 10000; // 10us after the launch - e.end_ns = now + 10000 + 50000; // 50us on device - eb.add(e); + // The sampled probe fires BEFORE the launch reaches its batch, and + // that order is the fix for issue #67 rather than a stylistic + // preference. The two records are twins - same correlation, one + // carrying the launch, the other only the CPU stack the consumer + // staples onto it (gpuprobe/sampledstacks.go) - and the consumer's + // two join paths are not equally safe. Sampled first parks the stack + // in pendingStacks, where only the twin can claim it and any number + // of unrelated batches may pass in between. Batched first holds the + // launch in deferredLaunches, which the very next batch of any other + // kind releases stackless - deliberately, since the timeline wants + // launches promptly - leaving the stack to park with nothing to join. + // + // With the add() first, a launch that both FILLED the batch and was + // sampled put the batched record on the wire inside that add(), and + // the exec batch below landed between the twins: 58 sampled, 57 + // attached, 1 parked forever on the privileged gate. Firing here + // instead makes sampled-first unconditional, because a record cannot + // be in a batch before add() puts it there - so no flush, on this + // thread or on the Drainer's, can carry the twin past this probe. + // + // should_sample() is called on every launch regardless (&& is + // short-circuit and the sampler call is the left operand), so the + // schedule does not depend on whether a consumer is attached. + // // Unbatched: the eBPF consumer captures the calling thread's stack // the instant this probe fires, so the record must ride alone. if (sampler.should_sample() && gpu_launch_sampled_v1_enabled()) { @@ -136,6 +150,17 @@ perfagent_stub_run(unsigned launches, unsigned period_us, unsigned sample_period gpu_launch_sampled_v1_emit(&sl, 1, sampled_seq++); } + lb.add(l); + + gpu_exec_v1 e{}; + e.correlation = i; + e.kernel_id = l.kernel_id; + e.queue_id = 1; + e.device_id = 0; + e.start_ns = now + 10000; // 10us after the launch + e.end_ns = now + 10000 + 50000; // 50us on device + eb.add(e); + if (period_us) std::this_thread::sleep_for(std::chrono::microseconds(period_us)); }