Re: [PATCH net 1/2] vsock/virtio: collapse receive queue under memory pressure

[Stefano Garzarella <sgarzare@redhat.com>]

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 <sgarzare@redhat.com>
>>
>> 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: stable@vger.kernel.org
>> Reported-by: Brien Oberstein <brienpub@gmail.com>
>> Closes: https://lore.kernel.org/netdev/618701dd023e$063de350$12b9a9f0$@gmail.com/
>> Tested-by: Brien Oberstein <brienpub@gmail.com>
>> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>> ---
>>  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