"Zi Yan" <ziy@xxxxxxxxxx> writes:
> On Fri Apr 4, 2025 at 6:18 AM EDT, Toke Høiland-Jørgensen wrote:
>> Since we are about to stash some more information into the pp_magic
>> field, let's move the magic signature checks into a pair of helper
>> functions so it can be changed in one place.
>>
>> Reviewed-by: Mina Almasry <almasrymina@xxxxxxxxxx>
>> Tested-by: Yonglong Liu <liuyonglong@xxxxxxxxxx>
>> Acked-by: Jesper Dangaard Brouer <hawk@xxxxxxxxxx>
>> Reviewed-by: Ilias Apalodimas <ilias.apalodimas@xxxxxxxxxx>
>> Signed-off-by: Toke Høiland-Jørgensen <toke@xxxxxxxxxx>
>> ---
>> drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c | 4 ++--
>> include/net/page_pool/types.h | 18 ++++++++++++++++++
>> mm/page_alloc.c | 9 +++------
>> net/core/netmem_priv.h | 5 +++++
>> net/core/skbuff.c | 16 ++--------------
>> net/core/xdp.c | 4 ++--
>> 6 files changed, 32 insertions(+), 24 deletions(-)
>>
>
> <snip>
>
>> diff --git a/include/net/page_pool/types.h b/include/net/page_pool/types.h
>> index 36eb57d73abc6cfc601e700ca08be20fb8281055..df0d3c1608929605224feb26173135ff37951ef8 100644
>> --- a/include/net/page_pool/types.h
>> +++ b/include/net/page_pool/types.h
>> @@ -54,6 +54,14 @@ struct pp_alloc_cache {
>> netmem_ref cache[PP_ALLOC_CACHE_SIZE];
>> };
>>
>> +/* 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.
>> + * page_is_pfmemalloc() is checked in __page_pool_put_page() to avoid recycling
>> + * the pfmemalloc page.
>> + */
>> +#define PP_MAGIC_MASK ~0x3UL
>> +
>> /**
>> * struct page_pool_params - page pool parameters
>> * @fast: params accessed frequently on hotpath
>> @@ -264,6 +272,11 @@ void page_pool_destroy(struct page_pool *pool);
>> void page_pool_use_xdp_mem(struct page_pool *pool, void (*disconnect)(void *),
>> const struct xdp_mem_info *mem);
>> void page_pool_put_netmem_bulk(netmem_ref *data, u32 count);
>> +
>> +static inline bool page_pool_page_is_pp(struct page *page)
>> +{
>> + return (page->pp_magic & PP_MAGIC_MASK) == PP_SIGNATURE;
>> +}
>> #else
>> static inline void page_pool_destroy(struct page_pool *pool)
>> {
>> @@ -278,6 +291,11 @@ static inline void page_pool_use_xdp_mem(struct page_pool *pool,
>> static inline void page_pool_put_netmem_bulk(netmem_ref *data, u32 count)
>> {
>> }
>> +
>> +static inline bool page_pool_page_is_pp(struct page *page)
>> +{
>> + return false;
>> +}
>> #endif
>>
>> void page_pool_put_unrefed_netmem(struct page_pool *pool, netmem_ref netmem,
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index f51aa6051a99867d2d7d8c70aa7c30e523629951..347a3cc2c188f4a9ced85e0d198947be7c503526 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -55,6 +55,7 @@
>> #include <linux/delayacct.h>
>> #include <linux/cacheinfo.h>
>> #include <linux/pgalloc_tag.h>
>> +#include <net/page_pool/types.h>
>> #include <asm/div64.h>
>> #include "internal.h"
>> #include "shuffle.h"
>> @@ -897,9 +898,7 @@ static inline bool page_expected_state(struct page *page,
>> #ifdef CONFIG_MEMCG
>> page->memcg_data |
>> #endif
>> -#ifdef CONFIG_PAGE_POOL
>> - ((page->pp_magic & ~0x3UL) == PP_SIGNATURE) |
>> -#endif
>> + page_pool_page_is_pp(page) |
>> (page->flags & check_flags)))
>> return false;
>>
>> @@ -926,10 +925,8 @@ 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
>> -#ifdef CONFIG_PAGE_POOL
>> - if (unlikely((page->pp_magic & ~0x3UL) == PP_SIGNATURE))
>> + if (unlikely(page_pool_page_is_pp(page)))
>> bad_reason = "page_pool leak";
>> -#endif
>> return bad_reason;
>> }
>>
>
> I wonder if it is OK to make page allocation depend on page_pool from
> net/page_pool.
Why? It's not really a dependency, just a header include with a static
inline function...
> Would linux/mm.h be a better place for page_pool_page_is_pp()?
That would require moving all the definitions introduced in patch 2,
which I don't think is appropriate.
-Toke
