On 5/29/25 12:59, Jason Wang wrote:
On Wed, May 28, 2025 at 12:19 AM Bui Quang Minh
<minhquangbu...@gmail.com> wrote:
Currently, in zerocopy mode with mergeable receive buffer, virtio-net
does not support multi buffer but a single buffer only. This commit adds
support for multi mergeable receive buffer in the zerocopy XDP path by
utilizing XDP buffer with frags.
Signed-off-by: Bui Quang Minh <minhquangbu...@gmail.com>
---
drivers/net/virtio_net.c | 123 +++++++++++++++++++++------------------
1 file changed, 66 insertions(+), 57 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index e53ba600605a..a9558650f205 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -45,6 +45,8 @@ module_param(napi_tx, bool, 0644);
#define VIRTIO_XDP_TX BIT(0)
#define VIRTIO_XDP_REDIR BIT(1)
+#define VIRTNET_MAX_ZC_SEGS 8
+
/* RX packet size EWMA. The average packet size is used to determine the
packet
* buffer size when refilling RX rings. As the entire RX ring may be refilled
* at once, the weight is chosen so that the EWMA will be insensitive to
short-
@@ -1232,65 +1234,53 @@ static void xsk_drop_follow_bufs(struct net_device *dev,
}
}
-static int xsk_append_merge_buffer(struct virtnet_info *vi,
- struct receive_queue *rq,
- struct sk_buff *head_skb,
- u32 num_buf,
- struct virtio_net_hdr_mrg_rxbuf *hdr,
- struct virtnet_rq_stats *stats)
+static int virtnet_build_xsk_buff_mrg(struct virtnet_info *vi,
+ struct receive_queue *rq,
+ u32 num_buf,
+ struct xdp_buff *xdp,
+ struct virtnet_rq_stats *stats)
{
- struct sk_buff *curr_skb;
- struct xdp_buff *xdp;
- u32 len, truesize;
- struct page *page;
+ unsigned int len;
void *buf;
- curr_skb = head_skb;
+ if (num_buf < 2)
+ return 0;
+
+ while (num_buf > 1) {
+ struct xdp_buff *new_xdp;
- while (--num_buf) {
buf = virtqueue_get_buf(rq->vq, &len);
- if (unlikely(!buf)) {
- pr_debug("%s: rx error: %d buffers out of %d missing\n",
- vi->dev->name, num_buf,
- virtio16_to_cpu(vi->vdev,
- hdr->num_buffers));
+ if (!unlikely(buf)) {
+ pr_debug("%s: rx error: %d buffers missing\n",
+ vi->dev->name, num_buf);
DEV_STATS_INC(vi->dev, rx_length_errors);
- return -EINVAL;
- }
-
- u64_stats_add(&stats->bytes, len);
-
- xdp = buf_to_xdp(vi, rq, buf, len);
- if (!xdp)
- goto err;
-
- buf = napi_alloc_frag(len);
- if (!buf) {
- xsk_buff_free(xdp);
- goto err;
+ return -1;
}
- memcpy(buf, xdp->data - vi->hdr_len, len);
-
- xsk_buff_free(xdp);
+ new_xdp = buf_to_xdp(vi, rq, buf, len);
+ if (!new_xdp)
+ goto drop_bufs;
- page = virt_to_page(buf);
+ /* In virtnet_add_recvbuf_xsk(), we ask the host to fill from
+ * xdp->data - vi->hdr_len with both virtio_net_hdr and data.
+ * However, only the first packet has the virtio_net_hdr, the
+ * following ones do not. So we need to adjust the following
Typo here.
I'm sorry, could you clarify which word contains the typo?
+ * packets' data pointer to the correct place.
+ */
I wonder what happens if we don't use this trick? I meant we don't
reuse the header room for the virtio-net header. This seems to be fine
for a mergeable buffer and can help to reduce the trick.
I don't think using the header room for virtio-net header creates this
case handling. In my opinion, it comes from the slightly difference in
the recvbuf between single buffer and multi-buffer. When we have n
single-buffer packets, each buffer will have its own virtio-net header.
But when we have 1 multi-buffer packet (which spans across n buffers),
only the first buffer has virtio-net header, the following buffers do not.
There 2 important pointers here. The pointer we announce to the vhost
side to fill the data, let's call it announced_addr, and xdp_buff->data
which is expected to point the the start of Ethernet frame. Currently,
announced_addr = xdp_buff->data - hdr_len
The host side will write the virtio-net header to announced_addr then
the Ethernet frame's data in the first buffer. In case of multi-buffer
packet, in the following buffers, host side writes the Ethernet frame's
data to the announced_addr no virtio-net header. So in the virtio-net,
we need to subtract xdp_buff->data, otherwise, we lose some Ethernet
frame's data.
I think a slightly better solution is that we set announced_addr =
xdp_buff->data then we only need to xdp_buff->data += hdr_len for the
first buffer and do need to adjust xdp_buff->data of the following buffers.
+ new_xdp->data -= vi->hdr_len;
+ new_xdp->data_end = new_xdp->data + len;
- truesize = len;
+ if (!xsk_buff_add_frag(xdp, new_xdp))
+ goto drop_bufs;
- curr_skb = virtnet_skb_append_frag(head_skb, curr_skb, page,
- buf, len, truesize);
- if (!curr_skb) {
- put_page(page);
- goto err;
- }
+ num_buf--;
}
return 0;
-err:
+drop_bufs:
xsk_drop_follow_bufs(vi->dev, rq, num_buf, stats);
- return -EINVAL;
+ return -1;
}
static struct sk_buff *virtnet_receive_xsk_merge(struct net_device *dev,
struct virtnet_info *vi,
@@ -1307,23 +1297,42 @@ static struct sk_buff *virtnet_receive_xsk_merge(struct
net_device *dev, struct
num_buf = virtio16_to_cpu(vi->vdev, hdr->num_buffers);
ret = XDP_PASS;
+ if (virtnet_build_xsk_buff_mrg(vi, rq, num_buf, xdp, stats))
+ goto drop;
+
rcu_read_lock();
prog = rcu_dereference(rq->xdp_prog);
- /* TODO: support multi buffer. */
- if (prog && num_buf == 1)
- ret = virtnet_xdp_handler(prog, xdp, dev, xdp_xmit, stats);
Without this patch it looks like we had a bug:
ret = XDP_PASS;
rcu_read_lock();
prog = rcu_dereference(rq->xdp_prog);
/* TODO: support multi buffer. */
if (prog && num_buf == 1)
ret = virtnet_xdp_handler(prog, xdp, dev, xdp_xmit, stats);
rcu_read_unlock();
This implies if num_buf is greater than 1, we will assume XDP_PASS?
Yes, I think XDP_DROP should be returned in that case.
+ if (prog) {
+ /* We are in zerocopy mode so we cannot copy the multi-buffer
+ * xdp buff to a single linear xdp buff. If we do so, in case
+ * the BPF program decides to redirect to a XDP socket (XSK),
+ * it will trigger the zerocopy receive logic in XDP socket.
+ * The receive logic thinks it receives zerocopy buffer while
+ * in fact, it is the copy one and everything is messed up.
+ * So just drop the packet here if we have a multi-buffer xdp
+ * buff and the BPF program does not support it.
+ */
+ if (xdp_buff_has_frags(xdp) && !prog->aux->xdp_has_frags)
+ ret = XDP_DROP;
Could we move the check before trying to build a multi-buffer XDP buff?
Yes, I'll fix this in next version.
+ else
+ ret = virtnet_xdp_handler(prog, xdp, dev, xdp_xmit,
+ stats);
+ }
rcu_read_unlock();
switch (ret) {
case XDP_PASS:
- skb = xsk_construct_skb(rq, xdp);
+ skb = xdp_build_skb_from_zc(xdp);
Is this better to make this change a separate patch?
Okay, I'll create a separate patch to convert the current XDP_PASS
handler to use xdp_build_skb_from_zc helper.
if (!skb)
- goto drop_bufs;
+ break;
- if (xsk_append_merge_buffer(vi, rq, skb, num_buf, hdr, stats)) {
- dev_kfree_skb(skb);
- goto drop;
- }
+ /* Later, in virtnet_receive_done(), eth_type_trans()
+ * is called. However, in xdp_build_skb_from_zc(), it is called
+ * already. As a result, we need to reset the data to before
+ * the mac header so that the later call in
+ * virtnet_receive_done() works correctly.
+ */
+ skb_push(skb, ETH_HLEN);
return skb;
@@ -1332,14 +1341,11 @@ static struct sk_buff *virtnet_receive_xsk_merge(struct
net_device *dev, struct
return NULL;
default:
- /* drop packet */
- xsk_buff_free(xdp);
+ break;
}
-drop_bufs:
- xsk_drop_follow_bufs(dev, rq, num_buf, stats);
-
drop:
+ xsk_buff_free(xdp);
u64_stats_inc(&stats->drops);
return NULL;
}
@@ -1396,6 +1402,8 @@ static int virtnet_add_recvbuf_xsk(struct virtnet_info
*vi, struct receive_queue
return -ENOMEM;
len = xsk_pool_get_rx_frame_size(pool) + vi->hdr_len;
+ /* Reserve some space for skb_shared_info */
+ len -= SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
for (i = 0; i < num; ++i) {
/* Use the part of XDP_PACKET_HEADROOM as the virtnet hdr
space.
@@ -6734,6 +6742,7 @@ static int virtnet_probe(struct virtio_device *vdev)
dev->netdev_ops = &virtnet_netdev;
dev->stat_ops = &virtnet_stat_ops;
dev->features = NETIF_F_HIGHDMA;
+ dev->xdp_zc_max_segs = VIRTNET_MAX_ZC_SEGS;
dev->ethtool_ops = &virtnet_ethtool_ops;
SET_NETDEV_DEV(dev, &vdev->dev);
--
2.43.0
Thanks
