Thread (4 messages) 4 messages, 3 authors, 2025-10-29

Re: [PATCH net] virtio-net: calculate header alignment mask based on features

From: "Michael S. Tsirkin" <mst@redhat.com>
Date: 2025-10-28 14:38:31
Also in: lkml, netdev, stable
Subsystem: networking drivers, the rest, virtio core, virtio net driver · Maintainers: Andrew Lunn, "David S. Miller", Eric Dumazet, Jakub Kicinski, Paolo Abeni, Linus Torvalds, "Michael S. Tsirkin", Jason Wang

On Tue, Oct 28, 2025 at 11:03:41AM +0800, Jason Wang wrote:
Commit 56a06bd40fab ("virtio_net: enable gso over UDP tunnel support.")
switches to check the alignment of the virtio_net_hdr_v1_hash_tunnel
even when doing the transmission even if the feature is not
negotiated. This will cause 

you mean this causes
a series performance degradation of pktgen
as the skb->data can't satisfy the alignment requirement due to the
increase of the header size then virtio-net must prepare at least 2
sgs with indirect descriptors which will introduce overheads in the

introduces, accordinglt
device.

Fixing this by calculate the header alignment during probe so when
tunnel gso is not negotiated, we can less strict.

Pktgen in guest + XDP_DROP on TAP + vhost_net shows the TX PPS is
recovered from 2.4Mpps to 4.45Mpps.

Note that we still need a way to recover the performance when tunnel
gso is enabled, probably a new vnet header format.
you mean improve, not recover as such

quoted hunk ↗ jump to hunk
Fixes: 56a06bd40fab ("virtio_net: enable gso over UDP tunnel support.")
Cc: stable@vger.kernel.org
Signed-off-by: Jason Wang <redacted>
---
 drivers/net/virtio_net.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 31bd32bdecaf..5b851df749c0 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -441,6 +441,9 @@ struct virtnet_info {
 	/* Packet virtio header size */
 	u8 hdr_len;
 
+	/* header alignment */
+	size_t hdr_align;
+
It makes no sense to have u8 for length but size_t for alignment,
and u8 would fit in a memory hole we have, anyway.
quoted hunk ↗ jump to hunk
 	/* Work struct for delayed refilling if we run low on memory. */
 	struct delayed_work refill;
 
@@ -3308,8 +3311,9 @@ static int xmit_skb(struct send_queue *sq, struct sk_buff *skb, bool orphan)
 	pr_debug("%s: xmit %p %pM\n", vi->dev->name, skb, dest);
 
 	can_push = vi->any_header_sg &&
-		!((unsigned long)skb->data & (__alignof__(*hdr) - 1)) &&
+		!((unsigned long)skb->data & (vi->hdr_align - 1)) &&

So let me get it straight.
We use the alignment check to be able to cast to the correct type.
The issue is that alignment for the header changed. 

virtio_net_hdr_v1 has 2 byte alignment, but:



struct virtio_net_hdr_v1_hash {
        struct virtio_net_hdr_v1 hdr; 
        __le32 hash_value;
#define VIRTIO_NET_HASH_REPORT_NONE            0
#define VIRTIO_NET_HASH_REPORT_IPv4            1
#define VIRTIO_NET_HASH_REPORT_TCPv4           2
#define VIRTIO_NET_HASH_REPORT_UDPv4           3
#define VIRTIO_NET_HASH_REPORT_IPv6            4
#define VIRTIO_NET_HASH_REPORT_TCPv6           5
#define VIRTIO_NET_HASH_REPORT_UDPv6           6
#define VIRTIO_NET_HASH_REPORT_IPv6_EX         7
#define VIRTIO_NET_HASH_REPORT_TCPv6_EX        8
#define VIRTIO_NET_HASH_REPORT_UDPv6_EX        9
        __le16 hash_report; 
        __le16 padding;
};      


has 4 byte due to hash_value, and accordingly:


struct virtio_net_hdr_v1_hash_tunnel {
        struct virtio_net_hdr_v1_hash hash_hdr;
        __le16 outer_th_offset;                                
        __le16 inner_nh_offset;
};              


now is 4 byte aligned so everything is messed up:
net tends not to be 4 byte aligned.



quoted hunk ↗ jump to hunk
 		!skb_header_cloned(skb) && skb_headroom(skb) >= hdr_len;
+
 	/* Even if we can, don't push here yet as this would skew
 	 * csum_start offset below. */
 	if (can_push)
@@ -6926,15 +6930,20 @@ static int virtnet_probe(struct virtio_device *vdev)
 	}
 
 	if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO) ||
-	    virtio_has_feature(vdev, VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO))
+	    virtio_has_feature(vdev, VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO)) {
 		vi->hdr_len = sizeof(struct virtio_net_hdr_v1_hash_tunnel);
-	else if (vi->has_rss_hash_report)
+		vi->hdr_align = __alignof__(struct virtio_net_hdr_v1_hash_tunnel);
+	} else if (vi->has_rss_hash_report) {
 		vi->hdr_len = sizeof(struct virtio_net_hdr_v1_hash);
-	else if (virtio_has_feature(vdev, VIRTIO_NET_F_MRG_RXBUF) ||
-		 virtio_has_feature(vdev, VIRTIO_F_VERSION_1))
+		vi->hdr_align = __alignof__(struct virtio_net_hdr_v1_hash);
+	} else if (virtio_has_feature(vdev, VIRTIO_NET_F_MRG_RXBUF) ||
+		virtio_has_feature(vdev, VIRTIO_F_VERSION_1)) {
 		vi->hdr_len = sizeof(struct virtio_net_hdr_mrg_rxbuf);
-	else
+		vi->hdr_align = __alignof__(struct virtio_net_hdr_mrg_rxbuf);
+	} else {
 		vi->hdr_len = sizeof(struct virtio_net_hdr);
+		vi->hdr_align = __alignof__(struct virtio_net_hdr);
+	}
 
 	if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO_CSUM))
 		vi->rx_tnl_csum = true;
So how about just fixing the root cause then?
Like this (untested, if you agree pls take over this):

---

virtio_net: fix alignment for virtio_net_hdr_v1_hash


changing alignment of header would mean it's no longer safe to cast a 2
byte aligned pointer between formats. Use two 16 bit fields to make it 2
byte aligned as previously.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

-- 
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 31bd32bdecaf..02ce5316f47d 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2535,6 +2535,13 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
 	return NULL;
 }
 
+static inline u16
+virtio_net_hash_value(const struct virtio_net_hdr_v1_hash *hdr_hash)
+{
+	return __le16_to_cpu(hdr_hash->hash_value_lo) |
+		(__le16_to_cpu(hdr_hash->hash_value_hi) << 16);
+}
+
 static void virtio_skb_set_hash(const struct virtio_net_hdr_v1_hash *hdr_hash,
 				struct sk_buff *skb)
 {
@@ -2561,7 +2568,7 @@ static void virtio_skb_set_hash(const struct virtio_net_hdr_v1_hash *hdr_hash,
 	default:
 		rss_hash_type = PKT_HASH_TYPE_NONE;
 	}
-	skb_set_hash(skb, __le32_to_cpu(hdr_hash->hash_value), rss_hash_type);
+	skb_set_hash(skb, virtio_net_hash_value(hdr_hash), rss_hash_type);
 }
 
 static void virtnet_receive_done(struct virtnet_info *vi, struct receive_queue *rq,
@@ -3307,6 +3314,10 @@ static int xmit_skb(struct send_queue *sq, struct sk_buff *skb, bool orphan)
 
 	pr_debug("%s: xmit %p %pM\n", vi->dev->name, skb, dest);
 
+	/* Make sure it's safe to cast between formats */
+	BUILD_BUG_ON(__alignof__(*hdr) != __alignof__(hdr->hash_hdr));
+	BUILD_BUG_ON(__alignof__(*hdr) != __alignof__(hdr->hash_hdr.hdr));
+
 	can_push = vi->any_header_sg &&
 		!((unsigned long)skb->data & (__alignof__(*hdr) - 1)) &&
 		!skb_header_cloned(skb) && skb_headroom(skb) >= hdr_len;
@@ -6755,7 +6766,7 @@ static int virtnet_xdp_rx_hash(const struct xdp_md *_ctx, u32 *hash,
 		hash_report = VIRTIO_NET_HASH_REPORT_NONE;
 
 	*rss_type = virtnet_xdp_rss_type[hash_report];
-	*hash = __le32_to_cpu(hdr_hash->hash_value);
+	*hash = virtio_net_hash_value(hdr_hash);
 	return 0;
 }
 
diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
index 8bf27ab8bcb4..1db45b01532b 100644
--- a/include/uapi/linux/virtio_net.h
+++ b/include/uapi/linux/virtio_net.h
@@ -193,7 +193,8 @@ struct virtio_net_hdr_v1 {
 
 struct virtio_net_hdr_v1_hash {
 	struct virtio_net_hdr_v1 hdr;
-	__le32 hash_value;
+	__le16 hash_value_lo;
+	__le16 hash_value_hi;
 #define VIRTIO_NET_HASH_REPORT_NONE            0
 #define VIRTIO_NET_HASH_REPORT_IPv4            1
 #define VIRTIO_NET_HASH_REPORT_TCPv4           2
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help