Re: [RFC v4 03/18] page_pool: use netmem alloc/put APIs in __page_pool_alloc_page_order()

[Pavel Begunkov <asml.silence@gmail.com>]

On 6/5/25 20:39, Mina Almasry wrote:
> On Thu, Jun 5, 2025 at 3:25 AM Pavel Begunkov <asml.silence@gmail.com> wrote:
>>
>> On 6/4/25 03:52, Byungchul Park wrote:
>>> Use netmem alloc/put APIs instead of page alloc/put APIs and make it
>>> return netmem_ref instead of struct page * in
>>> __page_pool_alloc_page_order().
>>>
>>> Signed-off-by: Byungchul Park <byungchul@sk.com>
>>> Reviewed-by: Mina Almasry <almasrymina@google.com>
>>> ---
>>>    net/core/page_pool.c | 26 +++++++++++++-------------
>>>    1 file changed, 13 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
>>> index 4011eb305cee..523354f2db1c 100644
>>> --- a/net/core/page_pool.c
>>> +++ b/net/core/page_pool.c
>>> @@ -518,29 +518,29 @@ static bool page_pool_dma_map(struct page_pool *pool, netmem_ref netmem, gfp_t g
>>>        return false;
>>>    }
>>>
>>> -static struct page *__page_pool_alloc_page_order(struct page_pool *pool,
>>> -                                              gfp_t gfp)
>>> +static netmem_ref __page_pool_alloc_page_order(struct page_pool *pool,
>>> +                                            gfp_t gfp)
>>>    {
>>> -     struct page *page;
>>> +     netmem_ref netmem;
>>>
>>>        gfp |= __GFP_COMP;
>>> -     page = alloc_pages_node(pool->p.nid, gfp, pool->p.order);
>>> -     if (unlikely(!page))
>>> -             return NULL;
>>> +     netmem = alloc_netmems_node(pool->p.nid, gfp, pool->p.order);
>>> +     if (unlikely(!netmem))
>>> +             return 0;
>>>
>>> -     if (pool->dma_map && unlikely(!page_pool_dma_map(pool, page_to_netmem(page), gfp))) {
>>> -             put_page(page);
>>> -             return NULL;
>>> +     if (pool->dma_map && unlikely(!page_pool_dma_map(pool, netmem, gfp))) {
>>> +             put_netmem(netmem);
>>
>> It's a bad idea to have {put,get}_netmem in page pool's code, it has a
>> different semantics from what page pool expects for net_iov. I.e.
>> instead of releasing the netmem and allowing it to be reallocated by
>> page pool, put_netmem(niov) will drop a memory provider reference and
>> leak the net_iov. Depending on implementation it might even underflow
>> mp refs if a net_iov is ever passed here.
>>
> 
> Hmm, put_netmem (I hope) is designed and implemented to do the right
> thing no matter what netmem you pass it (and it needs to, because we
> can't predict what netmem will be passed to it):
> 
> - For non-pp pages, it drops a page ref.
> - For pp pages, it drops a pp ref.
> - For non-pp net_iovs (devmem TX), it drops a net_iov ref (which for
> devmem net_iovs is a binding ref)
> - For pp net_iovs, it drops a niov->pp ref (the same for both iouring
> and devmem).

void put_netmem(netmem_ref netmem)
{
	struct net_iov *niov;

	if (netmem_is_net_iov(netmem)) {
		niov = netmem_to_net_iov(netmem);
		if (net_is_devmem_iov(niov))
			net_devmem_put_net_iov(netmem_to_net_iov(netmem));
		return;
	}

	put_page(netmem_to_page(netmem));
}
EXPORT_SYMBOL(put_netmem);

void net_devmem_put_net_iov(struct net_iov *niov)
{
	net_devmem_dmabuf_binding_put(net_devmem_iov_binding(niov));
}

Am I looking at an outdated version? for devmem net_iov it always puts
the binding and not niov refs, and it's always does put_page for pages.
And it'd also silently ignore io_uring. And we're also patching early
alloc/init failures in this series, so gauging if it's pp or non-pp
originated struct page might be dangerous and depend on init order. We
don't even need to think about all that if we continue to use put_page,
which is why I think it's a much better option.


> In my estimation using it should be safe to use put_netmem here, but
> I'm not opposed to reverting to put_page here, since we're sure it's a
> page in this call path anyway.
> 

-- 
Pavel Begunkov