Re: [PATCH v6] virtio_net: add page_pool support for buffer allocation

[Vishwanath Seshagiri <vishs@meta.com>]

On 2/8/26 11:12 PM, Xuan Zhuo wrote:
>>>>
>>>> +static void virtnet_put_page(struct receive_queue *rq, struct page *page,
>>>> +			     bool allow_direct)
>>>> +{
>>>> +	if (page_pool_page_is_pp(page))
>>>> +		page_pool_put_page(rq->page_pool, page, -1, allow_direct);
>>>> +	else
>>>> +		put_page(page);
>>>> +}
>>>
>>> Why we need this?
>>> For the caller, we should know which one should be used?
>>>
>>
>> This was after some feedback to unify the alloc/free path checks in v4.
>> But you raise a valid point - callers already know the mode via
>> virtnet_no_page_pool(). I can simplify this to just call
>> page_pool_put_page() directly, since virtnet_put_page() is only called
>> from paths that already checked we're using page_pool. Would you prefer
>> that?
> 
> 
> Based on my understanding, the big mode should directly call the Page API, while
> all other modes should directly call the PP API. Therefore, I believe it's
> better for each mode to directly invoke its respective API.

ack. I will move this into each callsite.

>>>>
>>>> +static int virtnet_create_page_pools(struct virtnet_info *vi)
>>>> +{
>>>> +	int i, err;
>>>> +
>>>> +	if (!vi->mergeable_rx_bufs && vi->big_packets)
>>>> +		return 0;
>>>> +
>>>> +	for (i = 0; i < vi->max_queue_pairs; i++) {
>>>> +		struct receive_queue *rq = &vi->rq[i];
>>>> +		struct page_pool_params pp_params = { 0 };
>>>> +		struct device *dma_dev;
>>>> +
>>>> +		if (rq->page_pool)
>>>> +			continue;
>>>> +
>>>> +		if (rq->xsk_pool)
>>>> +			continue;
>>>> +
>>>> +		pp_params.order = 0;
>>>> +		pp_params.pool_size = virtqueue_get_vring_size(rq->vq);
>>>> +		pp_params.nid = dev_to_node(vi->vdev->dev.parent);
>>>> +		pp_params.netdev = vi->dev;
>>>> +		pp_params.napi = &rq->napi;
>>>> +
>>>> +		/* Check if backend supports DMA API (e.g., vhost, virtio-pci).
>>>> +		 * If so, use page_pool's DMA mapping for premapped buffers.
>>>> +		 * Otherwise (e.g., VDUSE), page_pool only handles allocation.
>>>> +		 */
>>>> +		dma_dev = virtqueue_dma_dev(rq->vq);
>>>> +		if (dma_dev) {
>>>> +			pp_params.dev = dma_dev;
>>>> +			pp_params.flags = PP_FLAG_DMA_MAP;
>>>> +			pp_params.dma_dir = DMA_FROM_DEVICE;
>>>> +			rq->use_page_pool_dma = true;
>>>> +		} else {
>>>> +			pp_params.dev = vi->vdev->dev.parent;
>>>> +			pp_params.flags = 0;
>>>> +			rq->use_page_pool_dma = false;
>>>
>>> Can the page pool handles dma with vi->vdev->dev.parent?
>>
>> No, we cannot use the page_pool DMA with vi->vdev->dev.parent in VDUSE
>> case because VDUSE uses its own address translation. virtqueue_dma_dev()
>> returns NULL, virtio doesn't use standard DMA API at all. Now that I
>> think about it, setting pp_params.dev in this branch is unnecessary
>> since it is never accessed. I can remove it, if you prefer.
> 
> If that's the case, then it is indeed a bit troublesome. I don't know if VDUSE
> has a better solution. What I don't like is use_page_pool_dma -- it
> introduces many branches into the code, making it more chaotic. We may need to
> look for a better unified solution.
> 
> Thanks.

VDUSE does not have a DMA device. virtqueue_dma_dev() returns NULL and
virtqueue_map_single_attrs() just returns virt_to_phys(). There's
nothing to map or sync. These branches exist because page_pool's DMA
APIs require a configured DMA device. I can remove the use_page_pool_dma
and check pool->dma_map directly to reduce state. However, some amount
of branching is unavoidable unless page_pool adds a no-op variant of its
DMA functions.

To bring parity with vhost/virtio-pci, VDUSE would need to implement the
standard DMA API, which conflicts with VDUSE's fundamental architecture
since it uses its own IOVA translation for userpsace access. I don't see
a way to avoid these branches without dropping page_pool for VDUSE,
which I'd prefer not to do. Open to suggestions if I have missed
something.

> 
>>
>>
>>>
>>> Thanks.
>>>