[PATCH v2 0/8] vsock/virtio: SKB allocation improvements

[PATCH v2 0/8] vsock/virtio: SKB allocation improvements

[Will Deacon]

Hello again,

Here is version two of the patches I previously posted here:

  https://lore.kernel.org/r/20250625131543.5155-1-will@kernel.org

Changes since v1 include:

  * Remove virtio_vsock_alloc_skb_with_frags() and instead push decision
    to allocate nonlinear SKBs into virtio_vsock_alloc_skb()

  * Remove VIRTIO_VSOCK_DEFAULT_RX_BUF_SIZE and inline its definition
    along with a comment

  * Validate the length advertised by the packet header on the guest
    receive path

  * Minor tweaks to the commit messages and addition of stable tags

Thanks to Stefano for all the review feedback so far.

Cheers,

Will

Cc: Keir Fraser <keirf@google.com>
Cc: Steven Moreland <smoreland@google.com>
Cc: Frederick Mayle <fmayle@google.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
Cc: Stefano Garzarella <sgarzare@redhat.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
Cc: "Eugenio Pérez" <eperezma@redhat.com>
Cc: linux-kernel@vger.kernel.org 
Cc: netdev@vger.kernel.org 
Cc: virtualization@lists.linux.dev

--->8

Will Deacon (8):
  vhost/vsock: Avoid allocating arbitrarily-sized SKBs
  vsock/virtio: Validate length in packet header before skb_put()
  vsock/virtio: Move length check to callers of
    virtio_vsock_skb_rx_put()
  vsock/virtio: Resize receive buffers so that each SKB fits in a page
  vsock/virtio: Add vsock helper for linear SKB allocation
  vhost/vsock: Allocate nonlinear SKBs for handling large receive
    buffers
  vsock/virtio: Rename virtio_vsock_skb_rx_put() to
    virtio_vsock_skb_put()
  vsock/virtio: Allocate nonlinear SKBs for handling large transmit
    buffers

 drivers/vhost/vsock.c                   | 15 +++++-----
 include/linux/virtio_vsock.h            | 37 +++++++++++++++++++------
 net/vmw_vsock/virtio_transport.c        | 25 +++++++++++++----
 net/vmw_vsock/virtio_transport_common.c |  3 +-
 4 files changed, 59 insertions(+), 21 deletions(-)

-- 
2.50.0.727.gbf7dc18ff4-goog

[PATCH v2 1/8] vhost/vsock: Avoid allocating arbitrarily-sized SKBs

[Will Deacon]

vhost_vsock_alloc_skb() returns NULL for packets advertising a length
larger than VIRTIO_VSOCK_MAX_PKT_BUF_SIZE in the packet header. However,
this is only checked once the SKB has been allocated and, if the length
in the packet header is zero, the SKB may not be freed immediately.

Hoist the size check before the SKB allocation so that an iovec larger
than VIRTIO_VSOCK_MAX_PKT_BUF_SIZE + the header size is rejected
outright. The subsequent check on the length field in the header can
then simply check that the allocated SKB is indeed large enough to hold
the packet.

Cc: <stable@vger.kernel.org>
Fixes: 71dc9ec9ac7d ("virtio/vsock: replace virtio_vsock_pkt with sk_buff")
Signed-off-by: Will Deacon <will@kernel.org>
---
 drivers/vhost/vsock.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
index 802153e23073..66a0f060770e 100644
--- a/drivers/vhost/vsock.c
+++ b/drivers/vhost/vsock.c
@@ -344,6 +344,9 @@ vhost_vsock_alloc_skb(struct vhost_virtqueue *vq,
 
 	len = iov_length(vq->iov, out);
 
+	if (len > VIRTIO_VSOCK_MAX_PKT_BUF_SIZE + VIRTIO_VSOCK_SKB_HEADROOM)
+		return NULL;
+
 	/* len contains both payload and hdr */
 	skb = virtio_vsock_alloc_skb(len, GFP_KERNEL);
 	if (!skb)
@@ -367,8 +370,7 @@ vhost_vsock_alloc_skb(struct vhost_virtqueue *vq,
 		return skb;
 
 	/* The pkt is too big or the length in the header is invalid */
-	if (payload_len > VIRTIO_VSOCK_MAX_PKT_BUF_SIZE ||
-	    payload_len + sizeof(*hdr) > len) {
+	if (payload_len + sizeof(*hdr) > len) {
 		kfree_skb(skb);
 		return NULL;
 	}
-- 
2.50.0.727.gbf7dc18ff4-goog

[PATCH v2 2/8] vsock/virtio: Validate length in packet header before skb_put()

[Will Deacon]

When receiving a vsock packet in the guest, only the virtqueue buffer
size is validated prior to virtio_vsock_skb_rx_put(). Unfortunately,
virtio_vsock_skb_rx_put() uses the length from the packet header as the
length argument to skb_put(), potentially resulting in SKB overflow if
the host has gone wonky.

Validate the length as advertised by the packet header before calling
virtio_vsock_skb_rx_put().

Cc: <stable@vger.kernel.org>
Fixes: 71dc9ec9ac7d ("virtio/vsock: replace virtio_vsock_pkt with sk_buff")
Signed-off-by: Will Deacon <will@kernel.org>
---
 net/vmw_vsock/virtio_transport.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c
index f0e48e6911fc..bd2c6aaa1a93 100644
--- a/net/vmw_vsock/virtio_transport.c
+++ b/net/vmw_vsock/virtio_transport.c
@@ -624,8 +624,9 @@ static void virtio_transport_rx_work(struct work_struct *work)
 	do {
 		virtqueue_disable_cb(vq);
 		for (;;) {
+			unsigned int len, payload_len;
+			struct virtio_vsock_hdr *hdr;
 			struct sk_buff *skb;
-			unsigned int len;
 
 			if (!virtio_transport_more_replies(vsock)) {
 				/* Stop rx until the device processes already
@@ -642,12 +643,19 @@ static void virtio_transport_rx_work(struct work_struct *work)
 			vsock->rx_buf_nr--;
 
 			/* Drop short/long packets */
-			if (unlikely(len < sizeof(struct virtio_vsock_hdr) ||
+			if (unlikely(len < sizeof(*hdr) ||
 				     len > virtio_vsock_skb_len(skb))) {
 				kfree_skb(skb);
 				continue;
 			}
 
+			hdr = virtio_vsock_hdr(skb);
+			payload_len = le32_to_cpu(hdr->len);
+			if (payload_len > len - sizeof(*hdr)) {
+				kfree_skb(skb);
+				continue;
+			}
+
 			virtio_vsock_skb_rx_put(skb);
 			virtio_transport_deliver_tap_pkt(skb);
 			virtio_transport_recv_pkt(&virtio_transport, skb);
-- 
2.50.0.727.gbf7dc18ff4-goog

[PATCH v2 3/8] vsock/virtio: Move length check to callers of virtio_vsock_skb_rx_put()

[Will Deacon]

virtio_vsock_skb_rx_put() only calls skb_put() if the length in the
packet header is not zero even though skb_put() handles this case
gracefully.

Remove the functionally redundant check from virtio_vsock_skb_rx_put()
and, on the assumption that this is a worthwhile optimisation for
handling credit messages, augment the existing length checks in
virtio_transport_rx_work() to elide the call for zero-length payloads.
Note that the vhost code already has similar logic in
vhost_vsock_alloc_skb().

Signed-off-by: Will Deacon <will@kernel.org>
---
 include/linux/virtio_vsock.h     | 4 +---
 net/vmw_vsock/virtio_transport.c | 4 +++-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/linux/virtio_vsock.h b/include/linux/virtio_vsock.h
index 36fb3edfa403..eb6980aa19fd 100644
--- a/include/linux/virtio_vsock.h
+++ b/include/linux/virtio_vsock.h
@@ -52,9 +52,7 @@ static inline void virtio_vsock_skb_rx_put(struct sk_buff *skb)
 	u32 len;
 
 	len = le32_to_cpu(virtio_vsock_hdr(skb)->len);
-
-	if (len > 0)
-		skb_put(skb, len);
+	skb_put(skb, len);
 }
 
 static inline struct sk_buff *virtio_vsock_alloc_skb(unsigned int size, gfp_t mask)
diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c
index bd2c6aaa1a93..488e6ddc6ffa 100644
--- a/net/vmw_vsock/virtio_transport.c
+++ b/net/vmw_vsock/virtio_transport.c
@@ -656,7 +656,9 @@ static void virtio_transport_rx_work(struct work_struct *work)
 				continue;
 			}
 
-			virtio_vsock_skb_rx_put(skb);
+			if (payload_len)
+				virtio_vsock_skb_rx_put(skb);
+
 			virtio_transport_deliver_tap_pkt(skb);
 			virtio_transport_recv_pkt(&virtio_transport, skb);
 		}
-- 
2.50.0.727.gbf7dc18ff4-goog

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

[Will Deacon]

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@kernel.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);
 	struct scatterlist pkt, *p;
 	struct virtqueue *vq;
 	struct sk_buff *skb;
-- 
2.50.0.727.gbf7dc18ff4-goog

[PATCH v2 5/8] vsock/virtio: Add vsock helper for linear SKB allocation

[Will Deacon]

In preparation for nonlinear allocations for large SKBs, introduce a
new virtio_vsock_alloc_linear_skb() helper to return linear SKBs
unconditionally and switch all callers over to this new interface for
now.

No functional change.

Signed-off-by: Will Deacon <will@kernel.org>
---
 drivers/vhost/vsock.c                   | 2 +-
 include/linux/virtio_vsock.h            | 6 ++++++
 net/vmw_vsock/virtio_transport.c        | 2 +-
 net/vmw_vsock/virtio_transport_common.c | 2 +-
 4 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
index 66a0f060770e..b13f6be452ba 100644
--- a/drivers/vhost/vsock.c
+++ b/drivers/vhost/vsock.c
@@ -348,7 +348,7 @@ vhost_vsock_alloc_skb(struct vhost_virtqueue *vq,
 		return NULL;
 
 	/* len contains both payload and hdr */
-	skb = virtio_vsock_alloc_skb(len, GFP_KERNEL);
+	skb = virtio_vsock_alloc_linear_skb(len, GFP_KERNEL);
 	if (!skb)
 		return NULL;
 
diff --git a/include/linux/virtio_vsock.h b/include/linux/virtio_vsock.h
index 1b5731186095..6d4a933c895a 100644
--- a/include/linux/virtio_vsock.h
+++ b/include/linux/virtio_vsock.h
@@ -70,6 +70,12 @@ static inline struct sk_buff *virtio_vsock_alloc_skb(unsigned int size, gfp_t ma
 	return skb;
 }
 
+static inline struct sk_buff *
+virtio_vsock_alloc_linear_skb(unsigned int size, gfp_t mask)
+{
+	return virtio_vsock_alloc_skb(size, mask);
+}
+
 static inline void
 virtio_vsock_skb_queue_head(struct sk_buff_head *list, struct sk_buff *skb)
 {
diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c
index 3daba06ed499..2959db0404ed 100644
--- a/net/vmw_vsock/virtio_transport.c
+++ b/net/vmw_vsock/virtio_transport.c
@@ -321,7 +321,7 @@ static void virtio_vsock_rx_fill(struct virtio_vsock *vsock)
 	vq = vsock->vqs[VSOCK_VQ_RX];
 
 	do {
-		skb = virtio_vsock_alloc_skb(total_len, GFP_KERNEL);
+		skb = virtio_vsock_alloc_linear_skb(total_len, GFP_KERNEL);
 		if (!skb)
 			break;
 
diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
index 1b5d9896edae..c9eb7f7ac00d 100644
--- a/net/vmw_vsock/virtio_transport_common.c
+++ b/net/vmw_vsock/virtio_transport_common.c
@@ -261,7 +261,7 @@ static struct sk_buff *virtio_transport_alloc_skb(struct virtio_vsock_pkt_info *
 	if (!zcopy)
 		skb_len += payload_len;
 
-	skb = virtio_vsock_alloc_skb(skb_len, GFP_KERNEL);
+	skb = virtio_vsock_alloc_linear_skb(skb_len, GFP_KERNEL);
 	if (!skb)
 		return NULL;
 
-- 
2.50.0.727.gbf7dc18ff4-goog

[PATCH v2 6/8] vhost/vsock: Allocate nonlinear SKBs for handling large receive buffers

[Will Deacon]

When receiving a packet from a guest, vhost_vsock_handle_tx_kick()
calls vhost_vsock_alloc_linear_skb() to allocate and fill an SKB with
the receive data. Unfortunately, these are always linear allocations and
can therefore result in significant pressure on kmalloc() considering
that the maximum packet size (VIRTIO_VSOCK_MAX_PKT_BUF_SIZE +
VIRTIO_VSOCK_SKB_HEADROOM) is a little over 64KiB, resulting in a 128KiB
allocation for each packet.

Rework the vsock SKB allocation so that, for sizes with page order
greater than PAGE_ALLOC_COSTLY_ORDER, a nonlinear SKB is allocated
instead with the packet header in the SKB and the receive data in the
fragments. Move the VIRTIO_VSOCK_SKB_HEADROOM check out of the
allocation function and into the single caller that needs it and add a
debug warning if virtio_vsock_skb_rx_put() is ever called on an SKB with
a non-zero length, as this would be destructive for the nonlinear case.

Signed-off-by: Will Deacon <will@kernel.org>
---
 drivers/vhost/vsock.c        | 11 +++++------
 include/linux/virtio_vsock.h | 32 +++++++++++++++++++++++++-------
 2 files changed, 30 insertions(+), 13 deletions(-)

diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
index b13f6be452ba..f3c2ea1d0ae7 100644
--- a/drivers/vhost/vsock.c
+++ b/drivers/vhost/vsock.c
@@ -344,11 +344,12 @@ vhost_vsock_alloc_skb(struct vhost_virtqueue *vq,
 
 	len = iov_length(vq->iov, out);
 
-	if (len > VIRTIO_VSOCK_MAX_PKT_BUF_SIZE + VIRTIO_VSOCK_SKB_HEADROOM)
+	if (len < VIRTIO_VSOCK_SKB_HEADROOM ||
+	    len > VIRTIO_VSOCK_MAX_PKT_BUF_SIZE + VIRTIO_VSOCK_SKB_HEADROOM)
 		return NULL;
 
 	/* len contains both payload and hdr */
-	skb = virtio_vsock_alloc_linear_skb(len, GFP_KERNEL);
+	skb = virtio_vsock_alloc_skb(len, GFP_KERNEL);
 	if (!skb)
 		return NULL;
 
@@ -377,10 +378,8 @@ vhost_vsock_alloc_skb(struct vhost_virtqueue *vq,
 
 	virtio_vsock_skb_rx_put(skb);
 
-	nbytes = copy_from_iter(skb->data, payload_len, &iov_iter);
-	if (nbytes != payload_len) {
-		vq_err(vq, "Expected %zu byte payload, got %zu bytes\n",
-		       payload_len, nbytes);
+	if (skb_copy_datagram_from_iter(skb, 0, &iov_iter, payload_len)) {
+		vq_err(vq, "Failed to copy %zu byte payload\n", payload_len);
 		kfree_skb(skb);
 		return NULL;
 	}
diff --git a/include/linux/virtio_vsock.h b/include/linux/virtio_vsock.h
index 6d4a933c895a..ad69668f6b91 100644
--- a/include/linux/virtio_vsock.h
+++ b/include/linux/virtio_vsock.h
@@ -51,29 +51,47 @@ static inline void virtio_vsock_skb_rx_put(struct sk_buff *skb)
 {
 	u32 len;
 
+	DEBUG_NET_WARN_ON_ONCE(skb->len);
 	len = le32_to_cpu(virtio_vsock_hdr(skb)->len);
-	skb_put(skb, len);
+
+	if (skb_is_nonlinear(skb))
+		skb->len = len;
+	else
+		skb_put(skb, len);
 }
 
-static inline struct sk_buff *virtio_vsock_alloc_skb(unsigned int size, gfp_t mask)
+static inline struct sk_buff *
+__virtio_vsock_alloc_skb_with_frags(unsigned int header_len,
+				    unsigned int data_len,
+				    gfp_t mask)
 {
 	struct sk_buff *skb;
+	int err;
 
-	if (size < VIRTIO_VSOCK_SKB_HEADROOM)
-		return NULL;
-
-	skb = alloc_skb(size, mask);
+	skb = alloc_skb_with_frags(header_len, data_len,
+				   PAGE_ALLOC_COSTLY_ORDER, &err, mask);
 	if (!skb)
 		return NULL;
 
 	skb_reserve(skb, VIRTIO_VSOCK_SKB_HEADROOM);
+	skb->data_len = data_len;
 	return skb;
 }
 
 static inline struct sk_buff *
 virtio_vsock_alloc_linear_skb(unsigned int size, gfp_t mask)
 {
-	return virtio_vsock_alloc_skb(size, mask);
+	return __virtio_vsock_alloc_skb_with_frags(size, 0, mask);
+}
+
+static inline struct sk_buff *virtio_vsock_alloc_skb(unsigned int size, gfp_t mask)
+{
+	if (size <= SKB_WITH_OVERHEAD(PAGE_SIZE << PAGE_ALLOC_COSTLY_ORDER))
+		return virtio_vsock_alloc_linear_skb(size, mask);
+
+	size -= VIRTIO_VSOCK_SKB_HEADROOM;
+	return __virtio_vsock_alloc_skb_with_frags(VIRTIO_VSOCK_SKB_HEADROOM,
+						   size, mask);
 }
 
 static inline void
-- 
2.50.0.727.gbf7dc18ff4-goog

[PATCH v2 7/8] vsock/virtio: Rename virtio_vsock_skb_rx_put() to virtio_vsock_skb_put()

[Will Deacon]

In preparation for using virtio_vsock_skb_rx_put() when populating SKBs
on the vsock TX path, rename virtio_vsock_skb_rx_put() to
virtio_vsock_skb_put().

No functional change.

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
Signed-off-by: Will Deacon <will@kernel.org>
---
 drivers/vhost/vsock.c            | 2 +-
 include/linux/virtio_vsock.h     | 2 +-
 net/vmw_vsock/virtio_transport.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
index f3c2ea1d0ae7..a6cd72a32f63 100644
--- a/drivers/vhost/vsock.c
+++ b/drivers/vhost/vsock.c
@@ -376,7 +376,7 @@ vhost_vsock_alloc_skb(struct vhost_virtqueue *vq,
 		return NULL;
 	}
 
-	virtio_vsock_skb_rx_put(skb);
+	virtio_vsock_skb_put(skb);
 
 	if (skb_copy_datagram_from_iter(skb, 0, &iov_iter, payload_len)) {
 		vq_err(vq, "Failed to copy %zu byte payload\n", payload_len);
diff --git a/include/linux/virtio_vsock.h b/include/linux/virtio_vsock.h
index ad69668f6b91..ed5eab46e3dc 100644
--- a/include/linux/virtio_vsock.h
+++ b/include/linux/virtio_vsock.h
@@ -47,7 +47,7 @@ static inline void virtio_vsock_skb_clear_tap_delivered(struct sk_buff *skb)
 	VIRTIO_VSOCK_SKB_CB(skb)->tap_delivered = false;
 }
 
-static inline void virtio_vsock_skb_rx_put(struct sk_buff *skb)
+static inline void virtio_vsock_skb_put(struct sk_buff *skb)
 {
 	u32 len;
 
diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c
index 2959db0404ed..44751cf8dfca 100644
--- a/net/vmw_vsock/virtio_transport.c
+++ b/net/vmw_vsock/virtio_transport.c
@@ -662,7 +662,7 @@ static void virtio_transport_rx_work(struct work_struct *work)
 			}
 
 			if (payload_len)
-				virtio_vsock_skb_rx_put(skb);
+				virtio_vsock_skb_put(skb);
 
 			virtio_transport_deliver_tap_pkt(skb);
 			virtio_transport_recv_pkt(&virtio_transport, skb);
-- 
2.50.0.727.gbf7dc18ff4-goog

[PATCH v2 8/8] vsock/virtio: Allocate nonlinear SKBs for handling large transmit buffers

[Will Deacon]

When transmitting a vsock packet, virtio_transport_send_pkt_info() calls
virtio_transport_alloc_linear_skb() to allocate and fill SKBs with the
transmit data. Unfortunately, these are always linear allocations and
can therefore result in significant pressure on kmalloc() considering
that the maximum packet size (VIRTIO_VSOCK_MAX_PKT_BUF_SIZE +
VIRTIO_VSOCK_SKB_HEADROOM) is a little over 64KiB, resulting in a 128KiB
allocation for each packet.

Rework the vsock SKB allocation so that, for sizes with page order
greater than PAGE_ALLOC_COSTLY_ORDER, a nonlinear SKB is allocated
instead with the packet header in the SKB and the transmit data in the
fragments. No that this affects both the vhost and virtio transports.

Signed-off-by: Will Deacon <will@kernel.org>
---
 net/vmw_vsock/virtio_transport_common.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
index c9eb7f7ac00d..f74677c3511e 100644
--- a/net/vmw_vsock/virtio_transport_common.c
+++ b/net/vmw_vsock/virtio_transport_common.c
@@ -109,7 +109,8 @@ static int virtio_transport_fill_skb(struct sk_buff *skb,
 		return __zerocopy_sg_from_iter(info->msg, NULL, skb,
 					       &info->msg->msg_iter, len, NULL);
 
-	return memcpy_from_msg(skb_put(skb, len), info->msg, len);
+	virtio_vsock_skb_put(skb);
+	return skb_copy_datagram_from_iter(skb, 0, &info->msg->msg_iter, len);
 }
 
 static void virtio_transport_init_hdr(struct sk_buff *skb,
@@ -261,7 +262,7 @@ static struct sk_buff *virtio_transport_alloc_skb(struct virtio_vsock_pkt_info *
 	if (!zcopy)
 		skb_len += payload_len;
 
-	skb = virtio_vsock_alloc_linear_skb(skb_len, GFP_KERNEL);
+	skb = virtio_vsock_alloc_skb(skb_len, GFP_KERNEL);
 	if (!skb)
 		return NULL;
 
-- 
2.50.0.727.gbf7dc18ff4-goog

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

[David Laight]

On Tue,  1 Jul 2025 17:45:03 +0100
Will Deacon <will@kernel.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@kernel.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.

	David

>  	struct scatterlist pkt, *p;
>  	struct virtqueue *vq;
>  	struct sk_buff *skb;

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

[Stefano Garzarella]

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@kernel.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@kernel.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) ?

Thanks,
Stefano

>
>	David
>
>>  	struct scatterlist pkt, *p;
>>  	struct virtqueue *vq;
>>  	struct sk_buff *skb;
>
>

Re: [PATCH v2 3/8] vsock/virtio: Move length check to callers of virtio_vsock_skb_rx_put()

[Stefano Garzarella]

On Tue, Jul 01, 2025 at 05:45:02PM +0100, Will Deacon wrote:
>virtio_vsock_skb_rx_put() only calls skb_put() if the length in the
>packet header is not zero even though skb_put() handles this case
>gracefully.
>
>Remove the functionally redundant check from virtio_vsock_skb_rx_put()
>and, on the assumption that this is a worthwhile optimisation for
>handling credit messages, augment the existing length checks in
>virtio_transport_rx_work() to elide the call for zero-length payloads.
>Note that the vhost code already has similar logic in
>vhost_vsock_alloc_skb().
>
>Signed-off-by: Will Deacon <will@kernel.org>
>---
> include/linux/virtio_vsock.h     | 4 +---
> net/vmw_vsock/virtio_transport.c | 4 +++-
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
>diff --git a/include/linux/virtio_vsock.h b/include/linux/virtio_vsock.h
>index 36fb3edfa403..eb6980aa19fd 100644
>--- a/include/linux/virtio_vsock.h
>+++ b/include/linux/virtio_vsock.h
>@@ -52,9 +52,7 @@ static inline void virtio_vsock_skb_rx_put(struct sk_buff *skb)
> 	u32 len;
>
> 	len = le32_to_cpu(virtio_vsock_hdr(skb)->len);
>-
>-	if (len > 0)
>-		skb_put(skb, len);
>+	skb_put(skb, len);

Since the caller is supposed to check the len, can we just pass it as 
parameter?

So we can avoid the `le32_to_cpu(virtio_vsock_hdr(skb)->len)` here.

Thanks,
Stefano

> }
>
> static inline struct sk_buff *virtio_vsock_alloc_skb(unsigned int size, gfp_t mask)
>diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c
>index bd2c6aaa1a93..488e6ddc6ffa 100644
>--- a/net/vmw_vsock/virtio_transport.c
>+++ b/net/vmw_vsock/virtio_transport.c
>@@ -656,7 +656,9 @@ static void virtio_transport_rx_work(struct work_struct *work)
> 				continue;
> 			}
>
>-			virtio_vsock_skb_rx_put(skb);
>+			if (payload_len)
>+				virtio_vsock_skb_rx_put(skb);
>+
> 			virtio_transport_deliver_tap_pkt(skb);
> 			virtio_transport_recv_pkt(&virtio_transport, skb);
> 		}
>-- 
>2.50.0.727.gbf7dc18ff4-goog
>

Re: [PATCH v2 5/8] vsock/virtio: Add vsock helper for linear SKB allocation

[Stefano Garzarella]

On Tue, Jul 01, 2025 at 05:45:04PM +0100, Will Deacon wrote:
>In preparation for nonlinear allocations for large SKBs, introduce a
>new virtio_vsock_alloc_linear_skb() helper to return linear SKBs
>unconditionally and switch all callers over to this new interface for
>now.
>
>No functional change.
>
>Signed-off-by: Will Deacon <will@kernel.org>
>---
> drivers/vhost/vsock.c                   | 2 +-
> include/linux/virtio_vsock.h            | 6 ++++++
> net/vmw_vsock/virtio_transport.c        | 2 +-
> net/vmw_vsock/virtio_transport_common.c | 2 +-
> 4 files changed, 9 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
>index 66a0f060770e..b13f6be452ba 100644
>--- a/drivers/vhost/vsock.c
>+++ b/drivers/vhost/vsock.c
>@@ -348,7 +348,7 @@ vhost_vsock_alloc_skb(struct vhost_virtqueue *vq,
> 		return NULL;
>
> 	/* len contains both payload and hdr */
>-	skb = virtio_vsock_alloc_skb(len, GFP_KERNEL);
>+	skb = virtio_vsock_alloc_linear_skb(len, GFP_KERNEL);
> 	if (!skb)
> 		return NULL;
>
>diff --git a/include/linux/virtio_vsock.h b/include/linux/virtio_vsock.h
>index 1b5731186095..6d4a933c895a 100644
>--- a/include/linux/virtio_vsock.h
>+++ b/include/linux/virtio_vsock.h
>@@ -70,6 +70,12 @@ static inline struct sk_buff *virtio_vsock_alloc_skb(unsigned int size, gfp_t ma
> 	return skb;
> }
>
>+static inline struct sk_buff *
>+virtio_vsock_alloc_linear_skb(unsigned int size, gfp_t mask)
>+{
>+	return virtio_vsock_alloc_skb(size, mask);

Why not just renaming virtio_vsock_alloc_skb in this patch?

In that way we are sure when building this patch we don't leave any 
"old" virtio_vsock_alloc_skb() around.

Thanks,
Stefano

>+}
>+
> static inline void
> virtio_vsock_skb_queue_head(struct sk_buff_head *list, struct sk_buff *skb)
> {
>diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c
>index 3daba06ed499..2959db0404ed 100644
>--- a/net/vmw_vsock/virtio_transport.c
>+++ b/net/vmw_vsock/virtio_transport.c
>@@ -321,7 +321,7 @@ static void virtio_vsock_rx_fill(struct virtio_vsock *vsock)
> 	vq = vsock->vqs[VSOCK_VQ_RX];
>
> 	do {
>-		skb = virtio_vsock_alloc_skb(total_len, GFP_KERNEL);
>+		skb = virtio_vsock_alloc_linear_skb(total_len, GFP_KERNEL);
> 		if (!skb)
> 			break;
>
>diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>index 1b5d9896edae..c9eb7f7ac00d 100644
>--- a/net/vmw_vsock/virtio_transport_common.c
>+++ b/net/vmw_vsock/virtio_transport_common.c
>@@ -261,7 +261,7 @@ static struct sk_buff *virtio_transport_alloc_skb(struct virtio_vsock_pkt_info *
> 	if (!zcopy)
> 		skb_len += payload_len;
>
>-	skb = virtio_vsock_alloc_skb(skb_len, GFP_KERNEL);
>+	skb = virtio_vsock_alloc_linear_skb(skb_len, GFP_KERNEL);
> 	if (!skb)
> 		return NULL;
>
>-- 
>2.50.0.727.gbf7dc18ff4-goog
>

Re: [PATCH v2 6/8] vhost/vsock: Allocate nonlinear SKBs for handling large receive buffers

[Stefano Garzarella]

On Tue, Jul 01, 2025 at 05:45:05PM +0100, Will Deacon wrote:
>When receiving a packet from a guest, vhost_vsock_handle_tx_kick()
>calls vhost_vsock_alloc_linear_skb() to allocate and fill an SKB with
>the receive data. Unfortunately, these are always linear allocations and
>can therefore result in significant pressure on kmalloc() considering
>that the maximum packet size (VIRTIO_VSOCK_MAX_PKT_BUF_SIZE +
>VIRTIO_VSOCK_SKB_HEADROOM) is a little over 64KiB, resulting in a 128KiB
>allocation for each packet.
>
>Rework the vsock SKB allocation so that, for sizes with page order
>greater than PAGE_ALLOC_COSTLY_ORDER, a nonlinear SKB is allocated
>instead with the packet header in the SKB and the receive data in the
>fragments. Move the VIRTIO_VSOCK_SKB_HEADROOM check out of the
>allocation function and into the single caller that needs it and add a
>debug warning if virtio_vsock_skb_rx_put() is ever called on an SKB with
>a non-zero length, as this would be destructive for the nonlinear case.
>
>Signed-off-by: Will Deacon <will@kernel.org>
>---
> drivers/vhost/vsock.c        | 11 +++++------
> include/linux/virtio_vsock.h | 32 +++++++++++++++++++++++++-------
> 2 files changed, 30 insertions(+), 13 deletions(-)
>
>diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
>index b13f6be452ba..f3c2ea1d0ae7 100644
>--- a/drivers/vhost/vsock.c
>+++ b/drivers/vhost/vsock.c
>@@ -344,11 +344,12 @@ vhost_vsock_alloc_skb(struct vhost_virtqueue *vq,
>
> 	len = iov_length(vq->iov, out);
>
>-	if (len > VIRTIO_VSOCK_MAX_PKT_BUF_SIZE + VIRTIO_VSOCK_SKB_HEADROOM)
>+	if (len < VIRTIO_VSOCK_SKB_HEADROOM ||
>+	    len > VIRTIO_VSOCK_MAX_PKT_BUF_SIZE + VIRTIO_VSOCK_SKB_HEADROOM)
> 		return NULL;
>
> 	/* len contains both payload and hdr */
>-	skb = virtio_vsock_alloc_linear_skb(len, GFP_KERNEL);
>+	skb = virtio_vsock_alloc_skb(len, GFP_KERNEL);
> 	if (!skb)
> 		return NULL;
>
>@@ -377,10 +378,8 @@ vhost_vsock_alloc_skb(struct vhost_virtqueue *vq,
>
> 	virtio_vsock_skb_rx_put(skb);
>
>-	nbytes = copy_from_iter(skb->data, payload_len, &iov_iter);
>-	if (nbytes != payload_len) {
>-		vq_err(vq, "Expected %zu byte payload, got %zu bytes\n",
>-		       payload_len, nbytes);
>+	if (skb_copy_datagram_from_iter(skb, 0, &iov_iter, payload_len)) {
>+		vq_err(vq, "Failed to copy %zu byte payload\n", payload_len);
> 		kfree_skb(skb);
> 		return NULL;
> 	}
>diff --git a/include/linux/virtio_vsock.h b/include/linux/virtio_vsock.h
>index 6d4a933c895a..ad69668f6b91 100644
>--- a/include/linux/virtio_vsock.h
>+++ b/include/linux/virtio_vsock.h
>@@ -51,29 +51,47 @@ static inline void virtio_vsock_skb_rx_put(struct sk_buff *skb)
> {
> 	u32 len;
>
>+	DEBUG_NET_WARN_ON_ONCE(skb->len);
> 	len = le32_to_cpu(virtio_vsock_hdr(skb)->len);
>-	skb_put(skb, len);
>+
>+	if (skb_is_nonlinear(skb))
>+		skb->len = len;
>+	else
>+		skb_put(skb, len);
> }

>
>-static inline struct sk_buff *virtio_vsock_alloc_skb(unsigned int size, gfp_t mask)
>+static inline struct sk_buff *
>+__virtio_vsock_alloc_skb_with_frags(unsigned int header_len,
>+				    unsigned int data_len,
>+				    gfp_t mask)
> {
> 	struct sk_buff *skb;
>+	int err;
>
>-	if (size < VIRTIO_VSOCK_SKB_HEADROOM)
>-		return NULL;

I would have made this change in a separate patch, but IIUC the only 
other caller is virtio_transport_alloc_skb() where this condition is 
implied, right?

I don't know, maybe we could have one patch where you touch this and 
virtio_vsock_skb_rx_put(), and another where you introduce nonlinear 
allocation for vhost/vsock.  What do you think? (not a strong opinion, 
just worried about doing 2 things in a single patch)

Thanks,
Stefano

>-
>-	skb = alloc_skb(size, mask);
>+	skb = alloc_skb_with_frags(header_len, data_len,
>+				   PAGE_ALLOC_COSTLY_ORDER, &err, mask);
> 	if (!skb)
> 		return NULL;
>
> 	skb_reserve(skb, VIRTIO_VSOCK_SKB_HEADROOM);
>+	skb->data_len = data_len;
> 	return skb;
> }
>
> static inline struct sk_buff *
> virtio_vsock_alloc_linear_skb(unsigned int size, gfp_t mask)
> {
>-	return virtio_vsock_alloc_skb(size, mask);
>+	return __virtio_vsock_alloc_skb_with_frags(size, 0, mask);
>+}
>+
>+static inline struct sk_buff *virtio_vsock_alloc_skb(unsigned int size, gfp_t mask)
>+{
>+	if (size <= SKB_WITH_OVERHEAD(PAGE_SIZE << PAGE_ALLOC_COSTLY_ORDER))
>+		return virtio_vsock_alloc_linear_skb(size, mask);
>+
>+	size -= VIRTIO_VSOCK_SKB_HEADROOM;
>+	return __virtio_vsock_alloc_skb_with_frags(VIRTIO_VSOCK_SKB_HEADROOM,
>+						   size, mask);
> }
>
> static inline void
>-- 
>2.50.0.727.gbf7dc18ff4-goog
>

Re: [PATCH v2 8/8] vsock/virtio: Allocate nonlinear SKBs for handling large transmit buffers

[Stefano Garzarella]

On Tue, Jul 01, 2025 at 05:45:07PM +0100, Will Deacon wrote:
>When transmitting a vsock packet, virtio_transport_send_pkt_info() calls
>virtio_transport_alloc_linear_skb() to allocate and fill SKBs with the
>transmit data. Unfortunately, these are always linear allocations and
>can therefore result in significant pressure on kmalloc() considering
>that the maximum packet size (VIRTIO_VSOCK_MAX_PKT_BUF_SIZE +
>VIRTIO_VSOCK_SKB_HEADROOM) is a little over 64KiB, resulting in a 128KiB
>allocation for each packet.
>
>Rework the vsock SKB allocation so that, for sizes with page order
>greater than PAGE_ALLOC_COSTLY_ORDER, a nonlinear SKB is allocated
>instead with the packet header in the SKB and the transmit data in the
>fragments. No that this affects both the vhost and virtio transports.
>
>Signed-off-by: Will Deacon <will@kernel.org>
>---
> net/vmw_vsock/virtio_transport_common.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

>
>diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>index c9eb7f7ac00d..f74677c3511e 100644
>--- a/net/vmw_vsock/virtio_transport_common.c
>+++ b/net/vmw_vsock/virtio_transport_common.c
>@@ -109,7 +109,8 @@ static int virtio_transport_fill_skb(struct sk_buff *skb,
> 		return __zerocopy_sg_from_iter(info->msg, NULL, skb,
> 					       &info->msg->msg_iter, len, NULL);
>
>-	return memcpy_from_msg(skb_put(skb, len), info->msg, len);
>+	virtio_vsock_skb_put(skb);
>+	return skb_copy_datagram_from_iter(skb, 0, &info->msg->msg_iter, len);
> }
>
> static void virtio_transport_init_hdr(struct sk_buff *skb,
>@@ -261,7 +262,7 @@ static struct sk_buff *virtio_transport_alloc_skb(struct virtio_vsock_pkt_info *
> 	if (!zcopy)
> 		skb_len += payload_len;
>
>-	skb = virtio_vsock_alloc_linear_skb(skb_len, GFP_KERNEL);
>+	skb = virtio_vsock_alloc_skb(skb_len, GFP_KERNEL);
> 	if (!skb)
> 		return NULL;
>
>-- 
>2.50.0.727.gbf7dc18ff4-goog
>

Re: [PATCH v2 0/8] vsock/virtio: SKB allocation improvements

[Lei Yang]

I tested this series of patches with virtio-net regression tests,
everything works fine.

Tested-by: Lei Yang <leiyang@redhat.com>

On Wed, Jul 2, 2025 at 12:48 AM Will Deacon <will@kernel.org> wrote:
>
> Hello again,
>
> Here is version two of the patches I previously posted here:
>
>   https://lore.kernel.org/r/20250625131543.5155-1-will@kernel.org
>
> Changes since v1 include:
>
>   * Remove virtio_vsock_alloc_skb_with_frags() and instead push decision
>     to allocate nonlinear SKBs into virtio_vsock_alloc_skb()
>
>   * Remove VIRTIO_VSOCK_DEFAULT_RX_BUF_SIZE and inline its definition
>     along with a comment
>
>   * Validate the length advertised by the packet header on the guest
>     receive path
>
>   * Minor tweaks to the commit messages and addition of stable tags
>
> Thanks to Stefano for all the review feedback so far.
>
> Cheers,
>
> Will
>
> Cc: Keir Fraser <keirf@google.com>
> Cc: Steven Moreland <smoreland@google.com>
> Cc: Frederick Mayle <fmayle@google.com>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Cc: Stefano Garzarella <sgarzare@redhat.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: "Eugenio Pérez" <eperezma@redhat.com>
> Cc: linux-kernel@vger.kernel.org
> Cc: netdev@vger.kernel.org
> Cc: virtualization@lists.linux.dev
>
> --->8
>
> Will Deacon (8):
>   vhost/vsock: Avoid allocating arbitrarily-sized SKBs
>   vsock/virtio: Validate length in packet header before skb_put()
>   vsock/virtio: Move length check to callers of
>     virtio_vsock_skb_rx_put()
>   vsock/virtio: Resize receive buffers so that each SKB fits in a page
>   vsock/virtio: Add vsock helper for linear SKB allocation
>   vhost/vsock: Allocate nonlinear SKBs for handling large receive
>     buffers
>   vsock/virtio: Rename virtio_vsock_skb_rx_put() to
>     virtio_vsock_skb_put()
>   vsock/virtio: Allocate nonlinear SKBs for handling large transmit
>     buffers
>
>  drivers/vhost/vsock.c                   | 15 +++++-----
>  include/linux/virtio_vsock.h            | 37 +++++++++++++++++++------
>  net/vmw_vsock/virtio_transport.c        | 25 +++++++++++++----
>  net/vmw_vsock/virtio_transport_common.c |  3 +-
>  4 files changed, 59 insertions(+), 21 deletions(-)
>
> --
> 2.50.0.727.gbf7dc18ff4-goog
>
>

Re: [PATCH v2 0/8] vsock/virtio: SKB allocation improvements

[Will Deacon]

On Fri, Jul 04, 2025 at 05:50:16PM +0800, Lei Yang wrote:
> I tested this series of patches with virtio-net regression tests,
> everything works fine.
> 
> Tested-by: Lei Yang <leiyang@redhat.com>

Thanks, but this series doesn't touch virtio-net: it's purely about
the virtio transport for vsock. Do the virtio-net regression tests
exercise that?

Cheers,

Will

Re: [PATCH v2 3/8] vsock/virtio: Move length check to callers of virtio_vsock_skb_rx_put()

[Will Deacon]

On Wed, Jul 02, 2025 at 06:28:37PM +0200, Stefano Garzarella wrote:
> On Tue, Jul 01, 2025 at 05:45:02PM +0100, Will Deacon wrote:
> > virtio_vsock_skb_rx_put() only calls skb_put() if the length in the
> > packet header is not zero even though skb_put() handles this case
> > gracefully.
> > 
> > Remove the functionally redundant check from virtio_vsock_skb_rx_put()
> > and, on the assumption that this is a worthwhile optimisation for
> > handling credit messages, augment the existing length checks in
> > virtio_transport_rx_work() to elide the call for zero-length payloads.
> > Note that the vhost code already has similar logic in
> > vhost_vsock_alloc_skb().
> > 
> > Signed-off-by: Will Deacon <will@kernel.org>
> > ---
> > include/linux/virtio_vsock.h     | 4 +---
> > net/vmw_vsock/virtio_transport.c | 4 +++-
> > 2 files changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/include/linux/virtio_vsock.h b/include/linux/virtio_vsock.h
> > index 36fb3edfa403..eb6980aa19fd 100644
> > --- a/include/linux/virtio_vsock.h
> > +++ b/include/linux/virtio_vsock.h
> > @@ -52,9 +52,7 @@ static inline void virtio_vsock_skb_rx_put(struct sk_buff *skb)
> > 	u32 len;
> > 
> > 	len = le32_to_cpu(virtio_vsock_hdr(skb)->len);
> > -
> > -	if (len > 0)
> > -		skb_put(skb, len);
> > +	skb_put(skb, len);
> 
> Since the caller is supposed to check the len, can we just pass it as
> parameter?
> 
> So we can avoid the `le32_to_cpu(virtio_vsock_hdr(skb)->len)` here.

Sure, I'll do that. It means that virtio_vsock_skb_rx_put() will briefly
be a simple wrapper around skb_put() but once the non-linear handling
comes in then it becomes useful again.

Will

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

[Will Deacon]

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@kernel.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@kernel.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

Re: [PATCH v2 5/8] vsock/virtio: Add vsock helper for linear SKB allocation

[Will Deacon]

On Wed, Jul 02, 2025 at 06:40:17PM +0200, Stefano Garzarella wrote:
> On Tue, Jul 01, 2025 at 05:45:04PM +0100, Will Deacon wrote:
> > In preparation for nonlinear allocations for large SKBs, introduce a
> > new virtio_vsock_alloc_linear_skb() helper to return linear SKBs
> > unconditionally and switch all callers over to this new interface for
> > now.
> > 
> > No functional change.
> > 
> > Signed-off-by: Will Deacon <will@kernel.org>
> > ---
> > drivers/vhost/vsock.c                   | 2 +-
> > include/linux/virtio_vsock.h            | 6 ++++++
> > net/vmw_vsock/virtio_transport.c        | 2 +-
> > net/vmw_vsock/virtio_transport_common.c | 2 +-
> > 4 files changed, 9 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
> > index 66a0f060770e..b13f6be452ba 100644
> > --- a/drivers/vhost/vsock.c
> > +++ b/drivers/vhost/vsock.c
> > @@ -348,7 +348,7 @@ vhost_vsock_alloc_skb(struct vhost_virtqueue *vq,
> > 		return NULL;
> > 
> > 	/* len contains both payload and hdr */
> > -	skb = virtio_vsock_alloc_skb(len, GFP_KERNEL);
> > +	skb = virtio_vsock_alloc_linear_skb(len, GFP_KERNEL);
> > 	if (!skb)
> > 		return NULL;
> > 
> > diff --git a/include/linux/virtio_vsock.h b/include/linux/virtio_vsock.h
> > index 1b5731186095..6d4a933c895a 100644
> > --- a/include/linux/virtio_vsock.h
> > +++ b/include/linux/virtio_vsock.h
> > @@ -70,6 +70,12 @@ static inline struct sk_buff *virtio_vsock_alloc_skb(unsigned int size, gfp_t ma
> > 	return skb;
> > }
> > 
> > +static inline struct sk_buff *
> > +virtio_vsock_alloc_linear_skb(unsigned int size, gfp_t mask)
> > +{
> > +	return virtio_vsock_alloc_skb(size, mask);
> 
> Why not just renaming virtio_vsock_alloc_skb in this patch?
> 
> In that way we are sure when building this patch we don't leave any "old"
> virtio_vsock_alloc_skb() around.

We'll be bringing virtio_vsock_alloc_skb() back almost immediately, but
I can do that if you like.

Will

Re: [PATCH v2 6/8] vhost/vsock: Allocate nonlinear SKBs for handling large receive buffers

[Will Deacon]

On Wed, Jul 02, 2025 at 06:50:59PM +0200, Stefano Garzarella wrote:
> On Tue, Jul 01, 2025 at 05:45:05PM +0100, Will Deacon wrote:
> > -static inline struct sk_buff *virtio_vsock_alloc_skb(unsigned int size, gfp_t mask)
> > +static inline struct sk_buff *
> > +__virtio_vsock_alloc_skb_with_frags(unsigned int header_len,
> > +				    unsigned int data_len,
> > +				    gfp_t mask)
> > {
> > 	struct sk_buff *skb;
> > +	int err;
> > 
> > -	if (size < VIRTIO_VSOCK_SKB_HEADROOM)
> > -		return NULL;
> 
> I would have made this change in a separate patch, but IIUC the only other
> caller is virtio_transport_alloc_skb() where this condition is implied,
> right?

At this point in the series, virtio_vsock_alloc_skb() only has a single
caller (vhost_vsock_alloc_skb()) which already has a partial bounds check
and so this patch extends it to cover the lower bound.

Later (in "vsock/virtio: Allocate nonlinear SKBs for handling large
transmit buffers"), virtio_transport_alloc_skb() gets converted over
and that's fine because it never allocates less than
VIRTIO_VSOCK_SKB_HEADROOM.

> I don't know, maybe we could have one patch where you touch this and
> virtio_vsock_skb_rx_put(), and another where you introduce nonlinear
> allocation for vhost/vsock.  What do you think? (not a strong opinion, just
> worried about doing 2 things in a single patch)

I can spin a separate patch for the bounds check.

Will