Re: [PATCH net-next v7 1/2] page_pool: Move pp_magic check into helper functions

["Toke Høiland-Jørgensen" <toke@redhat.com>]

Zi Yan <ziy@nvidia.com> writes:

> Resend to fix my signature.
>
> On 7 Apr 2025, at 4:53, Toke Høiland-Jørgensen wrote:
>
>> "Zi Yan" <ziy@nvidia.com> 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@google.com>
>>>> Tested-by: Yonglong Liu <liuyonglong@huawei.com>
>>>> Acked-by: Jesper Dangaard Brouer <hawk@kernel.org>
>>>> Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>>>> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
>>>> ---
>>>>  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...
>
> The function is checking, not even modifying, an core mm data structure,
> struct page, which is also used by almost all subsystems. I do not get
> why the function is in net subsystem.

Well, because it's using details of the PP definitions, so keeping it
there nicely encapsulates things. I mean, that's the whole point of
defining a wrapper function - encapsulating the logic :)

>>> 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.
>
> Why? I do not see page_pool_page_is_pp() or PP_SIGNATURE is used anywhere
> in patch 2.

Look again. Patch 2 redefines PP_MAGIC_MASK in terms of all the other
definitions.

-Toke