perf(net-inject): fewer guest IRQs under multi-flow load - #491
Conversation
Stretch the inject gather window when batches look busy and shrink it when idle or very light, so multi-flow host→VM fires fewer guest IRQs without changing EVENT_IDX notify math. Add interval inject stats and a mechanism-model example.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — one design concern and a nit, both non-blocking.
Reviewed changes — an adaptive RX-inject coalesce window replacing the fixed 200 µs timeout, aiming to cut guest IRQs under multi-flow load where GSO can't merge across flows.
- Add
CoalescePolicy(coalesce.rs) — batch-size-driven window in[100, 800] µs: expand ×3/2 when busy (filled_earlyorbatch >= 12), shrink ×3/4 when quiet (batch < 4), hold otherwise. Classification deliberately ignores wall-clockspent. - Rework the inject loop (
inject.rs) — moveBATCH_SIZEintocoalesce, drop theCOALESCE_TIMEOUTconst, and centralize flush + stats +observein afinish_batchclosure covering the normal, exhaustion, disconnect, and idle exits. Exhaustion flushes passallow_adapt=false. - Add
InjectStats(stats.rs) — lifetime totals for shutdown logs plus a 1 Hz interval debug line (frames/irqs/frames-per-IRQ/current window). - Add
coalesce_simexample — no-VM discrete-event model of the mechanism. - Swap dependency
arcbox-virtio→arcbox-virtio-core— the crate only usedGuestMemWriter/QueueConfig/SplitQueue, all re-exported by core;queue.rs/lib.rsare import/doc updates only.
ℹ️ Nitpicks
inject.rs:184-211— the exhaustion flush correctly passesallow_adapt=false, but when exhaustion truncates the gather tobatch=0and the nextrecv_timeoutreturns empty, the normal end-of-gatherobserve(0, …, true)at line 217 shrinks the window. That mildly contradicts the "descriptor-exhaustion flushes do not adapt" intent (shrinking during a storm kicks the guest more often). Self-correcting once load resumes, so low impact — worth a comment if the intent is stricter.
Claude Opus ｜ 𝕏
There was a problem hiding this comment.
Pull request overview
This PR introduces an adaptive RX inject coalescing gather window in virt/arcbox-net-inject to reduce guest IRQ/vCPU kick frequency under multi-flow (high frame-rate) host→VM traffic, while keeping single-stream GSO behavior stable. It also adds lightweight per-thread stats logging and a no-VM simulation example to help tune/validate the policy.
Changes:
- Add
CoalescePolicyto adapt the gather window between 100–800µs based on observed batch activity. - Add
InjectStatsfor lifetime + 1Hz interval debug counters (frames/flushes/IRQs/window). - Switch
arcbox-net-injectto depend onarcbox-virtio-coretypes and add acoalesce_simdiscrete-event example.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| virt/arcbox-net-inject/src/stats.rs | New inject-thread stats + 1Hz debug logging for coalesce effectiveness. |
| virt/arcbox-net-inject/src/coalesce.rs | New adaptive gather-window policy + unit tests. |
| virt/arcbox-net-inject/src/inject.rs | Integrates adaptive window + unified batch finishing (flush/stats/policy). |
| virt/arcbox-net-inject/src/queue.rs | Switches SplitQueue imports to arcbox_virtio_core. |
| virt/arcbox-net-inject/src/lib.rs | Exposes new coalesce and stats modules. |
| virt/arcbox-net-inject/examples/coalesce_sim.rs | Adds a no-VM coalescing simulation to model IRQ reduction. |
| virt/arcbox-net-inject/Cargo.toml | Replaces arcbox-virtio dependency with arcbox-virtio-core. |
| Cargo.lock | Updates lockfile for the dependency rename. |
Comments suppressed due to low confidence (1)
virt/arcbox-net-inject/src/inject.rs:150
- The gather
remainingbudget is computed once before the recv loop, so multiplerecv_timeout(remaining)calls can block past the intended coalescing window (latency bound) once time has already been spent injecting/receiving. Recomputeremainingfromloop_startinside the loop (or use a deadline) so the window is a true upper bound per gather.
// Phase 3: Drain channel frames (classifier / DHCP / DNS / ARP).
// Use the remaining adaptive window after inline polling.
let elapsed = loop_start.elapsed();
let remaining = window.saturating_sub(elapsed);
while (batch as usize) < BATCH_SIZE {
// Use the remaining timeout for the first recv, then zero
// for subsequent ones to drain without blocking.
let timeout = if batch == 0 && inline_conns.is_empty() {
// No inline conns and nothing batched yet — block for
// the full coalescing window.
window
} else if remaining.is_zero() {
// Timeout already consumed by inline polling — try_recv only.
Duration::ZERO
} else {
remaining
};
| // Always observe (incl. batch == 0 idle) so the window can decay. | ||
| policy.observe(batch, filled_early, allow_adapt); | ||
| stats.sync_policy_events(policy.expand_events(), policy.shrink_events()); | ||
| stats.window_us = window_budget_us; | ||
| stats.maybe_log(); | ||
| }; |
There was a problem hiding this comment.
Addressed in e84e059: sync_from_policy / note_window update window_us_max on every finish_batch, including empty idle gathers.
| pub fn maybe_log(&mut self) { | ||
| let now = Instant::now(); |
There was a problem hiding this comment.
Addressed in e84e059: maybe_log returns immediately when !tracing::enabled!(Level::DEBUG) so the hot path skips Instant::now() under normal logging.
| irqs_fired = d_irqs, | ||
| irqs_suppressed = d_suppressed, | ||
| frames_per_irq = format!("{fpi:.1}"), | ||
| avg_batch = format!("{avg_batch:.1}"), |
There was a problem hiding this comment.
Already fixed in 8f15143 (and kept): interval rates are logged as f64 fields without format!.
Replace the multi-arg finish_batch closure with a method + FinishBatch args struct, keep InjectStats fields private behind getters, and sync policy events in one place. Log interval rates as f64 without format!.
| // Use the remaining timeout for the first recv, then zero | ||
| // for subsequent ones to drain without blocking. | ||
| let timeout = if batch == 0 && inline_conns.is_empty() { | ||
| // No inline conns and nothing batched yet — block for | ||
| // the full coalescing timeout. | ||
| COALESCE_TIMEOUT | ||
| // the full coalescing window. |
There was a problem hiding this comment.
Addressed in e84e059: remaining is recomputed from loop_start.elapsed() each iteration of the recv loop so the gather window is a real deadline.
| // Final flush on shutdown if a partial batch was left mid-iteration | ||
| // (running flag cleared between pushes). | ||
| if fire { | ||
| self.flush_interrupt(&queue, fire); | ||
| } |
There was a problem hiding this comment.
Addressed in follow-up: removed the unreachable post-loop if fire flush. Every iteration ends in finish_batch, which flushes when batch > 0.
There was a problem hiding this comment.
ℹ️ No new issues — one doc-comment nit inline, non-blocking.
Reviewed changes — this run covers the follow-up refactor commit that reshapes the inject-thread plumbing added in the first commit; no coalesce-policy logic changed.
- Extract
finish_batchinto a method — the multi-arg closure becameRxInjectThread::finish_batchtaking aFinishBatch<'_>argument struct, replacing the nine positional parameters at each call site. - Encapsulate
InjectStats— struct fields are now private with#[must_use] const fngetters (frames_injected/flushes/irqs_fired/window_us_max), and the two shutdowninfo!sites use them. - Fold window tracking into
sync_from_policy— replacessync_policy_events(expand, shrink), now also settingwindow_usso the closure no longer writes the private field directly. - Drop
format!in the 1 Hz debug log —fpi/avg_batchare passed totracing::debug!as baref64instead of pre-formatted strings.
Claude Opus ｜ 𝕏
Greptile SummaryThis PR adds adaptive interrupt coalescing to the guest RX injection path. The main changes are:
Confidence Score: 5/5This looks safe to merge. The simulator now states that its IRQ figures model non-empty flushes with EVENT_IDX disabled. Production statistics distinguish flushes from IRQs that actually fire. No blocking issue related to the earlier reporting problem remains. No files need additional attention.
 T-Rex LogsWhat T-Rex did
Important Files Changed
Reviews (2): Last reviewed commit: "docs(net-inject): note coalesce_sim coun..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Classify busy/quiet by implied fps over the gather budget so an elevated window does not trap a later single stream as busy. Track window_us_max on every finish_batch, gate maybe_log on DEBUG, recompute recv remaining each iteration, and skip idle adapt once after descriptor exhaustion.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c85bba0b3d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let filled_early = (batch as usize) >= BATCH_SIZE; | ||
| finish_batch( |
There was a problem hiding this comment.
For sustained but non-bursty RX streams, this labels any BATCH_SIZE gather as filled_early, but the loop computes remaining once before the receive loop and then reuses it for every recv_timeout, so reaching 256 frames only proves each inter-arrival gap was below the current window, not that 256 frames arrived within the window. A single GSO stream around 37k fps can therefore fill the batch over several milliseconds, get classified as busy, and expand to the 800µs window even though the policy and commit message say that band should hold, adding much larger RX/ACK latency on the production RX engine.
AGENTS.md reference: virt/AGENTS.md:L16-L20
Useful? React with 👍 / 👎.
| let busy = filled_early || batch >= EXPAND_MIN_BATCH; | ||
| let quiet = batch < QUIET_MAX_BATCH; |
There was a problem hiding this comment.
When the inline RX path delivers one large host read, poll_inline_conns adds num_used to batch, so a single GSO frame that spans 12+ RX buffers is treated here as a multi-flow/high-fps batch and expands the window. That contradicts the intended single-stream hold behavior and can push ordinary single-flow inline traffic to the 800µs coalescing delay even though only one packet was delivered; the adaptive threshold needs packet/read count or an inline-specific signal rather than raw used-entry count.
AGENTS.md reference: virt/AGENTS.md:L71-L78
Useful? React with 👍 / 👎.
| /// | ||
| /// At 200 µs: ~12 frames ≈ 60k fps — above single-stream GSO (~7 @ 37k fps | ||
| /// / 10 Gbps) and into multi-flow territory. | ||
| pub const EXPAND_MIN_BATCH: u16 = 12; |
There was a problem hiding this comment.
Because the busy cutoff stays fixed at 12 entries while the window can grow to 800µs, a connection that should be in the documented single-stream band after a multi-flow burst no longer holds: ~37k fps is only ~7 frames at 200µs, but it is ~30 frames at 800µs, so batch >= EXPAND_MIN_BATCH keeps the policy pinned at the maximum until traffic becomes nearly idle. This leaves normal single-stream host→VM traffic paying the higher coalescing delay after any prior busy period instead of returning to the 200µs regime.
AGENTS.md reference: virt/AGENTS.md:L16-L20
Useful? React with 👍 / 👎.
…ption Remove unreachable post-loop fire flush and note that coalesce_sim counts flushes, not EVENT_IDX-suppressed guest IRQs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
virt/arcbox-net-inject/src/inject.rs:235
- This "final flush on shutdown" block appears to be dead code:
fireis reset tofalseat the end of every gather iteration, and the loop onlybreaks at the top before starting a new iteration. That meansfirecan never betrueafter the loop, so this branch is unreachable and misleading.
// Every loop iteration ends in finish_batch (which flushes when
// batch > 0), so no trailing flush is needed on exit.
tracing::info!(
frames = stats.frames_injected(),
virt/arcbox-net-inject/src/inject.rs:132
- The comment says only the first
recv_timeoutshould block and subsequent receives should be non-blocking, but the code blocks up toremainingon every iteration until the gather deadline is exhausted. This mismatch makes the gather/timeout behavior harder to reason about.
// Use the remaining timeout for the first recv, then zero
// for subsequent ones to drain without blocking.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — the three commits since the prior pullfrog review (8f15143): a rate-based rework of the coalesce busy/quiet test, an inject-loop cleanup, and a sim doc clarification. Both prior pullfrog threads are addressed and now resolved.
- Rate-based classification (
coalesce.rs) — replaced the raw-countEXPAND_MIN_BATCHwithEXPAND_MIN_FPS=60_000and animplied_fps(batch, window_budget_us)helper;observenow takes the gather budget. An elevated window decays once fps drops below multi-flow, so a single stream following a multi-flow burst no longer pins at the 800µs cap — the window-ratchet concern from the first review. - Post-exhaustion idle guard (
inject.rs) — addedskip_idle_adaptso the first empty gather after descriptor exhaustion passesallow_adapt=falseinstead of shrinking the window mid-storm, addressing the earlier exhaustion-shrink nit. - Per-gather deadline (
inject.rs) —remainingis recomputed fromloop_startinside the recv loop, making the window a true upper bound per gather. - Dead shutdown flush removed (
inject.rs) — every loop iteration ends infinish_batch(which flushes whenbatch > 0) and resetsfire, so the trailing post-loop flush was unreachable. maybe_logDEBUG gate (stats.rs) — early-returns when DEBUG is disabled to keepInstant::now()off the hot path, and the stale allocation comment is gone;note_windowfolds the shared window high-water update.- Sim IRQ clarification (
coalesce_sim.rs) — docs/print note that the model'sirqsare non-empty flushes with EVENT_IDX off (production can suppress further).
Verified the mixed→single transition decays 800µs→200µs and that cold single-stream holds at the 200µs default. System-level (HV iperf) validation is still deferred to a follow-up per the PR description — sim-only so far, unchanged by these commits.
Claude Opus ｜ 𝕏
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
virt/arcbox-net-inject/src/inject.rs:135
- The
timeoutselection is effectively justremainingunless the window is already consumed; thebatch == 0 && inline_conns.is_empty()branch is redundant, and the comment about “first recv then zero for subsequent” no longer matches the implementation. Simplify this to a single remaining-deadline check so the gather/deadline behavior is unambiguous.
// Use the remaining timeout for the first recv, then zero
// for subsequent ones to drain without blocking.
let timeout = if batch == 0 && inline_conns.is_empty() {
// No inline conns and nothing batched yet — block for
// the full coalescing window (or what's left of it).
| // Always observe (incl. batch == 0 idle) so the window can decay. | ||
| f.policy | ||
| .observe(f.batch, f.window_budget_us, f.filled_early, f.allow_adapt); | ||
| f.stats.sync_from_policy(f.policy, f.window_budget_us); | ||
| f.stats.maybe_log(); |
|
Thanks for your contribution! Could you resolve all bot reviews first? Remember to click "Resolve conversation" after replying to them. |
Multi-flow host→VM still costs about half single-stream on the same
CPU. Most of the extra time is the inject thread kicking the guest:
GSO only merges within one flow, so more flows mean more frames and
more interrupts for the same bytes.
We already batch (256 / 200 µs) and skip kicks when the guest is
polling (EVENT_IDX). The fixed 200 µs window is fine for one fat
stream; under multi-flow load it's still too chatty.
This stretches the gather window when a full-window batch looks busy
(≥12 used entries, or we hit the 256 cap) up to ~800 µs, and shrinks
it when the gather is idle or very light (<4). Single-stream GSO
(~7 frames / 200 µs) holds. Notify math and fences are unchanged.
Batch size only — not wall-clock spend — so a timeout full of idle
recv_timeoutdoes not look “busy”. Empty idle gathers still observeso the window can decay after load. Descriptor-exhaustion flushes do
not adapt.
Inject debug logs 1 Hz interval frames / irqs / frames-per-IRQ and
the current window. Mechanism model (no VM):
System numbers (HV iperf) on follow-up when available.
Also depends on
arcbox-virtio-coreonly (this crate only used core types).Related: #242
Not touching multi-queue, retransmit, or the IRQ delivery path.