File each lost observation against the packet it belonged to - #18
Conversation
Attributing a hole to a probe narrowed where to look but left every absence unusable: knowing that ip_rcv failed to emit somewhere says nothing about whether a particular packet's missing hop was never reached or merely never observed. The identity of the packet is already in scope wherever a reserve fails, so record the loss itself rather than inferring it later: a table keyed by packet and probe, written only on the failure path. Deliberately a plain hash and not an LRU. A full LRU evicts silently, and a silently evicted entry would make a packet look like it lost nothing, which is exactly the claim this table exists to support. A plain hash refuses the insert and counts the refusal instead. While that count is zero the table is exhaustive, so a packet absent from it provably lost nothing and a function missing from its chain was never reached. explain now rules per packet: complete, lost, or unknown. Reserve failures are the only loss kind a packet can own. A recursion miss never reaches the emit path, and a decode, enrichment or output failure discards a record after the kernel has handed it over, by which point nothing knows which packet it described. Any of those leaves a hole no packet can be cleared of, so clearing any packet requires all of them to be zero. A packet can now appear in the ledger while appearing nowhere in the capture. That is a packet whose every observation was dropped, which previously left no trace at all; replay counts these separately from the packets it actually observed. Carries the review fixes applied to the parent branch through to the new field: skb_loss dedupes revisited keys from bpf_map_get_next_key iteration, and both breakdowns are parameters of into_reliability so neither can be left unset by a future call site. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 23 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 (8)
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 |
1 participant
Supersedes #16, which GitHub auto-closed when its base branch was deleted as part of merging #15. Same content, rebuilt on top of
main— see the note at the end.Evidence change
#15 narrowed a hole to a probe, but left every absence unusable. Knowing
ip_rcvfailed to emit somewhere says nothing about whether a particular packet's missing hop was never reached or merely never observed, socomplete: falsestill downgraded every absence in the capture.This files each hole against the packet it belonged to. Two packets from the same globally-incomplete capture (16,382 reserve failures), on a live kernel:
For the second packet, a function absent from those 10 hops was never reached. That is "this function was never hit" separated from "we lost that one" — per packet, inside a capture that is not complete overall.
Closes the question from the DEV thread that prompted #15.
Why a plain hash, not an LRU
This is the load-bearing decision. A full LRU evicts silently, and a silently evicted entry would make a packet look like it lost nothing — turning the feature into a lie under exactly the load that makes it matter. A plain hash refuses the insert and counts the refusal in
kernel_skb_loss_unattributed.That refusal is what makes the negative claim provable rather than merely likely: while the counter is zero the table is exhaustive, so a packet absent from it lost nothing. An LRU would trade that proof for a bounded amount of memory.
Reserve failures are the only loss a packet can own
loss_is_fully_attributed()requires the skb table to be exhaustive andkernel_recursion_misses,userspace_decode_failures,userspace_enrichment_failuresandoutput_failuresto all be zero. A recursion miss never reaches the emit path; a decode, enrichment or output failure discards a record after the kernel handed it over, when nothing knows which packet it described. Any of those leaves a hole no packet can be cleared of.Read failures are deliberately excluded — they degrade fields on an event that was still emitted, and are already visible on it.
An earlier draft checked only the skb table and would have cleared packets in captures with output loss. Test
loss_that_no_packet_owns_defeats_attributionpins each case.Design note: this is not what the follow-up in #15 described
#15 proposed per-skb emit sequencing — stamp a counter into every record, infer loss from gaps. The identity turns out to be in scope at all 16 failure sites, which makes recording the loss directly strictly better:
The trailing-event blind spot decided it: #15 already noted sequencing could not see a lost last hop. This has no such gap, and needs no flag because it costs nothing until something is already failing.
Packets that were previously invisible
The first draft of the summary line printed
363 of 320 packets affected. Not a formatting bug — 104 packets lost every observation, so they appear in the ledger but nowhere in the capture. A wholly-dropped chain previously left no trace at all. Replay now reports these asskbs_lost_entirely, separate fromdistinct_skbs, which counts only packets actually observed.Change design
BPF_MAP_TYPE_HASH, 4096 entries, preallocated. Written only on the failure path. Overflow is counted, never silent.kernel_skb_loss_unattributedis the single gate on whether any packet may be cleared; non-zero collapses the claim for all of them at once.#[serde(default)]and absent fromrequired; the pre-existing fixture still parses and replays.Checks run
make checkandmake build, both exit 0.explainverdicts, missing-footer, and entirely-lost packets.unattributed=0) → per-packetcomplete/lost; heavy loss (table full at 4096, 11,866 unfiled) →unknownfor every packet; lossless →complete. Filed + unfiled reconciled againstkernel_reserve_failuresexactly.Review fixes carried over
CodeRabbit's findings on #15 apply to this field too and are included:
skb_loss()collects through a map keyed by site rather than a list.keys()isbpf_map_get_next_key()iteration, which can revisit a key when entries are inserted while it runs, and probes keep firing throughout. A duplicate matters more here than in the per-probe table: it would put the same packet in the ledger twice, and that ledger is what a reader consults to decide whether a packet lost anything.kernel_loss_by_skbis a parameter ofinto_reliabilityrather than a field the caller patches in afterwards, so it cannot be left unset.Untested
Unchanged from #15: the TC/XDP program-site path never executes here.Now verified live. Both paths were exercised under forced loss in isolated network namespaces, and the per-packet ledger carries the program id the same way the per-probe table does:explainrules correctly on TC-traced packets from a lossy capture —{"verdict": "lost", "observations_lost": 1}for an affected packet,{"verdict": "complete"}for one the ledger clears.Full live suite: 13/14 pass.
live-bpf-helperfails identically onmainat b378c01, so it is pre-existing on this host, not caused by this change.4096 entries is sized for the light-loss case this feature is for. Under heavy loss the table fills and every verdict degrades to
unknown, which is correct but means the guarantee is only available when loss is bounded. That ceiling is a judgement call, not a measurement.🤖 Generated with Claude Code
Note on how this branch was rebuilt
#15 was squash-merged, so its original commits are not in
main's history and this branch could not simply retarget — GitHub closed the original PR when the base branch went away.The branch was rebuilt by replaying this change's net delta onto the new
main. Verified equivalent:git diffbetween the rebuilt branch and the reviewed one is empty acrosscrates/anddocs/architecture.md. The only difference is the presence of #17's script and validation-doc changes, which the rebuilt branch inherits frommainand the old branch predated.make checkpasses on the rebuilt branch, and the live gates were re-run after the review fixes:live-tc-program,live-xdp-program,live-stack-lifetime,live-metadata,live-rotationall pass, plus a fresh lossy capture confirming no duplicate entries in either breakdown (2 probe entries / 2 distinct, 1427 packet entries / 1427 distinct,kernel_skb_loss_unattributed = 0).