Re: [RFC PATCH net-next v2 1/2] virtio-net: support zerocopy multi buffer XDP in mergeable
From: Jason Wang <jasowang@redhat.com>
Date: 2025-05-29 05:59:40
Also in:
bpf, lkml, virtualization
On Wed, May 28, 2025 at 12:19 AM Bui Quang Minh [off-list ref] wrote:
quoted hunk ↗ jump to hunk
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 <redacted> --- 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.
+ * 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.
quoted hunk ↗ jump to hunk
+ 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?
+ 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?
+ 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?
quoted hunk ↗ jump to hunk
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