Re: [PATCH net-next v11] virtio_net: add page_pool support for buffer allocation

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Mar 23, 2026 at 01:50:29PM -0400, Vishwanath Seshagiri wrote:
> 
> 
> On 3/23/26 10:39 PM, Omar Elghoul wrote:
> > On 3/23/26 12:58 PM, Michael S. Tsirkin wrote:
> > 
> > > On Mon, Mar 23, 2026 at 11:52:34AM -0400, Michael S. Tsirkin wrote:
> > > > On Mon, Mar 23, 2026 at 11:01:31AM -0400, Omar Elghoul wrote:
> > > > > [...]
> > > > Well... I am not sure how I missed it. Obvious in hindsight:
> > > > 
> > > > static void receive_buf(struct virtnet_info *vi, struct
> > > > receive_queue *rq,
> > > >                          void *buf, unsigned int len, void **ctx,
> > > >                          unsigned int *xdp_xmit,
> > > >                          struct virtnet_rq_stats *stats)
> > > > {
> > > >          struct net_device *dev = vi->dev;
> > > >          struct sk_buff *skb;
> > > >          u8 flags;
> > > >          if (unlikely(len < vi->hdr_len + ETH_HLEN)) {
> > > >                  pr_debug("%s: short packet %i\n", dev->name, len);
> > > >                  DEV_STATS_INC(dev, rx_length_errors);
> > > >                  virtnet_rq_free_buf(vi, rq, buf);
> > > >                  return;
> > > >          }
> > > >          /* About the flags below:
> > > >           * 1. Save the flags early, as the XDP program might
> > > > overwrite them.
> > > >           * These flags ensure packets marked as
> > > > VIRTIO_NET_HDR_F_DATA_VALID
> > > >           * stay valid after XDP processing.
> > > >           * 2. XDP doesn't work with partially checksummed
> > > > packets (refer to
> > > >           * virtnet_xdp_set()), so packets marked as
> > > >           * VIRTIO_NET_HDR_F_NEEDS_CSUM get dropped during XDP
> > > > processing.
> > > >           */
> > > >          if (vi->mergeable_rx_bufs) {
> > > >                  flags = ((struct virtio_net_common_hdr *)buf)-
> > > > >hdr.flags;
> > > >                  skb = receive_mergeable(dev, vi, rq, buf, ctx,
> > > > len, xdp_xmit,
> > > >                                          stats);
> > > >          } else if (vi->big_packets) {
> > > >                  void *p = page_address((struct page *)buf);
> > > >                  flags = ((struct virtio_net_common_hdr *)p)->hdr.flags;
> > > >                  skb = receive_big(dev, vi, rq, buf, len, stats);
> > > >          } else {
> > > >                  flags = ((struct virtio_net_common_hdr *)buf)-
> > > > >hdr.flags;
> > > >                  skb = receive_small(dev, vi, rq, buf, ctx, len,
> > > > xdp_xmit, stats);
> > > >          }
> > > > 
> > > > 
> > > > So we are reading the header, before dma sync, which is within
> > > > receive_mergeable and friends:
> > > > 
> > > > static struct sk_buff *receive_mergeable(struct net_device *dev,
> > > >                                           struct virtnet_info *vi,
> > > >                                           struct receive_queue *rq,
> > > >                                           void *buf,
> > > >                                           void *ctx,
> > > >                                           unsigned int len,
> > > >                                           unsigned int *xdp_xmit,
> > > >                                           struct
> > > > virtnet_rq_stats *stats)
> > > > {
> > > >          struct virtio_net_hdr_mrg_rxbuf *hdr = buf;
> > > >          int num_buf = virtio16_to_cpu(vi->vdev, hdr->num_buffers);
> > > >          struct page *page = virt_to_head_page(buf);
> > > >          int offset = buf - page_address(page);
> > > >          struct sk_buff *head_skb, *curr_skb;
> > > >          unsigned int truesize = mergeable_ctx_to_truesize(ctx);
> > > >          unsigned int headroom = mergeable_ctx_to_headroom(ctx);
> > > >          head_skb = NULL;
> > > >          if (rq->use_page_pool_dma)
> > > >                  page_pool_dma_sync_for_cpu(rq->page_pool, page,
> > > > offset, len);
> > > > 
> > > > 
> > > > Just as a test, the below should fix it (compiled only), but the real
> > > > fix is more complex since we need to be careful to avoid
> > > > expensive syncing
> > > > twice.
> > > > 
> > > > 
> > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > index 97035b49bae7..57b4f5954bed 100644
> > > > --- a/drivers/net/virtio_net.c
> > > > +++ b/drivers/net/virtio_net.c
> > > > @@ -931,9 +931,19 @@ static struct sk_buff *page_to_skb(struct
> > > > virtnet_info *vi,
> > > >   static void *virtnet_rq_get_buf(struct receive_queue *rq, u32
> > > > *len, void **ctx)
> > > >   {
> > > > +    void *buf;
> > > > +
> > > >       BUG_ON(!rq->page_pool);
> > > > -    return virtqueue_get_buf_ctx(rq->vq, len, ctx);
> > > > +    buf = virtqueue_get_buf_ctx(rq->vq, len, ctx);
> > > > +    if (buf && rq->use_page_pool_dma && *len) {
> > > > +        struct page *page = virt_to_head_page(buf);
> > > > +        int offset = buf - page_address(page);
> > > > +
> > > > +        page_pool_dma_sync_for_cpu(rq->page_pool, page, offset, *len);
> > > > +    }
> > > > +
> > > > +    return buf;
> > > >   }
> > > >   static void virtnet_rq_unmap_free_buf(struct virtqueue *vq, void *buf)
> > > > 
> > > > 
> > > > 
> > > > 
> > > > -- 
> > > > MST
> > > or maybe like this:
> > Both of these patches resolve the issue on my end.
> > > 
> > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > index 97035b49bae7..835f52651006 100644
> > > --- a/drivers/net/virtio_net.c
> > > +++ b/drivers/net/virtio_net.c
> > > @@ -1956,13 +1956,6 @@ static struct sk_buff *receive_small(struct
> > > net_device *dev,
> > >        */
> > >       buf -= VIRTNET_RX_PAD + xdp_headroom;
> > > -    if (rq->use_page_pool_dma) {
> > > -        int offset = buf - page_address(page) +
> > > -                 VIRTNET_RX_PAD + xdp_headroom;
> > > -
> > > -        page_pool_dma_sync_for_cpu(rq->page_pool, page, offset, len);
> > > -    }
> > > -
> > >       len -= vi->hdr_len;
> > >       u64_stats_add(&stats->bytes, len);
> > > @@ -2398,9 +2391,6 @@ static struct sk_buff
> > > *receive_mergeable(struct net_device *dev,
> > >       head_skb = NULL;
> > > -    if (rq->use_page_pool_dma)
> > > -        page_pool_dma_sync_for_cpu(rq->page_pool, page, offset, len);
> > > -
> > >       u64_stats_add(&stats->bytes, len - vi->hdr_len);
> > >       if (check_mergeable_len(dev, ctx, len))
> > > @@ -2563,6 +2553,13 @@ static void receive_buf(struct virtnet_info
> > > *vi, struct receive_queue *rq,
> > >           return;
> > >       }
> > > +    if (rq->use_page_pool_dma) {
> > > +        struct page *page = virt_to_head_page(buf);
> > > +        int offset = buf - page_address(page);
> > > +
> > > +        page_pool_dma_sync_for_cpu(rq->page_pool, page, offset, len);
> > > +    }
> > > +
> > >       /* About the flags below:
> > >        * 1. Save the flags early, as the XDP program might overwrite
> > > them.
> > >        * These flags ensure packets marked as VIRTIO_NET_HDR_F_DATA_VALID
> > > 
> 
> Hello Omar. Thank you for the bisect, apologies for breaking the
> functionality. Thank you Michael for the patch, I will be careful next
> time about DMA sync issues. Looks like the bug is fixed, is there
> anything else needed from my end?
> 


Would be great to have you test the patch on your end and report
the performace. Thanks!

-- 
MST