On Wed, Jul 08, 2026 at 12:29:03PM +0200, 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 proactively from when the number of skb queued > is close to exceeding the overhead budget. > > 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. > > Walking a large queue may take a significant amount of time and cache > misses, causing traffic burstiness. To limit this, the collapse stops > once enough room is freed for this packet and the next one, but may > opportunistically free more to fill each collapsed skb to capacity. > > [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]>
this is the right approach Acked-by: Michael S. Tsirkin <[email protected]> > --- > v2: > - defined MAX_COLLAPSE_LEN macro instead of using a variable [Paolo] > - added a threshold to avoid walking all the queue while collapsing > [Paolo] > - collapsed the queue before calling virtio_transport_inc_rx_pkt(). > While working on the threshold, I figured out that the check I was > introducing can also be used to proactively trigger the collapse, so I > moved the call to virtio_transport_collapse_rx_queue() before acquiring > the rx_lock to have also a better diff to simplify backports > - improved code readability (removed `out` label, `keep` initialization, > etc.) [Paolo + other small stuff] > - Brien kindly retested this version as well (thank you so much) > --- > net/vmw_vsock/virtio_transport_common.c | 165 +++++++++++++++++++++++- > 1 file changed, 164 insertions(+), 1 deletion(-) > > diff --git a/net/vmw_vsock/virtio_transport_common.c > b/net/vmw_vsock/virtio_transport_common.c > index 09475007165b..8becad81279c 100644 > --- a/net/vmw_vsock/virtio_transport_common.c > +++ b/net/vmw_vsock/virtio_transport_common.c > @@ -26,6 +26,13 @@ > /* Threshold for detecting small packets to copy */ > #define GOOD_COPY_LEN 128 > > +/* Max payload that can be collapsed into a single linear skb, using the same > + * allocation threshold as virtio_vsock_alloc_skb() to avoid adding pressure > + * on the page allocator. > + */ > +#define MAX_COLLAPSE_LEN \ > + SKB_MAX_ORDER(VIRTIO_VSOCK_SKB_HEADROOM, PAGE_ALLOC_COSTLY_ORDER) > + > static void virtio_transport_cancel_close_work(struct vsock_sock *vsk, > bool cancel_timeout); > static s64 virtio_transport_has_space(struct virtio_vsock_sock *vvs); > @@ -420,6 +427,145 @@ static int virtio_transport_send_pkt_info(struct > vsock_sock *vsk, > return ret; > } > > +static bool virtio_transport_can_collapse(struct sk_buff *skb) > +{ > + /* 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 >= MAX_COLLAPSE_LEN) > + 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 target = skb->len - VIRTIO_VSOCK_SKB_CB(skb)->offset; > + > + while ((skb = skb_peek_next(skb, queue)) && > + virtio_transport_can_collapse(skb)) { > + unsigned int len = skb->len - VIRTIO_VSOCK_SKB_CB(skb)->offset; > + > + if (len > MAX_COLLAPSE_LEN - target) > + return target; > + > + target += len; > + } > + > + return target; > +} > + > +/* Called under lock_sock to compact the receive queue by merging small skbs. > + * @min_to_free: minimum number of skbs to eliminate from the queue. May free > + * more to fill each collapsed skb to capacity. > + */ > +static void > +virtio_transport_collapse_rx_queue(struct virtio_vsock_sock *vvs, > + u32 min_to_free) > +{ > + struct sk_buff *skb, *next_skb, *new_skb = NULL; > + struct sk_buff_head new_queue; > + u32 saved = 0; > + > + __skb_queue_head_init(&new_queue); > + > + skb_queue_walk_safe(&vvs->rx_queue, skb, next_skb) { > + 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; > + > + keep = !virtio_transport_can_collapse(skb); > + if (keep) { > + /* Finalize pending collapsed skb to preserve packet > + * ordering. > + */ > + if (new_skb) { > + __skb_queue_tail(&new_queue, new_skb); > + new_skb = NULL; > + saved--; > + } > + 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; > + saved--; > + } > + > + if (!new_skb) { > + unsigned int alloc_size; > + > + /* Check after finalizing to opportunistically fill > + * each collapsed skb to capacity, merging more skbs > + * than strictly required. > + */ > + if (saved >= min_to_free) > + break; > + > + alloc_size = virtio_transport_collapse_size(skb, > &vvs->rx_queue); > + > + /* Only this skb's data is eligible, nothing to merge > + * with. Keep as-is. > + */ > + if (alloc_size <= src_len) { > + keep = true; > + goto next; > + } > + > + new_skb = virtio_vsock_alloc_linear_skb(alloc_size + > + VIRTIO_VSOCK_SKB_HEADROOM, GFP_KERNEL); > + if (!new_skb) > + break; > + > + memcpy(virtio_vsock_hdr(new_skb), hdr, > + sizeof(struct virtio_vsock_hdr)); > + virtio_vsock_hdr(new_skb)->len = 0; > + } > + > + /* Cannot fail since src_off/src_len are within bounds, but if > + * it does, discard new_skb to avoid queuing corrupted data. > + */ > + if (WARN_ON_ONCE(skb_copy_bits(skb, src_off, > + skb_put(new_skb, src_len), > + src_len))) { > + kfree_skb(new_skb); > + new_skb = NULL; > + break; > + } > + > + le32_add_cpu(&virtio_vsock_hdr(new_skb)->len, src_len); > + virtio_vsock_hdr(new_skb)->flags |= hdr->flags; > + > +next: > + __skb_unlink(skb, &vvs->rx_queue); > + if (keep) { > + __skb_queue_tail(&new_queue, skb); > + } else { > + consume_skb(skb); > + saved++; > + } > + } > + > + if (new_skb) > + __skb_queue_tail(&new_queue, new_skb); > + > + skb_queue_splice(&new_queue, &vvs->rx_queue); > +} > + > static bool virtio_transport_inc_rx_pkt(struct virtio_vsock_sock *vvs, > u32 len) > { > @@ -1354,12 +1500,29 @@ virtio_transport_recv_enqueue(struct vsock_sock *vsk, > { > struct virtio_vsock_sock *vvs = vsk->trans; > bool can_enqueue, free_pkt = false; > + u32 len, queue_max, queue_len; > struct virtio_vsock_hdr *hdr; > - u32 len; > > hdr = virtio_vsock_hdr(skb); > len = le32_to_cpu(hdr->len); > > + /* virtio_transport_inc_rx_pkt() rejects packets when the per-skb > + * overhead (skb_queue_len * SKB_TRUESIZE(0)) exceeds buf_alloc. > + * Proactively collapse the queue before that happens. > + * No rx_lock needed: lock_sock is held by caller, preventing > + * concurrent enqueue or dequeue. > + */ > + queue_max = vvs->buf_alloc / SKB_TRUESIZE(0); > + queue_len = skb_queue_len(&vvs->rx_queue); > + if (queue_len >= queue_max) { > + /* Walking a large queue may take a significant amount of time > + * and cache misses, causing traffic burstiness. Limit the > + * collapse to freeing room for this packet and the next one. > + * It may free more to fill each collapsed skb to capacity. > + */ > + virtio_transport_collapse_rx_queue(vvs, queue_len + 2 - > queue_max); > + } > + > spin_lock_bh(&vvs->rx_lock); > > can_enqueue = virtio_transport_inc_rx_pkt(vvs, len); > -- > 2.55.0
