Re: [PATCH v7 06/15] net: page pool: add helper creating area from pages

[Mina Almasry <almasrymina@google.com>]

On Tue, Oct 29, 2024 at 4:06 PM David Wei <dw@davidwei.uk> wrote:
>
> From: Pavel Begunkov <asml.silence@gmail.com>
>
> Add a helper that takes an array of pages and initialises passed in
> memory provider's area with them, where each net_iov takes one page.
> It's also responsible for setting up dma mappings.
>
> We keep it in page_pool.c not to leak netmem details to outside
> providers like io_uring, which don't have access to netmem_priv.h
> and other private helpers.
>

I honestly prefer leaking netmem_priv.h details into the io_uring
rather than having io_uring provider specific code in page_pool.c.

> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> Signed-off-by: David Wei <dw@davidwei.uk>
> ---
>  include/net/page_pool/memory_provider.h | 10 ++++
>  net/core/page_pool.c                    | 63 ++++++++++++++++++++++++-
>  2 files changed, 71 insertions(+), 2 deletions(-)
>  create mode 100644 include/net/page_pool/memory_provider.h
>
> diff --git a/include/net/page_pool/memory_provider.h b/include/net/page_pool/memory_provider.h
> new file mode 100644
> index 000000000000..83d7eec0058d
> --- /dev/null
> +++ b/include/net/page_pool/memory_provider.h
> @@ -0,0 +1,10 @@
> +#ifndef _NET_PAGE_POOL_MEMORY_PROVIDER_H
> +#define _NET_PAGE_POOL_MEMORY_PROVIDER_H
> +
> +int page_pool_mp_init_paged_area(struct page_pool *pool,
> +                               struct net_iov_area *area,
> +                               struct page **pages);
> +void page_pool_mp_release_area(struct page_pool *pool,
> +                               struct net_iov_area *area);
> +
> +#endif
> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> index 9a675e16e6a4..8bd4a3c80726 100644
> --- a/net/core/page_pool.c
> +++ b/net/core/page_pool.c
> @@ -13,6 +13,7 @@
>
>  #include <net/netdev_rx_queue.h>
>  #include <net/page_pool/helpers.h>
> +#include <net/page_pool/memory_provider.h>
>  #include <net/xdp.h>
>
>  #include <linux/dma-direction.h>
> @@ -459,7 +460,8 @@ page_pool_dma_sync_for_device(const struct page_pool *pool,
>                 __page_pool_dma_sync_for_device(pool, netmem, dma_sync_size);
>  }
>
> -static bool page_pool_dma_map(struct page_pool *pool, netmem_ref netmem)
> +static bool page_pool_dma_map_page(struct page_pool *pool, netmem_ref netmem,
> +                                  struct page *page)

I have to say this is confusing for me. Passing in both the netmem and
the page is weird. The page is the one being mapped and the
netmem->dma_addr is the one being filled with the mapping.

Netmem is meant to be an abstraction over page. Passing both makes
little sense to me. The reason you're doing this is because the
io_uring memory provider is in a bit of a weird design IMO where the
memory comes in pages but it doesn't want to use paged-backed-netmem.
Instead it uses net_iov-backed-netmem and there is an out of band page
to be managed.

I think it would make sense to use paged-backed-netmem for your use
case, or at least I don't see why it wouldn't work. Memory providers
were designed to handle the hugepage usecase where the memory
allocated by the provider is pages. Is there a reason that doesn't
work for you as well?

If you really need to use net_iov-backed-netmem, can we put this
weirdness in the provider? I don't know that we want a generic-looking
dma_map function which is a bit confusing to take a netmem and a page.

>  {
>         dma_addr_t dma;
>
> @@ -468,7 +470,7 @@ static bool page_pool_dma_map(struct page_pool *pool, netmem_ref netmem)
>          * into page private data (i.e 32bit cpu with 64bit DMA caps)
>          * This mapping is kept for lifetime of page, until leaving pool.
>          */
> -       dma = dma_map_page_attrs(pool->p.dev, netmem_to_page(netmem), 0,
> +       dma = dma_map_page_attrs(pool->p.dev, page, 0,
>                                  (PAGE_SIZE << pool->p.order), pool->p.dma_dir,
>                                  DMA_ATTR_SKIP_CPU_SYNC |
>                                          DMA_ATTR_WEAK_ORDERING);
> @@ -490,6 +492,11 @@ static bool page_pool_dma_map(struct page_pool *pool, netmem_ref netmem)
>         return false;
>  }
>
> +static bool page_pool_dma_map(struct page_pool *pool, netmem_ref netmem)
> +{
> +       return page_pool_dma_map_page(pool, netmem, netmem_to_page(netmem));
> +}
> +
>  static struct page *__page_pool_alloc_page_order(struct page_pool *pool,
>                                                  gfp_t gfp)
>  {
> @@ -1154,3 +1161,55 @@ void page_pool_update_nid(struct page_pool *pool, int new_nid)
>         }
>  }
>  EXPORT_SYMBOL(page_pool_update_nid);
> +
> +static void page_pool_release_page_dma(struct page_pool *pool,
> +                                      netmem_ref netmem)
> +{
> +       __page_pool_release_page_dma(pool, netmem);
> +}
> +

Is this wrapper necessary? Do you wanna rename the original function
to remove the __ instead of a adding a wrapper?

> +int page_pool_mp_init_paged_area(struct page_pool *pool,
> +                                struct net_iov_area *area,
> +                                struct page **pages)
> +{
> +       struct net_iov *niov;
> +       netmem_ref netmem;
> +       int i, ret = 0;
> +
> +       if (!pool->dma_map)
> +               return -EOPNOTSUPP;
> +
> +       for (i = 0; i < area->num_niovs; i++) {
> +               niov = &area->niovs[i];
> +               netmem = net_iov_to_netmem(niov);
> +
> +               page_pool_set_pp_info(pool, netmem);
> +               if (!page_pool_dma_map_page(pool, netmem, pages[i])) {
> +                       ret = -EINVAL;
> +                       goto err_unmap_dma;
> +               }
> +       }
> +       return 0;
> +
> +err_unmap_dma:
> +       while (i--) {
> +               netmem = net_iov_to_netmem(&area->niovs[i]);
> +               page_pool_release_page_dma(pool, netmem);
> +       }
> +       return ret;
> +}
> +
> +void page_pool_mp_release_area(struct page_pool *pool,
> +                              struct net_iov_area *area)
> +{
> +       int i;
> +
> +       if (!pool->dma_map)
> +               return;
> +
> +       for (i = 0; i < area->num_niovs; i++) {
> +               struct net_iov *niov = &area->niovs[i];
> +
> +               page_pool_release_page_dma(pool, net_iov_to_netmem(niov));
> +       }
> +}

Similarly I these 2 functions belong in the provider to be honest.


-- 
Thanks,
Mina