netdev - Re: [PATCH net-next v7 1/2] page_pool: Move pp

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <66692A4A-5747-447A-A1E6-678EBB9A33E0@nvidia.com>
Date: Mon, 07 Apr 2025 12:06:47 -0400
From: Zi Yan <ziy@...dia.com>
To: Toke Høiland-Jørgensen <toke@...hat.com>
Cc: Jesper Dangaard Brouer <hawk@...nel.org>,
 "David S. Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>,
 Saeed Mahameed <saeedm@...dia.com>, Leon Romanovsky <leon@...nel.org>,
 Tariq Toukan <tariqt@...dia.com>, Andrew Lunn <andrew+netdev@...n.ch>,
 Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>,
 Ilias Apalodimas <ilias.apalodimas@...aro.org>,
 Simon Horman <horms@...nel.org>, Andrew Morton <akpm@...ux-foundation.org>,
 Mina Almasry <almasrymina@...gle.com>, Yonglong Liu <liuyonglong@...wei.com>,
 Yunsheng Lin <linyunsheng@...wei.com>,
 Pavel Begunkov <asml.silence@...il.com>,
 Matthew Wilcox <willy@...radead.org>, netdev@...r.kernel.org,
 bpf@...r.kernel.org, linux-rdma@...r.kernel.org, linux-mm@...ck.org,
 kernel-team <kernel-team@...udflare.com>
Subject: Re: [PATCH net-next v7 1/2] page_pool: Move pp_magic check into
 helper functions

On 7 Apr 2025, at 12:05, Toke Høiland-Jørgensen wrote:

> Zi Yan <ziy@...dia.com> writes:
>
>> On 7 Apr 2025, at 10:43, Jesper Dangaard Brouer wrote:
>>
>>> 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@...dia.com>  writes:
>>>>>>
>>>>>>> Resend to fix my signature.
>>>>>>>
>>>>>>> On 7 Apr 2025, at 4:53, Toke Høiland-Jørgensen wrote:
>>>>>>>
>>>>>>>> "Zi Yan"<ziy@...dia.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@...gle.com>
>>>>>>>>>> Tested-by: Yonglong Liu<liuyonglong@...wei.com>
>>>>>>>>>> Acked-by: Jesper Dangaard Brouer<hawk@...nel.org>
>>>>>>>>>> Reviewed-by: Ilias Apalodimas<ilias.apalodimas@...aro.org>
>>>>>>>>>> Signed-off-by: Toke Høiland-Jørgensen<toke@...hat.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.
>>
>> The patch at the bottom moves page_pool_page_is_pp() to mm.h and compiles.
>> The macros and the function use mm’s page->pp_magic, so I am not sure
>> why it is appropriate, especially the user of the macros, net/core/page_pool.c,
>> has already included mm.h.
>
> Well, I kinda considered those details page_pool-internal. But okay, I
> can move them if you prefer to have them in mm.h.

Thanks.

Best Regards,
Yan, Zi