fix(minvmd): bound outstanding vsock packets instead of coalescing reads - #926
Merged
Merged
Conversation
Replaces the coalescing patch from #921 with a packet-count backpressure approach that fixes the same bulk host->guest upload failure without changing delivery semantics. The failure is the guest kernel's per-skb receive-queue ceiling: it queues one skb per RW packet and, since Linux 6.12.92, resets the connection (RST + ENOBUFS) once (skb_queue_len + 1) * SKB_TRUESIZE(0) > buf_alloc. That is a second receive-side ceiling -- on packet count -- that the vsock credit protocol does not express, since credit meters bytes only. A sender that fragments the stream into sub-SKB_TRUESIZE(0) packets exhausts the count ceiling with the byte window mostly unused. Meter the packet count the same way the byte credit is metered. libkrun already tracks rx_cnt and peer_fwd_cnt; recording the tail rx_cnt of each emitted packet and retiring it when fwd_cnt passes it yields exactly the guest's skb_queue_len. Before emitting, if one more packet would approach buf_alloc / SKB_OVERHEAD, take the existing WaitForCredit path and wait for the peer to drain. SKB_OVERHEAD (1024) over-estimates the guest's real per-skb cost (576 B on arm64) so the bound stays under the true ceiling without hardcoding a guest-kernel constant. Includes the credit-request wakeup fix (push_credit_req must set signal_queue): the count ceiling makes reachable, during bulk transfer, the case where a connection must wait with no data packet in flight, which otherwise never wakes. Why replace coalescing: this does not change delivery granularity. send() stays 1:1 with recv() and packets are never delayed for batching -- it engages only as backpressure when the reader falls behind, exactly like byte credit, and is invisible otherwise. That is the objection coalescing draws (it "basically implements nagle's algorithm"), so this is the more defensible shape to carry and to upstream. Measured on a 12.8 MB host->guest transfer, libkrun the only variable: the current coalescing dylib and this one both pass; against the unpatched dylib 0 of 5 uploads complete, with this 6 of 6, a guest kernel instrumented on the rejection path logs zero resets, and the transfer still completes in ~200 ms (the reader keeps up, so the bound is rarely reached). Refs: #869
📝 WalkthroughWalkthroughUpdates libkrun’s Unix-vsock receive path to coalesce socket reads, bound guest outstanding packets using receive-queue depth, drain forwarded packets, and reliably signal credit-request updates. ChangesVsock receive path
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant UnixSocket
participant UnixProxy
participant GuestVsock
UnixProxy->>UnixSocket: recv(MSG_DONTWAIT)
UnixSocket-->>UnixProxy: data or EAGAIN
UnixProxy->>UnixSocket: poll(POLLIN) for more data
UnixProxy->>GuestVsock: emit coalesced RW packet
UnixProxy->>UnixProxy: record packet rx_cnt tail
GuestVsock-->>UnixProxy: forwarding counters and buffer allocation
UnixProxy->>UnixProxy: drain forwarded packets
UnixProxy-->>GuestVsock: wait when skb ceiling is reached
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@vendor/libkrun/patches/0001-vsock-bound-outstanding-packets.patch`:
- Around line 1-5: Update the patch’s subject line after “Subject: [PATCH]” to
use the repository’s Conventional Commit format, while preserving the existing
vsock scope and packet-boundary description. Do not alter the patch content or
metadata beyond the commit subject.
- Around line 137-144: Update skb_ceiling_reached to ensure an empty outstanding
queue is never considered at capacity, particularly when peer_buf_alloc is at or
below SKB_OVERHEAD; return false when outstanding is empty, while preserving the
existing ceiling calculation and capacity check for queued packets.
🪄 Autofix (Beta)
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: 193e2fcc-d58f-48d8-b179-2869b1731979
📒 Files selected for processing (2)
vendor/libkrun/patches/0001-vsock-bound-outstanding-packets.patchvendor/libkrun/patches/0001-vsock-coalesce-sub-skb.patch
💤 Files with no reviewable changes (1)
- vendor/libkrun/patches/0001-vsock-coalesce-sub-skb.patch
Comment on lines
+1
to
+5
| From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 | ||
| From: Norrie Taylor <norrie@minimal.dev> | ||
| Date: Wed, 22 Jul 2026 17:30:00 -0700 | ||
| Subject: [PATCH] vsock: bound outstanding packets by the guest's receive-queue | ||
| depth |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use a Conventional Commit subject.
vsock: bound outstanding packets... is not a Conventional Commit subject.
Proposed fix
-Subject: [PATCH] vsock: bound outstanding packets by the guest's receive-queue
+Subject: [PATCH] fix(vsock): bound outstanding packets by the guest receive queueAs per coding guidelines, “Use Conventional Commits as specified in docs/commit-conventions.md.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 | |
| From: Norrie Taylor <norrie@minimal.dev> | |
| Date: Wed, 22 Jul 2026 17:30:00 -0700 | |
| Subject: [PATCH] vsock: bound outstanding packets by the guest's receive-queue | |
| depth | |
| From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 | |
| From: Norrie Taylor <norrie@minimal.dev> | |
| Date: Wed, 22 Jul 2026 17:30:00 -0700 | |
| Subject: [PATCH] fix(vsock): bound outstanding packets by the guest receive queue |
🤖 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 `@vendor/libkrun/patches/0001-vsock-bound-outstanding-packets.patch` around
lines 1 - 5, Update the patch’s subject line after “Subject: [PATCH]” to use the
repository’s Conventional Commit format, while preserving the existing vsock
scope and packet-boundary description. Do not alter the patch content or
metadata beyond the commit subject.
Source: Coding guidelines
Comment on lines
+137
to
+144
| + fn skb_ceiling_reached(&self) -> bool { | ||
| + const SKB_OVERHEAD: usize = 1024; | ||
| + if self.peer_buf_alloc == 0 { | ||
| + return false; | ||
| + } | ||
| + let ceiling = self.peer_buf_alloc as usize / SKB_OVERHEAD; | ||
| + self.outstanding.len() + 1 >= ceiling | ||
| + } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Allow the first packet for small advertised receive buffers.
For peer_buf_alloc <= 1024, ceiling is 0 or 1, so an empty queue immediately satisfies 1 >= ceiling. This permanently returns WaitForCredit before any packet can be sent; forwarding cannot drain an empty queue, so credit updates cannot resolve the stall.
Proposed fix
- let ceiling = self.peer_buf_alloc as usize / SKB_OVERHEAD;
+ // Always permit one packet; otherwise small valid peer allocations
+ // deadlock before the queue can make progress.
+ let ceiling = (self.peer_buf_alloc as usize / SKB_OVERHEAD).max(2);
self.outstanding.len() + 1 >= ceiling📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| + fn skb_ceiling_reached(&self) -> bool { | |
| + const SKB_OVERHEAD: usize = 1024; | |
| + if self.peer_buf_alloc == 0 { | |
| + return false; | |
| + } | |
| + let ceiling = self.peer_buf_alloc as usize / SKB_OVERHEAD; | |
| + self.outstanding.len() + 1 >= ceiling | |
| + } | |
| fn skb_ceiling_reached(&self) -> bool { | |
| const SKB_OVERHEAD: usize = 1024; | |
| if self.peer_buf_alloc == 0 { | |
| return false; | |
| } | |
| // Always permit one packet; otherwise small valid peer allocations | |
| // deadlock before the queue can make progress. | |
| let ceiling = (self.peer_buf_alloc as usize / SKB_OVERHEAD).max(2); | |
| self.outstanding.len() + 1 >= ceiling | |
| } |
🤖 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 `@vendor/libkrun/patches/0001-vsock-bound-outstanding-packets.patch` around
lines 137 - 144, Update skb_ceiling_reached to ensure an empty outstanding queue
is never considered at capacity, particularly when peer_buf_alloc is at or below
SKB_OVERHEAD; return false when outstanding is empty, while preserving the
existing ceiling calculation and capacity check for queued packets.
twitchyliquid64
approved these changes
Jul 23, 2026
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
2 participants
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.
Replaces the coalescing patch merged in #921 with packet-count backpressure, which fixes the same bulk host→guest upload failure (#869) without changing delivery semantics. macOS source build; Linux needs the equivalent through
gominimal/pkgs.The failure, precisely
The guest kernel queues one skb per RW packet and, since Linux 6.12.92 (
a4f0b001782b), resets the connection (RST+ENOBUFS) once(skb_queue_len + 1) * SKB_TRUESIZE(0) > buf_alloc. That is a second receive-side ceiling — on packet count — that the vsock credit protocol does not express: credit meters bytes only. A sender that fragments the stream into sub-SKB_TRUESIZE(0)packets (576 B on arm64) exhausts the count ceiling with the byte window mostly unused. An instrumented guest kernel attributed every reset to this branch,rx_qlenexactly 455 (=262144 / 576), never the credit branch.The fix
Meter the packet count the same way byte credit is metered. libkrun already tracks
rx_cnt(bytes sent) andpeer_fwd_cnt(bytes the guest forwarded to userspace). Recording the tailrx_cntof each emitted packet in aVecDequeand retiring it whenfwd_cntpasses it yields exactly the guest'sskb_queue_len— one skb per RW packet, dropped when fully consumed. Before emitting, if one more packet would approachbuf_alloc / SKB_OVERHEAD, take the existingWaitForCreditpath and wait for the peer to drain.SKB_OVERHEAD(1024) over-estimates the guest's real per-skb cost (576 B) so the bound stays safely under the true ceiling without the VMM hardcoding a guest-kernel constant.Includes the credit-request wakeup fix (
push_credit_reqmust setsignal_queue): the count ceiling makes reachable, during bulk transfer, the case where a connection must wait with no data packet in flight, which otherwise never wakes.Why replace coalescing
This does not change delivery granularity.
send()stays 1:1 withrecv(); packets are never delayed for batching. It engages only as backpressure when the reader falls behind — exactly like byte credit — and is invisible otherwise.That directly answers the objection coalescing draws (Tom: it "basically implements nagle's algorithm", which breaks any consumer expecting
send()1:1 withrecv()). Framing it as a second dimension of the receive credit the protocol already defines is a far more defensible shape to carry locally and to take upstream.The one honest weakness:
SKB_OVERHEADis a guest-internal quantity. Mitigated by being a deliberate over-estimate; a maintainer conversation may want it configurable.Measurement
libkrun the only variable — same host, VMM binary, installed guest, 12.8 MB fixture.
The instrumented-kernel run is the important one: zero branch-B rejects means the bound held
skb_queue_lenunder the ceiling by construction, not by timing luck. Daemon-side confirmed the full 12,978,946 bytes received in ~200 ms — no throughput penalty, because the reader keeps up and the bound is rarely reached.The patch applies clean to the pinned libkrun commit
728df812and is self-contained (throttle + the signal-queue wakeup fix).Relationship to #921
#921 (coalescing) is already merged and also fixes the upload. This is an alternative with a better upstream story; it removes #921's patch and carries this one instead. If we'd rather keep both as defence in depth, say so and I'll rework — but coalescing's semantic change is exactly what this avoids, so carrying both re-introduces the thing we're trying not to ship.
Refs: #869
Note
Replace vsock coalescing patch with outstanding packet bounding in libkrun
VecDeque-based outstanding packet tracker inunix.rsto gate packet emission when the skb receive-queue ceiling is reached, draining forwarded entries on credit updates.unix.rsandtsi_stream.rsto setsignal_queue = true, ensuring an IRQ is raised when only a credit request is queued.Macroscope summarized 617c208.
Summary by CodeRabbit
Bug Fixes
Performance