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

[Vishwanath Seshagiri <vishs@meta.com>]

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?