On Sun, Jul 20, 2025 at 10:49 PM Byungchul Park <byungchul@xxxxxx> wrote: > > Hi, > > I focused on converting the existing APIs accessing ->pp_magic field to > page type APIs. However, yes. Additional works would better be > considered on top like: > > 1. Adjust how to store and retrieve dma index. Maybe network guys > can work better on top. > > 2. Move the sanity check for page pool in mm/page_alloc.c to on free. > > Byungchul > > ---8<--- > From 7d207a1b3e9f4ff2a72f5b54b09e3ed0c4aaaca3 Mon Sep 17 00:00:00 2001 > From: Byungchul Park <byungchul@xxxxxx> > Date: Mon, 21 Jul 2025 14:05:20 +0900 > Subject: [PATCH] mm, page_pool: introduce a new page type for page pool in page type > > ->pp_magic field in struct page is current used to identify if a page > belongs to a page pool. However, page type e.i. PGTY_netpp can be used > for that purpose. > > Use the page type APIs e.g. PageNetpp(), __SetPageNetpp(), and > __ClearPageNetpp() instead, and remove the existing APIs accessing > ->pp_magic e.g. page_pool_page_is_pp(), netmem_or_pp_magic(), and > netmem_clear_pp_magic() since they are totally replaced. > > This work was inspired by the following link by Pavel: > > [1] https://lore.kernel.org/all/582f41c0-2742-4400-9c81-0d46bf4e8314@xxxxxxxxx/ > > Suggested-by: Pavel Begunkov <asml.silence@xxxxxxxxx> > Signed-off-by: Byungchul Park <byungchul@xxxxxx> > --- > .../net/ethernet/mellanox/mlx5/core/en/xdp.c | 2 +- > include/linux/mm.h | 28 ++----------------- > include/linux/page-flags.h | 6 ++++ > include/net/netmem.h | 2 +- > mm/page_alloc.c | 4 +-- > net/core/netmem_priv.h | 16 ++--------- > net/core/page_pool.c | 10 +++++-- > 7 files changed, 24 insertions(+), 44 deletions(-) > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c b/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c > index 5d51600935a6..def274f5c1ca 100644 > --- a/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c > @@ -707,7 +707,7 @@ static void mlx5e_free_xdpsq_desc(struct mlx5e_xdpsq *sq, > xdpi = mlx5e_xdpi_fifo_pop(xdpi_fifo); > page = xdpi.page.page; > > - /* No need to check page_pool_page_is_pp() as we > + /* No need to check PageNetpp() as we > * know this is a page_pool page. > */ > page_pool_recycle_direct(pp_page_to_nmdesc(page)->pp, > diff --git a/include/linux/mm.h b/include/linux/mm.h > index ae50c1641bed..736061749535 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -4135,10 +4135,9 @@ int arch_lock_shadow_stack_status(struct task_struct *t, unsigned long status); > * DMA mapping IDs for page_pool > * > * When DMA-mapping a page, page_pool allocates an ID (from an xarray) and > - * stashes it in the upper bits of page->pp_magic. We always want to be able to > - * unambiguously identify page pool pages (using page_pool_page_is_pp()). Non-PP > - * pages can have arbitrary kernel pointers stored in the same field as pp_magic > - * (since it overlaps with page->lru.next), so we must ensure that we cannot > + * stashes it in the upper bits of page->pp_magic. Non-PP pages can have > + * arbitrary kernel pointers stored in the same field as pp_magic (since > + * it overlaps with page->lru.next), so we must ensure that we cannot > * mistake a valid kernel pointer with any of the values we write into this > * field. > * > @@ -4168,25 +4167,4 @@ int arch_lock_shadow_stack_status(struct task_struct *t, unsigned long status); > > #define PP_DMA_INDEX_MASK GENMASK(PP_DMA_INDEX_BITS + PP_DMA_INDEX_SHIFT - 1, \ > PP_DMA_INDEX_SHIFT) > - > -/* Mask used for checking in page_pool_page_is_pp() below. page->pp_magic is > - * OR'ed with PP_SIGNATURE after the allocation in order to preserve bit 0 for > - * the head page of compound page and bit 1 for pfmemalloc page, as well as the > - * bits used for the DMA index. page_is_pfmemalloc() is checked in > - * __page_pool_put_page() to avoid recycling the pfmemalloc page. > - */ > -#define PP_MAGIC_MASK ~(PP_DMA_INDEX_MASK | 0x3UL) > - > -#ifdef CONFIG_PAGE_POOL > -static inline bool page_pool_page_is_pp(const struct page *page) > -{ > - return (page->pp_magic & PP_MAGIC_MASK) == PP_SIGNATURE; > -} > -#else > -static inline bool page_pool_page_is_pp(const struct page *page) > -{ > - return false; > -} > -#endif > - > #endif /* _LINUX_MM_H */ > diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h > index 4fe5ee67535b..906ba7c9e372 100644 > --- a/include/linux/page-flags.h > +++ b/include/linux/page-flags.h > @@ -957,6 +957,7 @@ enum pagetype { > PGTY_zsmalloc = 0xf6, > PGTY_unaccepted = 0xf7, > PGTY_large_kmalloc = 0xf8, > + PGTY_netpp = 0xf9, > > PGTY_mapcount_underflow = 0xff > }; > @@ -1101,6 +1102,11 @@ PAGE_TYPE_OPS(Zsmalloc, zsmalloc, zsmalloc) > PAGE_TYPE_OPS(Unaccepted, unaccepted, unaccepted) > FOLIO_TYPE_OPS(large_kmalloc, large_kmalloc) > > +/* > + * Marks page_pool allocated pages. > + */ > +PAGE_TYPE_OPS(Netpp, netpp, netpp) > + > /** > * PageHuge - Determine if the page belongs to hugetlbfs > * @page: The page to test. > diff --git a/include/net/netmem.h b/include/net/netmem.h > index f7dacc9e75fd..3667334e16e7 100644 > --- a/include/net/netmem.h > +++ b/include/net/netmem.h > @@ -298,7 +298,7 @@ static inline struct net_iov *__netmem_clear_lsb(netmem_ref netmem) > */ > #define pp_page_to_nmdesc(p) \ > ({ \ > - DEBUG_NET_WARN_ON_ONCE(!page_pool_page_is_pp(p)); \ > + DEBUG_NET_WARN_ON_ONCE(!PageNetpp(p)); \ > __pp_page_to_nmdesc(p); \ > }) > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index 2ef3c07266b3..71c7666e48a9 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -898,7 +898,7 @@ static inline bool page_expected_state(struct page *page, > #ifdef CONFIG_MEMCG > page->memcg_data | > #endif > - page_pool_page_is_pp(page) | > + PageNetpp(page) | > (page->flags & check_flags))) > return false; > > @@ -925,7 +925,7 @@ static const char *page_bad_reason(struct page *page, unsigned long flags) > if (unlikely(page->memcg_data)) > bad_reason = "page still charged to cgroup"; > #endif > - if (unlikely(page_pool_page_is_pp(page))) > + if (unlikely(PageNetpp(page))) > bad_reason = "page_pool leak"; > return bad_reason; > } > diff --git a/net/core/netmem_priv.h b/net/core/netmem_priv.h > index cd95394399b4..39a97703d9ed 100644 > --- a/net/core/netmem_priv.h > +++ b/net/core/netmem_priv.h > @@ -8,21 +8,11 @@ static inline unsigned long netmem_get_pp_magic(netmem_ref netmem) > return __netmem_clear_lsb(netmem)->pp_magic & ~PP_DMA_INDEX_MASK; > } > > -static inline void netmem_or_pp_magic(netmem_ref netmem, unsigned long pp_magic) > -{ > - __netmem_clear_lsb(netmem)->pp_magic |= pp_magic; > -} > - > -static inline void netmem_clear_pp_magic(netmem_ref netmem) > -{ > - WARN_ON_ONCE(__netmem_clear_lsb(netmem)->pp_magic & PP_DMA_INDEX_MASK); > - > - __netmem_clear_lsb(netmem)->pp_magic = 0; > -} > - > static inline bool netmem_is_pp(netmem_ref netmem) > { > - return (netmem_get_pp_magic(netmem) & PP_MAGIC_MASK) == PP_SIGNATURE; > + if (netmem_is_net_iov(netmem)) > + return true; As Pavel alludes, this is dubious, and at least it's difficult to reason about it. There could be net_iovs that are not attached to pp, and should not be treated as pp memory. These are in the devmem (and future net_iov) tx paths. We need a way to tell if a net_iov is pp or not. A couple of options: 1. We could have it such that if net_iov->pp is set, then the netmem_is_pp == true, otherwise false. 2. We could implement a page-flags equivalent for net_iov. Option #1 is simpler and is my preferred. To do that properly, you need to: 1. Make sure everywhere net_iovs are allocated that pp=NULL in the non-pp case and pp=non NULL in the pp case. those callsites are net_devmem_bind_dmabuf (devmem rx & tx path), io_zcrx_create_area (io_uring rx path). 2. Change netmem_is_pp to check net_iov->pp in the net_iov case. -- Thanks, Mina
