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

[Jesper Dangaard Brouer <hawk@kernel.org>]

On 07/04/2025 16.15, Zi Yan wrote:
> On 7 Apr 2025, at 9:36, Zi Yan wrote:
> 
>> On 7 Apr 2025, at 9:14, Toke Høiland-Jørgensen wrote:
>>
>>> 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/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.
>> OK. Just checked. Yes, the function depends on PP_MAGIC_MASK.
>>
>> But net/types.h has a lot of unrelated page_pool functions and data structures
>> mm/page_alloc.c does not care about. Is there a way of moving page_pool_page_is_pp()
>> and its dependency to a separate header and including that in mm/page_alloc.c?
>>
>> Looking at the use of page_pool_page_is_pp() in mm/page_alloc.c, it seems to be
>> just error checking. Why can't page_pool do the error checking?
 >
> Or just remove page_pool_page_is_pp() in mm/page_alloc.c. Has it really been used?

We have actually used this at Cloudflare to catch some page_pool bugs.
And this have been backported to our 6.1 and 6.6 kernels and we have
enabled needed config CONFIG_DEBUG_VM (which we measured have low enough
overhead to enable in production).  AFAIK this is also enabled for our
6.12 kernels.

--Jesper