Repository navigation
pgo - #1825
Draft
daniel-noland wants to merge 10 commits into
Draft
pgo#1825daniel-noland wants to merge 10 commits into
daniel-noland wants to merge 10 commits into
Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
Comment |
daniel-noland
force-pushed
the
pr/daniel-noland/pgo-bolt
branch
from
October 3, 2026 03:28
5d2ac08 to
9606b0e
Compare
daniel-noland
force-pushed
the
pr/daniel-noland/dataplane-perf
branch
10 times, most recently
from
October 4, 2026 06:05
54624a4 to
76fd1ef
Compare
Base automatically changed from
pr/daniel-noland/dataplane-perf
to
split/dpdk-core/08-probes
October 4, 2026 06:05
daniel-noland
force-pushed
the
split/dpdk-core/08-probes
branch
from
October 7, 2026 03:09
6e3983c to
fcb1b62
Compare
daniel-noland
force-pushed
the
pr/daniel-noland/pgo-bolt
branch
from
October 7, 2026 03:09
9606b0e to
8cc6c76
Compare
daniel-noland
changed the base branch from
split/dpdk-core/08-probes
to
pr/daniel-noland/profiling
October 7, 2026 03:09
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
daniel-noland
force-pushed
the
pr/daniel-noland/profiling
branch
from
October 8, 2026 00:41
7085834 to
d75c833
Compare
daniel-noland
force-pushed
the
pr/daniel-noland/pgo-bolt
branch
2 times, most recently
from
October 8, 2026 01:53
7c5a00b to
bb0e9a7
Compare
daniel-noland
force-pushed
the
pr/daniel-noland/profiling
branch
2 times, most recently
from
October 8, 2026 03:32
6b444e9 to
b0247ce
Compare
daniel-noland
force-pushed
the
pr/daniel-noland/pgo-bolt
branch
from
October 8, 2026 03:32
bb0e9a7 to
9d2a932
Compare
daniel-noland
force-pushed
the
pr/daniel-noland/profiling
branch
from
October 8, 2026 07:16
b0247ce to
a8a70e1
Compare
daniel-noland
force-pushed
the
pr/daniel-noland/pgo-bolt
branch
from
October 8, 2026 07:16
9d2a932 to
8e6b420
Compare
daniel-noland
force-pushed
the
pr/daniel-noland/pgo-bolt
branch
from
October 16, 2026 05:47
8e6b420 to
8ad5c3b
Compare
daniel-noland
force-pushed
the
pr/daniel-noland/profiling
branch
from
October 16, 2026 08:43
687c8b6 to
2bea7fc
Compare
daniel-noland
force-pushed
the
pr/daniel-noland/pgo-bolt
branch
from
October 16, 2026 08:43
8ad5c3b to
1cca809
Compare
daniel-noland
force-pushed
the
pr/daniel-noland/profiling
branch
from
October 17, 2026 03:26
2bea7fc to
ee6d8cf
Compare
daniel-noland
force-pushed
the
pr/daniel-noland/pgo-bolt
branch
from
October 17, 2026 03:26
1cca809 to
18d1203
Compare
daniel-noland
force-pushed
the
pr/daniel-noland/profiling
branch
from
October 17, 2026 05:59
ee6d8cf to
f1e0f51
Compare
daniel-noland
force-pushed
the
pr/daniel-noland/pgo-bolt
branch
from
October 17, 2026 05:59
18d1203 to
1031436
Compare
…nges The monitor subscribed to netlink link multicast and nothing else, so it learned an interface's MAC only if the address changed *while it was listening*. Two ways that loses, and both stick forever because a MAC does not change twice: 1. The change lands before the router has the interface in its table. `handle_ifevent` returns early for an unknown ifindex (ctl.rs:268) and the event is gone. 2. The change happens before the monitor is listening at all -- which is the normal case, since init creates the control-plane tap and gives it the DPDK port's address during startup. The consequence is severe and silent. The datapath tests every arriving frame's destination against the table's address, so a stale entry means every unicast frame is dropped as `MacNotForUs`: ARP never resolves, BFD fails, BGP tears down, every route is withdrawn, and the gateway forwards nothing while looking healthy by every other measure -- process up, config applied, BGP configured. Observed repeatedly on the bench, where the table held a tap's random birth address (`0a:..`, `8a:..`) against a port MAC of `58:a2:e1:b3:3d:94`. So read the state instead of waiting to be told about it: - `resync` dumps every tracked link via RTM_GETLINK and emits the current state through the same channel. Run once at startup, before any notification is processed. - Re-run every 5s. A notification dropped downstream is then costly for seconds rather than for the life of the process. This is the level-triggered backstop; the multicast subscription remains the low-latency path. - `link_to_event` is split out of `netlink_to_event` so a dumped `LinkMessage` and a multicast notification share one conversion. This is the smaller half of closing this loop. The larger one -- making interface-manager authoritative for observed state, so configuration carries intent only and never a MAC -- is a separate change, and this is a strict subset of it rather than work that has to be undone. Tests cover the conversion resync depends on, and were break-tested: ignoring the address attribute fails `a_dumped_link_yields_its_current_mac`, and dropping every link fails that plus `a_link_without_an_address_still_reports_state`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`DriverStatus`'s `Display` read `DriverKernel::MAX_RX_PKT_BATCH` (128) unconditionally, so a gateway running the DPDK driver reported a receive batch of 128 when the real burst is `MBUF_BURST` (64). Nothing depended on the value, which is why it survived. It is still worth fixing: the number was read off a live gateway, used to size the per-burst cache working set, and produced a confident and wrong claim that the working set exceeded L1d (50.2 KB against 48 KB). The real figure is ~33 KB and fits. A wrong number on a status page became a wrong conclusion about cache pressure. `DriverStatus` now carries a `DriverLimits` that the publishing driver fills in, so each reports its own batch size and watchdog periods rather than the display assuming one driver's constants. The test asserts both that the publisher's value is shown and that the other driver's does not leak in, and opens with a vacuity guard: the whole test means nothing if the two constants ever become equal, so it says so rather than passing silently. Break-tested by restoring the hardcoded constant. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A profile-instrumented build writes its counters when told to, but the dataplane never exits on its own -- it runs until something kills it, and a kill loses the counters. This gives us a way to ask for them without stopping the gateway: signal init, and init passes SIGUSR1 along. Forwarding is opt-in via DATAPLANE_DEV_PROFILE_DUMP, because the default disposition of SIGUSR1 is to terminate, and an unwanted one would reach a process that dies on it. The value names the target, defaulting to "dataplane": FRR is supervised here too and reads SIGUSR1 as "rotate your logs", so a broadcast would quietly do something else entirely. The signal goes to the process, not its group. A group signal reaches children that have no handler, where the default disposition kills them; the test holds that line with a handler-less child of its own, and fails on the group-signalling version. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds `instrumentation=pgo`, and a way to get a profile out of a build that uses it. A `-Cprofile-generate` build writes its counters from an atexit handler that the dataplane never reaches: it runs until something kills it, and a kill loses them. SIGUSR1 now asks for them without stopping, so the profile can come off a run that is still under load -- which is the only run whose profile is worth having. The signal already arrived here and was matched into an arm that did nothing. It now calls the profiling runtime, and no longer shares an arm with SIGUSR2, SIGHUP, SIGALRM and SIGPIPE, which should still do nothing. Gating is on `profile_generate`, a cfg set by the same `instrumentation` argument that adds the flag, rather than a cargo feature. The two cannot come apart, so the symbol the call needs is present wherever the call compiles; an uninstrumented build takes the other arm and says so, rather than leaving a requested dump looking like it happened. Where the counters land is defaulted, not forced, the same way perf's `-o` is, and tied to the switch that turns forwarding on: a forward that arrives with nowhere to write is not much use. The destination is a plain filename and not one of LLVM's `%m` merge patterns -- each dump holds the counters accumulated since process start, so merging one into the last would count everything before the first dump twice. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…sion A profile is only worth collecting from a real run under real traffic, so an instrumented build that cannot be containerised cannot reach the test bench. `_refuse-instrumented-artifact` blocked every instrumentation mode, which made `pgo` useless from the moment it existed. It is now exempt; coverage and fuzz still refuse, because those really are diagnostics that never want to ship. The exemption is safe because the reason for the refusal is now addressed rather than waived. Instrumentation was not part of the version, so an instrumented image would take a clean tag and replace it -- the guard's own stated objection. `version_instr` puts it in, next to the sanitizer and feature components that are there for the same reason: an instrumented binary performs nothing like a clean one, and a tag that does not say so is a trap for whoever pulls it next. A pgo image is `-instr.pgo` and cannot occupy the clean tag. Also gives `-Cprofile-generate` the directory rustc requires; a bare one is rejected with "must have a value". It is only a default, since `LLVM_PROFILE_FILE` overrides it at runtime, and it names the same directory init puts a perf profile in -- the one place in the container that outlives the container. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…uld derive
`version_instr` only applies to a computed version: `version := env("VERSION",
...)` means an explicit VERSION= bypasses it entirely -- and that is the path
any deploy or bench script takes, so the protection was absent exactly where
it was most needed.
The guard now looks at the version it is about to tag with. An instrumented
image whose version does not name the instrumentation is refused, however that
version was arrived at.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`-Cprofile-use` consumes the Rust side only; instrumenting C as well collects counters nothing then reads, and C PGO would need its own `-fprofile-use`. This is a scope change, not a performance fix. It is worth recording what it is *not*, because the first version of this commit claimed otherwise. An instrumented build collapses under load, and excluding DPDK does not rescue it. Measured on env-5, 64 B at 128 streams: | workers | clean | C+Rust instr | Rust-only instr | |---|---|---|---| | 1 | 1.73 | 0.328 | 0.366 | | 16 | 14.56 | 0.128 | 0.033 | Excluding C buys nothing measurable at one worker, and at sixteen both are gone. The mechanism is not instrumentation overhead, which is a tolerable 5x per core: it is that LLVM's counters are process-global, so sixteen workers running identical code do read-modify-write on the same cache lines and ping-pong them. Throughput *falls* from 1 worker to 16 -- adding fifteen cores makes it 2.6x slower -- which per-core overhead cannot produce. The consequence for anyone collecting a profile: take it at one or two workers. PGO wants relative block frequencies, and a single worker walking the same pipeline gives those just as well, at a 5x slowdown rather than a 400x one. DPDK's own `b_pgo=generate` would not have helped; meson emits the same `-fprofile-generate`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collecting a profile was only half of it; nothing could consume one. Adds two instrumentation modes and the recipes that join them up. `pgo-use` feeds a merged profile back with `-Cprofile-use`. The path is fixed at `.pgo/dataplane.profdata` rather than passed as an argument, so nothing has to be threaded through the three `profiles.nix` call sites; nix copies the file into the store, so editing it rebuilds what depends on it, and a missing one fails evaluation naming the path. `-pgo-warn-missing-function` goes with it, because rustc's default is to warn quietly and carry on, which reads exactly like a successful PGO build when the profile no longer matches the code. `bolt` asks the linker for the relocations it normally drops, which is all a binary needs to become a BOLT input. It stays separate from `pgo-use` so the two compose: a PGO build is a perfectly good BOLT input. `just pgo-merge`, `just bolt-fdata` and `just bolt` drive the tools, and BOLT joins the dev shell from the same `llvmPackages'` as clang, so it cannot drift to a different LLVM than the one that built the binary. The container guard now refuses by name. It had been written as "anything that is not none", which would have blocked both new modes -- and those are the ones that produce artifacts worth shipping: `pgo-use` is not instrumented at all, and `bolt` only carries extra relocations. Only coverage and fuzz are diagnostic builds that never want to ship. Profiles are gitignored: megabytes of machine-specific counters with a short shelf life, and a stale one is worse than none, since `-Cprofile-use` will consume it happily and optimise for a workload nobody runs any more. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…acks properly Two things the first BOLT run got wrong, both found by running it. `instrument=bolt` did not build at all. `--emit-relocs` makes the linker emit `.rela.debug_*` alongside the code relocations, and postInstall then removes `.debug_loclists`, leaving a relocation pointing at a section that is gone: objcopy stops with "symbol `.debug_loclists' required but not present" and the cli derivation fails. A BOLT build now keeps its debug sections, which it wants anyway to update them as it moves code. Other builds are untouched -- the objcopy invocation is unchanged for them. `bolt-fdata` decided whether to pass `-nl` by grepping the report text for "branch", which reported "no branch stacks" for a recording made with `perf record -j any,u` and would have silently dropped every future run into degraded mode. It now reads the recording's own sample_type, which gains BRANCH exactly when branch records are present, and says nothing when they are. Verified end to end against a real build: perf2bolt consumed branch events and wrote a profile, llvm-bolt rewrote the binary (taken branches -43.7%), and the result runs with output identical to the original once argv[0] is normalised. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-authored-by: Codex <codex@openai.com>
daniel-noland
force-pushed
the
pr/daniel-noland/profiling
branch
from
October 17, 2026 06:17
f1e0f51 to
2771be3
Compare
daniel-noland
force-pushed
the
pr/daniel-noland/pgo-bolt
branch
from
October 17, 2026 06:17
1031436 to
2149d34
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
scratch: don't yet review please
🤖 Generated with Claude Code