Skip to content

virtio: build descriptor chains without a per-packet allocation#4034

Open
bitranox wants to merge 1 commit into
microsoft:mainfrom
bitranox:virtio-queue-descriptor-walk
Open

virtio: build descriptor chains without a per-packet allocation#4034
bitranox wants to merge 1 commit into
microsoft:mainfrom
bitranox:virtio-queue-descriptor-walk

Conversation

@bitranox

@bitranox bitranox commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

virtio: build descriptor chains without a per-packet allocation

The queue built a fresh Vec of payload buffers for every descriptor chain and
walked the chain through a Result-collecting iterator pipeline. On a saturated
virtio-net receive queue that pipeline and its allocation measured about a third
of the worker thread, against under 5% for the copy into guest memory that is the
actual work.

Walk the chain with a plain loop into a SmallVec instead. The inline capacity
covers the short chains, so those never reach the allocator. Longer chains spill
to the heap and stay bounded by the queue size, which the existing chain-length
check already enforces.

Packed rings also re-read the tail descriptor from guest memory after the walk,
only to recover its buffer id for the completion. Capture that id as the walk
passes it. For a single-descriptor chain that halves the descriptor reads, and the
captured id identifies the buffer the guest offered even if the guest rewrites the
ring afterwards.

virtio-net reuses its receive id buffer across wakes rather than allocating one per
process_virtio_rx call. Its length is bounded by the receive queue size, so the
initial capacity is also the maximum.

Measured on a receive path saturated by a fragmented-datagram burst, each binary
against its own base at matched offered rates: 342,660 to 403,773 packets per
second drained on one guest and 328,118 to 402,118 on another, with the host tap
drop rate falling from 49.1 to 39.1 percent and from 37.1 to 18.8 percent.

Two gaps in the queue tests are fixed alongside. The packed indirect tables wrote
buffer id 0, the same value the head descriptor carries, so a chain that took its
completion id from inside the indirect table was indistinguishable from a correct
one. And no test built a chain past the payload inline capacity, so the heap-spill
path had no coverage; both ring layouts now exercise it and assert buffer count and
order.

One note on the inline capacity and on where the measurements come from. A guest only
uses the big-packets receive layout, which posts MAX_SKB_FRAGS + 2 buffers per packet
and measured 19 here, when a guest-side segmentation feature is advertised without
VIRTIO_NET_F_MRG_RXBUF. main advertises neither GUEST_TSO nor GUEST_USO, so a Linux
guest on main stays in the small-packets layout and posts single-descriptor chains,
which the inline capacity already covers. The 19-buffer chains and the numbers above
come from a tree of mine that does advertise them; I had this wrong in #4035 and have
corrected it there. Raising the inline bound would trade the allocation for a larger
memcpy, because the array is inline in a value moved several times per packet, so it
looked like the wrong lever either way. Offering mergeable receive buffers removes the
amplification instead, which is #4035.

The queue built a fresh Vec of payload buffers for every descriptor chain and
walked the chain through a Result-collecting iterator pipeline. On a saturated
virtio-net receive queue that pipeline and its allocation measured about a third
of the worker thread, against under 5% for the copy into guest memory that is the
actual work.

Walk the chain with a plain loop into a SmallVec instead. The inline capacity
covers the short chains, so those never reach the allocator. Longer chains spill
to the heap and stay bounded by the queue size, which the existing chain-length
check already enforces.

Packed rings also re-read the tail descriptor from guest memory after the walk,
only to recover its buffer id for the completion. Capture that id as the walk
passes it. For a single-descriptor chain that halves the descriptor reads, and the
captured id identifies the buffer the guest offered even if the guest rewrites the
ring afterwards.

virtio-net reuses its receive id buffer across wakes rather than allocating one per
process_virtio_rx call. Its length is bounded by the receive queue size, so the
initial capacity is also the maximum.

Measured on a receive path saturated by a fragmented-datagram burst, each binary
against its own base at matched offered rates: 342,660 to 403,773 packets per
second drained on one guest and 328,118 to 402,118 on another, with the host tap
drop rate falling from 49.1 to 39.1 percent and from 37.1 to 18.8 percent.

Two gaps in the queue tests are fixed alongside. The packed indirect tables wrote
buffer id 0, the same value the head descriptor carries, so a chain that took its
completion id from inside the indirect table was indistinguishable from a correct
one. And no test built a chain past the payload inline capacity, so the heap-spill
path had no coverage; both ring layouts now exercise it and assert buffer count and
order.
Copilot AI review requested due to automatic review settings July 26, 2026 11:43
@bitranox
bitranox requested a review from a team as a code owner July 26, 2026 11:43
@github-actions

Copy link
Copy Markdown

⚠️ Unsafe Code Detected

This PR modifies files containing unsafe Rust code. Extra scrutiny is required during review.

For more on why we check whole files, instead of just diffs, check out the Rustonomicon

@github-actions github-actions Bot added the unsafe Related to unsafe code label Jul 26, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR optimizes virtio queue processing on the receive hot path by eliminating per-descriptor-chain allocations and reducing packed-ring descriptor rereads. It also reuses a scratch buffer in virtio-net RX processing and strengthens queue tests to cover previously untested correctness gaps (packed indirect buffer-id handling and the heap-spill path).

Changes:

  • Replace per-chain Vec allocation + iterator pipeline with a loop that builds payloads into an inline-capacity SmallVec, spilling to heap only for longer chains.
  • For packed rings, capture the primary-ring tail buffer id during the chain walk so completion doesn’t need to reread guest memory.
  • Reuse an RX id buffer across process_virtio_rx calls in virtio-net, and extend queue tests to validate indirect buffer-id handling and long-chain spill behavior.

Reviewed changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated no comments.

Show a summary per file
File Description
vm/devices/virtio/virtio/src/tests.rs Uses a non-zero sentinel buffer id in packed indirect tables and adds long-chain tests to exercise heap-spill payload handling.
vm/devices/virtio/virtio/src/queue/packed.rs Updates packed completion context construction to accept a captured buffer id directly.
vm/devices/virtio/virtio/src/queue.rs Builds payloads into SmallVec, records chain tail metadata for completions, and removes the iterator-based chain reader.
vm/devices/virtio/virtio/src/common.rs Switches VirtioQueueCallbackWork.payload to the new VirtioQueuePayloadVec type.
vm/devices/virtio/virtio/Cargo.toml Adds smallvec dependency for the virtio crate.
vm/devices/virtio/virtio_net/src/lib.rs Reuses a retained Vec<RxId> across RX passes to avoid per-call allocation.
Cargo.lock Records the added smallvec dependency in the lockfile.

@jstarks

jstarks commented Jul 26, 2026

Copy link
Copy Markdown
Member

It's been on my todo list to do something about this Vec. But I was thinking we should either have the frontends grab one descriptor at a time via some Iterator<Item = Result<_, _>> type thing, or provide a &mut Vec<_> when reading a descriptor chain--at least virtio-net can reuse the Vec across descriptor chains.

I haven't looked into doing this in detail yet. Either approach changes the programming model, but I think that's OK.

@bitranox

Copy link
Copy Markdown
Contributor Author

Agreed, and both of those sound better than what I did here, and the &mut Vec form looks like it subsumes most of the win in this PR. Let me try to tackle it.

The part I will need to work out is virtio-net RX. It holds a posted receive buffer's descriptor payload well past the read, because it needs the guest addresses when a packet eventually lands, and push_guest_addresses reads them before any packet exists. With many buffers outstanding at once a single reused Vec will not cover that, so I may end up with the iterator or &mut Vec form for frontends that consume a chain and drop it, and per buffer storage for the retained ones. I will see how it actually looks in code rather than guess.

One note for sizing the inline case, which ties into #4035: the 19 descriptor chains I measured come from our tree advertising GUEST_TSO, which as you said main does not. A guest in the small packets layout, or in the mergeable layout once MRG_RXBUF lands alongside GUEST_TSO, posts single descriptor chains.

@bitranox

Copy link
Copy Markdown
Contributor Author

One more data point on the reuse question, since it changes the shape rather than just the size.

virtio-blk retains the chain as well, so this is not only a virtio-net constraint. It moves the work into process_request, which is pushed onto a FuturesUnordered as a boxed future, and the work comes back out inside IoCompletion when the I/O finishes. So the payload stays live for the duration of the disk request, with as many outstanding as there are queued I/Os.

That means both hot frontends need one live buffer per in-flight request rather than one buffer reused across chains. A single &mut Vec the caller keeps does not fit either of them; the reuse would have to come from handing the buffer back at completion and taking the next one from a freelist. A frontend that reads a chain and drops it immediately could still take the iterator form.

Not arguing for either shape, just flagging that the constraint applies to both frontends before you settle on one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

unsafe Related to unsafe code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants