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

[Mina Almasry <almasrymina@google.com>]

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).

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.

-- 
Thanks,
Mina