Attribute ring-buffer loss to the CPU and probe that lost it - #15
Conversation
A reserve failure told a reader only that the capture had a hole somewhere. The kernel already held finer detail and userspace threw it away: the telemetry counters live in a per-CPU array, and the readout summed across CPUs before the numbers reached the stream. Report two independent projections of the same total rather than their cross product. `kernel_loss_by_cpu` bounds a hole to a core and costs nothing, being the map contents the lookup already returned. `kernel_loss_by_probe` names the kernel function or TC/XDP program that could not emit, which is what identifies the leg of a path a hole belongs to. Only CPUs and probes that lost something appear; an empty array positively states that none did. The per-probe attribution needs new kernel state: a preallocated per-CPU hash keyed by probe site, so the failure path never allocates and needs no atomic. All sixteen reserve-failure sites now route through one helper that bumps the authoritative total first and adds attribution second. A probe plan that overflows the map degrades to `kernel_unattributed_reserve_failures` rather than misfiling loss against the wrong probe. Because the total and the attribution are separate map reads taken while probes still fire, userspace reads them in the opposite order to the kernel's writes. That keeps the breakdown an undercount of the total it refines rather than, as measured before the ordering was fixed, a count that could exceed it. Reliability gains three optional fields. They are defaulted on read and absent from the schema's required list, so captures recorded before this change still parse and replay. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 34 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe change adds kernel loss attribution by CPU and probe site. BPF records attributed failures and overflow. Sensor code reads per-CPU and probe counters. Reliability contracts, schemas, replay output, documentation, and tests expose the resulting breakdowns. ChangesKernel loss attribution
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant BPF as skbx.bpf.c
participant Sensor as LiveSensor
participant Raw as KernelStatsByCpu
participant CLI as skbx-cli
participant Contract as Reliability
BPF->>BPF: Record failure by probe site
Sensor->>BPF: Read per-CPU statistics and probe_site_loss
BPF-->>Sensor: Return counters
Sensor->>Raw: Decode per-CPU telemetry
Raw-->>CLI: Provide totals and CPU losses
CLI->>CLI: Compute stable CPU and probe deltas
CLI->>Contract: Store reliability breakdowns
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/skbx-contract/src/lib.rs (1)
651-679: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider encoding the "exactly one site" rule in the type.
The doc comment states that exactly one of
functionandprogram_idis set. The struct permits bothNoneand bothSome. The CLI producer derives both fields from the sameProbeSiteKey, so a key withfunction_ip == 0andprogram_id == 0yields bothNone, and the replay output then printsunknown. An enum, for exampleKernelProbeSite { Function(FunctionRef), Program(u32) }, would make the invariant unrepresentable. This changes the wire shape, so defer it if the append-only field plan must hold.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/skbx-contract/src/lib.rs` around lines 651 - 679, Replace the independently optional `function` and `program_id` fields in `KernelProbeLoss` with an enum such as `KernelProbeSite` that represents exactly one function or program site, and update producers and consumers to use it. Ensure zero-valued `ProbeSiteKey` inputs are handled explicitly rather than producing an invalid empty site or replaying as `unknown`; defer this change only if preserving the append-only wire shape is required.crates/skbx-sensor/src/raw.rs (1)
583-602: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider passing the probe breakdown into
into_reliability.
into_reliabilityreturns aReliabilitywithkernel_loss_by_probealways empty and relies on the caller to fill it.crates/skbx-cli/src/main.rsdoes this at two sites, lines 1148 and 1168. A third caller that forgets the assignment drops the attribution without any signal. Accept the breakdown as a parameter so the field cannot be left unset by accident.♻️ Proposed signature change
pub fn into_reliability( self, recursion_misses: u64, decode_failures: u64, enrichment_failures: u64, + loss_by_probe: Vec<KernelProbeLoss>, ) -> Reliability { let total = self.total(); Reliability { @@ kernel_loss_by_cpu: self.loss_by_cpu(), kernel_unattributed_reserve_failures: total.unattributed_reserve_failures, - // Filled in by the caller, which owns the symbol table needed to - // turn a probe site address into a function name. - kernel_loss_by_probe: Vec::new(), + // The caller resolves probe sites, because it owns the symbol table. + kernel_loss_by_probe: loss_by_probe, } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/skbx-sensor/src/raw.rs` around lines 583 - 602, Update RawStats::into_reliability to accept the probe breakdown as a parameter and assign it directly to Reliability.kernel_loss_by_probe instead of initializing an empty vector. Update both callers in main.rs to pass their computed breakdown and remove the follow-up field assignments, ensuring every caller must provide probe attribution.crates/skbx-sensor/src/live.rs (1)
613-648: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDedupe duplicate sites while iterating the hash map.
MapHandle::keys()exposesbpf_map_get_next_key()iteration, so concurrent insertions can change the next key and cause the same key to be visited more than once. Returning each visit as a separate entry can duplicate sites inprobe_loss()and downstream loss breakdowns. Accumulate or dedupe byProbeSiteKeybefore returning the list.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/skbx-sensor/src/live.rs` around lines 613 - 648, Update probe_loss to deduplicate entries by ProbeSiteKey while iterating probe_site_loss.keys(), accumulating reserve_failures for repeated visits to the same site before constructing the returned Vec<ProbeSiteLoss>. Preserve the existing key/value validation, eviction handling, and omission of sites whose accumulated failures remain zero.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/skbx-contract/src/lib.rs`:
- Around line 651-679: Replace the independently optional `function` and
`program_id` fields in `KernelProbeLoss` with an enum such as `KernelProbeSite`
that represents exactly one function or program site, and update producers and
consumers to use it. Ensure zero-valued `ProbeSiteKey` inputs are handled
explicitly rather than producing an invalid empty site or replaying as
`unknown`; defer this change only if preserving the append-only wire shape is
required.
In `@crates/skbx-sensor/src/live.rs`:
- Around line 613-648: Update probe_loss to deduplicate entries by ProbeSiteKey
while iterating probe_site_loss.keys(), accumulating reserve_failures for
repeated visits to the same site before constructing the returned
Vec<ProbeSiteLoss>. Preserve the existing key/value validation, eviction
handling, and omission of sites whose accumulated failures remain zero.
In `@crates/skbx-sensor/src/raw.rs`:
- Around line 583-602: Update RawStats::into_reliability to accept the probe
breakdown as a parameter and assign it directly to
Reliability.kernel_loss_by_probe instead of initializing an empty vector. Update
both callers in main.rs to pass their computed breakdown and remove the
follow-up field assignments, ensuring every caller must provide probe
attribution.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 53e56ea4-9cbf-4100-84a8-46a184338169
📒 Files selected for processing (7)
crates/skbx-cli/src/main.rscrates/skbx-contract/src/lib.rscrates/skbx-sensor/bpf/skbx.bpf.ccrates/skbx-sensor/src/lib.rscrates/skbx-sensor/src/live.rscrates/skbx-sensor/src/raw.rsdocs/architecture.md
Two findings from CodeRabbit on this branch, both real. probe_loss collected into a list. keys() is bpf_map_get_next_key iteration, which can hand back the same key more than once when entries are inserted while it runs, and probes keep firing throughout the read. Every revisit became a second entry for the same probe and overstated that probe's share of the loss. Collect through a map keyed by site instead, so a revisit overwrites an equivalent reading rather than duplicating it. into_reliability returned a Reliability with kernel_loss_by_probe empty and left the caller to patch it in afterwards. A future third call site that forgot would drop the attribution silently, and a dropped breakdown reads exactly like a capture that lost nothing per probe. Take it as a parameter so the compiler requires it. A third finding, replacing the two optional site fields with an enum so that "exactly one of function and program_id" is unrepresentable, is correct but changes the wire shape. Deferred: the append-only schema is what lets captures recorded before these fields still parse. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
into_reliability gained a parameter and four unit tests still passed three arguments. cargo build does not compile tests, so building rather than running make check hid it; the previous commit was pushed red. Also assert the breakdown reaches the footer, which is the behaviour the new parameter exists to guarantee. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Evidence change
A reserve failure previously told a reader only that the capture had a hole. It did not say where, so
complete: falsehad to downgrade every absence in the capture at once.The footer now carries two independent breakdowns of the same total:
These are two projections, not a matrix. A probe-by-core cross product grows with both dimensions and answers a question nobody asked.
This narrows where to look. It does not restore absence as evidence: a hole attributed to
ip_rcvstill does not say which packet lost that observation, socomplete: falsecontinues to downgrade every absence in the capture.Prompted by a question on the DEV article — "a global reservation-failure counter tells you the capture has a hole without telling you which leg of the path it's in."
Change design
PT_REGS_IP(ctx)for kernel probes,CONFIG.dynamic_program_idfor TC/XDP. They occupy separate fields so nothing assumes a program id can never look like a kernel text address.BPF_MAP_TYPE_PERCPU_HASH, 512 entries, preallocated at load. Per-CPU means no atomic; preallocated means the failure path never allocates, which matters because kprobes fire in contexts where allocation is unsafe. Overflow degrades to a counter, never to a wrong attribution.kernel_unattributed_reserve_failureswhen the map is full, and read skew (below).kernel_reserve_failuresremains the only authoritative total.#[serde(default)]on read and absent from the schema'srequiredlist. The existingsample.traceq.jsonlfixture, which predates all of them, still parses and replays.Two bugs found by running it, not by reading it
Per-probe sum exceeded the total. The kernel bumps the global counter before the attribution map; userspace read them in that same order, so failures landing between the two reads were attributed without being counted. Measured at
+32on a 611k-failure capture. Fixed by reading attribution first, which makesattributed ≤ totalhold by construction. Re-runs:-1,-2,-1.Segment footers can overshoot. A segment is the difference of two checkpoints of two independently sampled series, so if the earlier checkpoint lagged further than the later one, the delta exceeds that segment's own total (measured: seg 2 at
-3, seg 3 at+2, compensating; cumulative still-1). Not fixable — two maps cannot be snapshotted atomically while 64 CPUs fire probes. Left as measured and documented rather than clamped, because clamping invents a number the kernel never reported.I had documented
attributed + unattributed == total. That was wrong; it is≤. The assertion claiming equality passed only because it ran on synthetic data. Both corrected.Checks run
make check— fmt,cargo test --workspace --all-features --locked --offline,cargo clippy --all-targets --all-features -- -D warnings. Exit 0.make build— release. Exit 0.Reliabilityvalidated against the emitted JSON Schema withjsonschema.probe_site_keyat size 16 andunattributed_reserve_failuresatbits_offset=192— byte 24, matching the Rust decoder'sbytes[24..32].doctorREADY. Verifier accepts the program. Clean capture reportscomplete=truewith both breakdowns empty. Forced ~611k reserve failures (4 probes +--output-skb --output-skb-shared-infounder iperf3 loopback load); per-CPU summed to the total exactly in every run. Rotation exercised: per-segment deltas are incremental, and probe ordering genuinely changed between segments, which is what the site-matching (rather than positional) delta exists for.Kernel assumptions and untested paths
BPF_MAP_TYPE_PERCPU_HASHandbpf_map_update_elemare long-standing; no new helpers.The TC/XDP program-site path never executed.Now verified live (see update below).PT_REGS_IP, which the file already relies on forevent->function_ip, so arm64 should follow but is untested here.Follow-up, not included
Per-skb emit sequencing, which would let replay spot a gap inside one packet's chain — the difference between "hop 3 is missing for this skb" and "hop 3 never happened." That is a hot-path cost, so it should be opt-in, and it still cannot see a lost trailing event without this PR's per-probe counter behind it.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Update: program-site path now verified live, plus full live-suite run
The TC/XDP program-site attribution had been compile-verified only. Both paths have now been exercised under forced ring-buffer loss, in isolated network namespaces:
TC (
helper_classifieron a veth egress hook, 6,871 reserve failures):XDP (
xdp_passon a veth ingress hook, 346,756 reserve failures):function: nullalongside a populatedprogram_idis the tagged-key design working as intended — the two site kinds occupy separate fields, so nothing has to guess whether a value is a kernel address or a program id.Full live suite: 13/14 pass — tunnel, netns, stack, stack-lifetime, tc-program, xdp-program, skb-replacement, xdp-lineage, metadata, skb-filter, btf-dump, text-output, rotation.
live-bpf-helperfails (exit 4). It fails identically onmainat b378c01, so it is pre-existing on this host and not caused by this change. I have not diagnosed it; flagging rather than fixing, since it is out of scope here.