diff --git a/.superpowers/sdd/issue-67-report.md b/.superpowers/sdd/issue-67-report.md new file mode 100644 index 0000000..b357776 --- /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 c3e83d2..c156d37 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 d17f82c..109404b 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 e25fbfe..76d890c 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 050c6fe..4327d1f 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 2077db8..d563eb1 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 0753122..c116d85 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 0000000..2bcd41c --- /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 6d96a1e..cb3bdb1 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)); }