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.
> +{
> + /* 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).
/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) {
> + /* 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?
/P
