fix pipeline drops stats - #1706
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe worker now transmits only delivered pipeline outputs, tracks selected and attempted packets separately, and computes drops from all outputs. ChangesPacket transmission behavior
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@dataplane/src/drivers/kernel/worker.rs`:
- Around line 241-250: The output-packet handling around get_done must handle
missing completion verdicts in release builds instead of relying on
debug_assert!. Ensure packets without a verdict are assigned Delivered or Local
before this boundary; otherwise record the missing verdict as a pipeline error
and increment the appropriate drop accounting so num_out_pkts is fully accounted
for. Preserve the existing Delivered transmission path and counters.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 18819c41-51ad-4c5e-a8ed-81f1b248838a
📒 Files selected for processing (2)
dataplane/src/drivers/kernel/worker.rsnet/src/packet/mod.rs
There was a problem hiding this comment.
Pull request overview
This PR corrects dataplane “pipeline drop” statistics by ensuring packets intended for local kernel consumption are not treated as pipeline drops, and by preventing the kernel driver from attempting to transmit those local-only packets.
Changes:
- Allow
DoneReason::Localpackets to pass throughPacket::enforce()so they reach the end of the pipeline. - Update the kernel worker TX loop to transmit only
DoneReason::Deliveredpackets and to log “sent out of” using only the packets actually eligible for TX. - Simplify/remove packet-injection-related scaffolding in the worker’s pipeline drop accounting logic.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| net/src/packet/mod.rs | Adjusts Packet::enforce() to keep Local packets (not drop-filter them). |
| dataplane/src/drivers/kernel/worker.rs | Skips TX for Local packets; updates “pipeline drops” computation and TX logging; simplifies tx_packet assumptions. |
The prior logic computed the pipeline drops by comparing the number of packets received with the number of packets output by the pipeline. However, there are received packets that the pipeline wouldn't normally output (because they need not be transmitted) but which should not be considered drops: local packets. So, the old logic was accounting those as pipeline drops, providing false positives on the number of drops. Fix this by letting the pipeline stats stage let those packets through (until the pkt injection lands) and make sure the driver ignores them on xmit. Also, remove the scaffolding for packet injection. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
d6998e6 to
f74e422
Compare
The prior logic computed the pipeline drops by comparing the number of packets received with the number of packets output by the pipeline. However, there are received packets that the pipeline wouldn't normally output (because they need not be transmitted) but which should not be considered drops: local packets.
So, the old logic was accounting those as pipeline drops, providing false positives on the number of drops.
Fix this by letting the pipeline stats stage let those packets through (until the pkt injection lands) and make sure the driver ignores them on xmit.
Also, remove the scaffolding for packet injection.