On Tue, Jun 30, 2026 at 11:53:04AM +0200, Paolo Abeni wrote:
On 6/26/26 3:48 PM, Stefano Garzarella wrote:
From: Stefano Garzarella <[email protected]>
When many small packets accumulate in the receive queue, the skb overhead
can exceed buf_alloc even while the payload is within bounds. This causes
virtio_transport_inc_rx_pkt() to reject packets, leading to connection
resets during large transfers under backpressure.
The issue was reported by Brien, who has a reproducer, but it is also
easily reproducible with iperf-vsock [1] using a small packet size:
iperf3 --vsock -c $CID -l 129
which fails immediately without this patch but with commit 059b7dbd20a6
("vsock/virtio: fix potential unbounded skb queue").
Inspired by TCP's tcp_collapse() which solves a similar problem, add
virtio_transport_collapse_rx_queue() that walks the receive queue and
re-copies data into compact linear skbs to reduce the overhead.
The collapse is triggered from virtio_transport_recv_enqueue() when
virtio_transport_inc_rx_pkt() fails. A pre-scan counts the eligible bytes
to size each allocation precisely, avoiding waste for isolated small
packets. Partially consumed skbs are kept as-is to preserve
buf_used/fwd_cnt accounting, EOM-marked skbs to maintain SEQPACKET
message boundaries, and skbs already larger than the collapse target
because they already have a good data-to-overhead ratio.
[1] https://github.com/stefano-garzarella/iperf-vsock
Fixes: 059b7dbd20a6 ("vsock/virtio: fix potential unbounded skb queue")
Cc: [email protected]
Reported-by: Brien Oberstein <[email protected]>
Closes:
https://lore.kernel.org/netdev/[email protected]/
Tested-by: Brien Oberstein <[email protected]>
Signed-off-by: Stefano Garzarella <[email protected]>
---
net/vmw_vsock/virtio_transport_common.c | 148 +++++++++++++++++++++++-
1 file changed, 146 insertions(+), 2 deletions(-)
diff --git a/net/vmw_vsock/virtio_transport_common.c
b/net/vmw_vsock/virtio_transport_common.c
index 09475007165b..304ea424995d 100644
--- a/net/vmw_vsock/virtio_transport_common.c
+++ b/net/vmw_vsock/virtio_transport_common.c
@@ -420,6 +420,137 @@ static int virtio_transport_send_pkt_info(struct
vsock_sock *vsk,
return ret;
}
+static bool virtio_transport_can_collapse(struct sk_buff *skb,
+ unsigned int size)
Why passing a `size` argument here? AFAICS the actual argument is always
a constant and IMHO rightfully so.
This comes from a previous implementation where this was not constant.
With the current code, I agree that a macro should be better.
I'll fix it.
+{
+ /* skbs that are partially consumed, mark a SEQPACKET message boundary,
+ * or are already large enough should not be collapsed: they either
+ * need special accounting, carry protocol state, or already have a
+ * good data-to-overhead ratio.
+ */
+ if (VIRTIO_VSOCK_SKB_CB(skb)->offset)
+ return false;
+ if (le32_to_cpu(virtio_vsock_hdr(skb)->flags) & VIRTIO_VSOCK_SEQ_EOM)
+ return false;
+ if (skb->len >= size)
+ return false;
+ return true;
+}
+
+/* Iterate through the packets in the queue starting from the current skb to
+ * count the number of bytes we can collapse.
+ */
+static unsigned int
+virtio_transport_collapse_size(struct sk_buff *skb,
+ struct sk_buff_head *queue,
+ unsigned int max_size)
+{
+ unsigned int target = skb->len - VIRTIO_VSOCK_SKB_CB(skb)->offset;
+
+ while ((skb = skb_peek_next(skb, queue)) &&
+ virtio_transport_can_collapse(skb, max_size)) {
+ unsigned int len = skb->len - VIRTIO_VSOCK_SKB_CB(skb)->offset;
+
+ if (len > max_size - target)
+ return target;
+
+ target += len;
+ }
+
+ return target;
+}
+
+/* Called under lock_sock when skb overhead exceeds the budget. */
+static void virtio_transport_collapse_rx_queue(struct virtio_vsock_sock *vvs)
+{
+ /* Use the same linear allocation threshold as virtio_vsock_alloc_skb()
+ * to avoid adding pressure on the page allocator.
+ */
+ unsigned int collapse_max = SKB_MAX_ORDER(VIRTIO_VSOCK_SKB_HEADROOM,
+ PAGE_ALLOC_COSTLY_ORDER);
+ struct sk_buff *skb, *next_skb, *new_skb = NULL;
+ struct sk_buff_head new_queue;
+
+ __skb_queue_head_init(&new_queue);
+
+ skb_queue_walk_safe(&vvs->rx_queue, skb, next_skb) {
If the queue is relevantly big, walking all of it may take a significant
amount of time/cache misses and causes traffic burstines. I think you
could add an additional stop condition, i.e. when the current queue size
is below a reasonable threshold (allowing the current packet to be
inserted plus some more slack).
Makes sense, any suggestion on the threshold?
I was thinking something like this: merge until we have space for at
least 2 skbs (because for now we estimate the overhead based on the
number of skbs, but in the future I'd like to support truesize), but
still trying to fill collapse_max as much as possible.
Does that make sense, or should we be more aggressive?
/P
+ struct virtio_vsock_hdr *hdr = virtio_vsock_hdr(skb);
+ u32 src_off = VIRTIO_VSOCK_SKB_CB(skb)->offset;
+ u32 src_len = skb->len - src_off;
+ bool keep = false;
+
+ if (!virtio_transport_can_collapse(skb, collapse_max)) {
Minor nit, possibly something alike the following lead to more
compact/more readable code:
keep = !virtio_transport_can_collapse(skb, collapse_max);
if (keep) {
Yeah, so I can remove the initialization to false. I'll change it.
+ /* Finalize pending collapsed skb to preserve packet
+ * ordering.
+ */
+ if (new_skb) {
+ __skb_queue_tail(&new_queue, new_skb);
+ new_skb = NULL;
+ }
+ keep = true;
+ goto next;
+ }
+
+ /* Finalize if this packet won't fit in the remaining tailroom,
+ * so we can allocate a right-sized new_skb.
+ */
+ if (new_skb && src_len > skb_tailroom(new_skb)) {
+ __skb_queue_tail(&new_queue, new_skb);
+ new_skb = NULL;
Possibly introduce an helper for the above 2 statements?
Do you mean something like this?
static void virtio_transport_queue_skb(struct sk_buff_head *queue,
struct sk_buff **skb)
{
__skb_queue_tail(queue, *skb);
*skb = NULL;
}
Not sure, just for 2 places, but if you prefer it, I can change.
Thanks,
Stefano
