netdev - Re: [PATCH v2 4/8] vsock/virtio: Resize receive buffers so that each SKB fits in a page

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aHQkhuHI0yRPTfvV@willie-the-truck>
Date: Sun, 13 Jul 2025 22:26:30 +0100
From: Will Deacon <will@...nel.org>
To: Stefano Garzarella <sgarzare@...hat.com>
Cc: David Laight <david.laight.linux@...il.com>,
	linux-kernel@...r.kernel.org, Keir Fraser <keirf@...gle.com>,
	Steven Moreland <smoreland@...gle.com>,
	Frederick Mayle <fmayle@...gle.com>,
	Stefan Hajnoczi <stefanha@...hat.com>,
	"Michael S. Tsirkin" <mst@...hat.com>,
	Jason Wang <jasowang@...hat.com>,
	Eugenio Pérez <eperezma@...hat.com>,
	netdev@...r.kernel.org, virtualization@...ts.linux.dev
Subject: Re: [PATCH v2 4/8] vsock/virtio: Resize receive buffers so that each
 SKB fits in a page

On Wed, Jul 02, 2025 at 03:16:19PM +0200, Stefano Garzarella wrote:
> On Tue, Jul 01, 2025 at 08:14:00PM +0100, David Laight wrote:
> > On Tue,  1 Jul 2025 17:45:03 +0100
> > Will Deacon <will@...nel.org> wrote:
> > 
> > > When allocating receive buffers for the vsock virtio RX virtqueue, an
> > > SKB is allocated with a 4140 data payload (the 44-byte packet header +
> > > VIRTIO_VSOCK_DEFAULT_RX_BUF_SIZE). Even when factoring in the SKB
> > > overhead, the resulting 8KiB allocation thanks to the rounding in
> > > kmalloc_reserve() is wasteful (~3700 unusable bytes) and results in a
> > > higher-order page allocation for the sake of a few hundred bytes of
> > > packet data.
> > > 
> > > Limit the vsock virtio RX buffers to a page per SKB, resulting in much
> > > better memory utilisation and removing the need to allocate higher-order
> > > pages entirely.
> > > 
> > > Signed-off-by: Will Deacon <will@...nel.org>
> > > ---
> > >  include/linux/virtio_vsock.h     | 1 -
> > >  net/vmw_vsock/virtio_transport.c | 7 ++++++-
> > >  2 files changed, 6 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/include/linux/virtio_vsock.h b/include/linux/virtio_vsock.h
> > > index eb6980aa19fd..1b5731186095 100644
> > > --- a/include/linux/virtio_vsock.h
> > > +++ b/include/linux/virtio_vsock.h
> > > @@ -109,7 +109,6 @@ static inline size_t virtio_vsock_skb_len(struct sk_buff *skb)
> > >  	return (size_t)(skb_end_pointer(skb) - skb->head);
> > >  }
> > > 
> > > -#define VIRTIO_VSOCK_DEFAULT_RX_BUF_SIZE	(1024 * 4)
> > >  #define VIRTIO_VSOCK_MAX_BUF_SIZE		0xFFFFFFFFUL
> > >  #define VIRTIO_VSOCK_MAX_PKT_BUF_SIZE		(1024 * 64)
> > > 
> > > diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c
> > > index 488e6ddc6ffa..3daba06ed499 100644
> > > --- a/net/vmw_vsock/virtio_transport.c
> > > +++ b/net/vmw_vsock/virtio_transport.c
> > > @@ -307,7 +307,12 @@ virtio_transport_cancel_pkt(struct vsock_sock *vsk)
> > > 
> > >  static void virtio_vsock_rx_fill(struct virtio_vsock *vsock)
> > >  {
> > > -	int total_len = VIRTIO_VSOCK_DEFAULT_RX_BUF_SIZE + VIRTIO_VSOCK_SKB_HEADROOM;
> > > +	/* Dimension the SKB so that the entire thing fits exactly into
> > > +	 * a single page. This avoids wasting memory due to alloc_skb()
> > > +	 * rounding up to the next page order and also means that we
> > > +	 * don't leave higher-order pages sitting around in the RX queue.
> > > +	 */
> > > +	int total_len = SKB_WITH_OVERHEAD(PAGE_SIZE);
> > 
> > Should that be an explicit 4096?
> > Otherwise it is very wasteful of memory on systems with large pages.
> 
> This is a good point!
> 
> What about SKB_WITH_OVERHEAD(VIRTIO_VSOCK_DEFAULT_RX_BUF_SIZE) ?

Personally, I'd prefer to use PAGE_SIZE here as I think it's reasonable
that using a larger page size ends up with a proportionately higher
memory utilisation but potentially better performance. At least,
configuring the kernel with a larger page size and expecting no impact
on memory consumption is misguided. The 4KiB constant seems fairly
arbitrary (and has been there since day 1), so I can only assume that it
is acting as a proxy for PAGE_SIZE on architectures where that is fixed
to 4KiB.

However, I concede that the intention of this patch is to avoid the
allocation inefficiency described in the commit message, so I'll revert
to 4KiB as you suggest above but moving the SKB overhead calculation
into the definition:

#define VIRTIO_VSOCK_DEFAULT_RX_BUF_SIZE        SKB_WITH_OVERHEAD(1024 * 4)

That way, I can move the comment there as well and it should be clear
why we've ended up with what we have.

Will