Re: [PATCH net-next v12 06/13] page_pool: devmem support

Pavel Begunkov
2024-06-17T14:16:57Z

On 6/13/24 02:35, Mina Almasry wrote:
> Convert netmem to be a union of struct page and struct netmem. Overload
> the LSB of struct netmem* to indicate that it's a net_iov, otherwise
> it's a page.
>
> Currently these entries in struct page are rented by the page_pool and
> used exclusively by the net stack:
>
> struct {
> unsigned long pp_magic;
> struct page_pool *pp;
> unsigned long _pp_mapping_pad;
> unsigned long dma_addr;
> atomic_long_t pp_ref_count;
> };
>
> Mirror these (and only these) entries into struct net_iov and implement
> netmem helpers that can access these common fields regardless of
> whether the underlying type is page or net_iov.
>
> Implement checks for net_iov in netmem helpers which delegate to mm
> APIs, to ensure net_iov are never passed to the mm stack.
>
> Signed-off-by: Mina Almasry <almasrymina@google.com>

Apart from small comments below

Reviewed-by: Pavel Begunkov <asml.silence@gmail.com>

> ---
> include/net/netmem.h | 137 ++++++++++++++++++++++++++++++--
> include/net/page_pool/helpers.h | 25 +++---
> net/core/devmem.c | 3 +
> net/core/page_pool.c | 26 +++---
> net/core/skbuff.c | 22 +++--
> 5 files changed, 168 insertions(+), 45 deletions(-)
>
> diff --git a/include/net/netmem.h b/include/net/netmem.h
> index 664df8325ece5..35ad237fdf29e 100644
> --- a/include/net/netmem.h
> +++ b/include/net/netmem.h
...
> -/* Converting from page to netmem is always safe, because a page can always be
> - * a netmem.
> - */
> static inline netmem_ref page_to_netmem(struct page *page)
> {
> return (__force netmem_ref)page;
> @@ -68,17 +107,103 @@ static inline netmem_ref page_to_netmem(struct page *page)
>
> static inline int netmem_ref_count(netmem_ref netmem)
> {
> + /* The non-pp refcount of net_iov is always 1. On net_iov, we only
> + * support pp refcounting which uses the pp_ref_count field.
> + */
> + if (netmem_is_net_iov(netmem))
> + return 1;
> +
> return page_ref_count(netmem_to_page(netmem));
> }
>
> static inline unsigned long netmem_to_pfn(netmem_ref netmem)
> {
> + if (netmem_is_net_iov(netmem))
> + return 0;

IIRC 0 is a valid pfn. Not much of a concern since it's
used only for tracing, but might make sense to pass some
invalid pfn if there is one

> +
> return page_to_pfn(netmem_to_page(netmem));
> }
>
...
> static inline netmem_ref netmem_compound_head(netmem_ref netmem)
> {
> + /* niov are never compounded */
> + if (netmem_is_net_iov(netmem))
> + return netmem;
> +
> return page_to_netmem(compound_head(netmem_to_page(netmem)));
> }
>
> +static inline void *netmem_address(netmem_ref netmem)

I don't think it's used anywhere, do I miss it?

> +{
> + if (netmem_is_net_iov(netmem))
> + return NULL;
> +
> + return page_address(netmem_to_page(netmem));
> +}
> +
...
> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> index a5957d3359762..1152e3547795a 100644
> --- a/net/core/page_pool.c
> +++ b/net/core/page_pool.c
> @@ -26,6 +26,8 @@
...
>
> /* If the page refcnt == 1, this will try to recycle the page.
> @@ -714,7 +713,7 @@ __page_pool_put_page(struct page_pool *pool, netmem_ref netmem,
> * refcnt == 1 means page_pool owns page, and can recycle it.
> *
> * page is NOT reusable when allocated when system is under
> - * some pressure. (page_is_pfmemalloc)
> + * some pressure. (page_pool_page_is_pfmemalloc)

There is no page_pool_page_is_pfmemalloc()

> */
> if (likely(__page_pool_page_can_be_recycled(netmem))) {
> /* Read barrier done in page_ref_count / READ_ONCE */
> @@ -727,6 +726,7 @@ __page_pool_put_page(struct page_pool *pool, netmem_ref netmem,
> /* Page found as candidate for recycling */
> return netmem;
> }

--
Pavel Begunkov