From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 70598C36010 for ; Mon, 7 Apr 2025 14:44:01 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 32B566B0006; Mon, 7 Apr 2025 10:43:59 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 2D9C96B0007; Mon, 7 Apr 2025 10:43:59 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 1A2616B0008; Mon, 7 Apr 2025 10:43:59 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id F3FA66B0006 for ; Mon, 7 Apr 2025 10:43:58 -0400 (EDT) Received: from smtpin28.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay05.hostedemail.com (Postfix) with ESMTP id 6EC1459463 for ; Mon, 7 Apr 2025 14:44:00 +0000 (UTC) X-FDA: 83307517440.28.E8AAED6 Received: from nyc.source.kernel.org (nyc.source.kernel.org [147.75.193.91]) by imf03.hostedemail.com (Postfix) with ESMTP id A059520006 for ; Mon, 7 Apr 2025 14:43:58 +0000 (UTC) Authentication-Results: imf03.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=LpufQ4SM; spf=pass (imf03.hostedemail.com: domain of hawk@kernel.org designates 147.75.193.91 as permitted sender) smtp.mailfrom=hawk@kernel.org; dmarc=pass (policy=quarantine) header.from=kernel.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1744037038; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=PSSvOzkbXYcusDFf+xS40R3lhGAoccWqanLmzQ5H0dc=; b=WPZAGHH2z5GwSWHJFYXTFWRHEr5aEYRfdX/deDRBu7T0Ol2S3G11vMi3HG17Q422hMGZti evPXhbo+L7M1YGnJ5C8ADgn+pFGiiXV/hcdVVnPKORG6oVCo/tXsWOE7V8UALh/nUf10px TvVxntVObpdmS5EsBei77EoiAHPM3VY= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1744037038; a=rsa-sha256; cv=none; b=W8kFnY5Ro1MsFBRCwBeNTlEQDOgixyyNotPA/iAMgDpLmKzL70dPA6//5yEDsjYtxWCGq9 NoBe5air9wPT3WHdzg/3gn88h5R/qMe9KGqTo+uzu7pMr6KNjJJ8l6KxcKyibNsp0Xrsd9 PyBRh+xrJp/e72xrTUVOHvuVCCPfdow= ARC-Authentication-Results: i=1; imf03.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=LpufQ4SM; spf=pass (imf03.hostedemail.com: domain of hawk@kernel.org designates 147.75.193.91 as permitted sender) smtp.mailfrom=hawk@kernel.org; dmarc=pass (policy=quarantine) header.from=kernel.org Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by nyc.source.kernel.org (Postfix) with ESMTP id 40E0EA4214B; Mon, 7 Apr 2025 14:38:29 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id EC694C4CEE7; Mon, 7 Apr 2025 14:43:52 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1744037037; bh=qNa5SnpYvPxSF+sauTUnQwpMRyj/0mKfqIpcvOwWmbw=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=LpufQ4SMtmAw34AH1lg4HaJpmi5uB/ITZaRXFjPtqQ5Jplj8LEJjg+voE3cdUfG+S Aff6Q90IfGj5NjihzGJi/2j6vzVQWb9amgjl3th/e4b/1QRRgnT0ynXRmsTVPLHLBV Fc66kZZ6DNvwLPaLfw0gzaiqid4TaVavtDVxSS/LJm+Fva+m8nzggXgUXT4xLU18Yo WkoTUnJXITYzzQOMzRxNtcMyR6O3kRsa+s0KyueBzgKtHRGJcKCj92N/XX/X84bLQz uDsO/MHk8D+kEiQ2o2Sr6pD8B6aINN8DBRIfBq/ik5khBvl2w4ylI6AOJjb6CJidsg tqK4TWZbxsZOw== Message-ID: <4d35bda2-d032-49db-bb6e-b1d70f10d436@kernel.org> Date: Mon, 7 Apr 2025 16:43:50 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH net-next v7 1/2] page_pool: Move pp_magic check into helper functions To: Zi Yan , =?UTF-8?Q?Toke_H=C3=B8iland-J=C3=B8rgensen?= Cc: "David S. Miller" , Jakub Kicinski , Saeed Mahameed , Leon Romanovsky , Tariq Toukan , Andrew Lunn , Eric Dumazet , Paolo Abeni , Ilias Apalodimas , Simon Horman , Andrew Morton , Mina Almasry , Yonglong Liu , Yunsheng Lin , Pavel Begunkov , Matthew Wilcox , netdev@vger.kernel.org, bpf@vger.kernel.org, linux-rdma@vger.kernel.org, linux-mm@kvack.org, kernel-team References: <20250404-page-pool-track-dma-v7-0-ad34f069bc18@redhat.com> <20250404-page-pool-track-dma-v7-1-ad34f069bc18@redhat.com> <87cydoxsgs.fsf@toke.dk> <87v7rgw1us.fsf@toke.dk> <893B4BFD-1FDA-46DE-82D5-9E5CBDD90068@nvidia.com> Content-Language: en-US From: Jesper Dangaard Brouer In-Reply-To: <893B4BFD-1FDA-46DE-82D5-9E5CBDD90068@nvidia.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Stat-Signature: n1uijwgezfwwwbm8h7xcb5iy6ebxamuj X-Rspam-User: X-Rspamd-Queue-Id: A059520006 X-Rspamd-Server: rspam08 X-HE-Tag: 1744037038-949333 X-HE-Meta: U2FsdGVkX1/h1EfbMBv22I8gTO1qBSx4WXaGyuxvBYq/MFZl/gTn4SkGxteBEYXfGVvDMKetZ7QLK89zE1IeBFcu8scUKT4ZsZCcTP1ILjOR373knvOmU1aDdpKp0F8yWRTHz/FijnVuRmAfouC95wNxAQU8to8xk2WPxKA4w2TQf0Cz9ZmCbULxTOTy0oQlH+e7kjc9Jg1EwIwZ9Wj5dFaeJSKcc2QkcyUHeVJ0ol2az0qRfMgYTGl78eZAgTH9wsU7uBkP3PIfWC2iaPFcnD0Hchs+oOdFkJfST3QmCHjhenIj0bJTxwhsDMYW6+JmvcN8as3tS/8Wzmpo0HembYff6LhJprd7K8vKcUcDfVxGNT0rRmMZWC/zCf001xtZ7/6cLBZGYJAGixPlR5thyiL8p34ki+RwKs2PafrcHPyZa9inm7HFZnQGeC/X7XTxB5FVIqo8fitbON4owpAtIN3kJ3d7s3kda7RXQzcdVlF77btqilmVnKa5GcEKGLZgYKsbRyPH5OAcs5qdEf7HSrXvWrAG9ue1QEdG6kEbTr5OyxTPL3ZmppPIDCCsOFNqdyDDjSmByFROtjhf771aQJS38u5+azPyNLzirW55+nsDOtR3Snl2aPEdyHa7P1oOcWE1GiRdDEYVI3+Xo//fglT6C7IhQsHISMzJHKkYBI85JYzntghH5Hl/ipT2LdZwa8ROo9oAb3MiyjQPyvrZUI1d94+te75u56MS8olk+z01ROrjPzR06OCBBlOIAzkoJF6pNdr6UXtlpkvKurkOihitit7oU6zu3ekNznWbwl2OJqyu5pKxKZkqvvx279N/IloIym8hdUhCodf+5I+TsnccxsQ91c2oX7UYOIK62R0DRofb86oRHvPpqhOQsvr9TeLgH96LLSGRO38DSf88nCB980A6pAzBnEafKTcOgM0H0aWWbKBnJ/twBnb9E8gVe13zn7kTK+ezjiMZR+4 S41aWURB fzs/sXTAnMcP6RrEqgwI1mYZYeJ9dXrgOQhszeFOed39G5tKoa/O6QZJ+dLuSdT0kojuZ/M+vKPPHqI1pxhWYgbyf6Nt7wliOffkpFU3mTiB07VT0QjnjifOwMsfAwYSC8EwsOz4W0S6fe1jSelQPAsODkTIp6eIqCeFduw8jfKDwY1BMOiSzwNVqPYDqCUjaEY73TibrcWKpUuMUdrZMaUctWGDQAeWepBvhKL4Xkab8Z0h9/E2LrNJaA4SCt9ApCOKSzNGOzK2aHjjH6gi4M3QjSPSiMBMA4sq+t9e1Iwp/7P34aRy+zVfYM8lsz26nsr6+nEZAavBh5Rf/gzJc4nUQYYnMzsqEX3+hfZqJ95U78erbe2GBtHVwZyiTZxHc0tTz1WSpNIKm760A64oazPcOPDSTHvE1DZjt2YUlSh7CO4TybeIiOl1Yev/+Nw5VPkC6bLJmmVngP/oB2o+Ff//62GGQRWgDiKsQecPEToijDlCtQqKVMFvH+1wKirAJaMI+NnroNTSIF056du/4XgI9UEYzi9HxxlK2dvICHbqILaeyHtxpIbr7IlN9xXrPw/pwTu0xCJeuO3Q= X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: 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 writes: >>> >>>> Resend to fix my signature. >>>> >>>> On 7 Apr 2025, at 4:53, Toke Høiland-Jørgensen wrote: >>>> >>>>> "Zi Yan" 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 >>>>>>> Tested-by: Yonglong Liu >>>>>>> Acked-by: Jesper Dangaard Brouer >>>>>>> Reviewed-by: Ilias Apalodimas >>>>>>> Signed-off-by: Toke Høiland-Jørgensen >>>>>>> --- >>>>>>> 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(-) >>>>>>> >>>>>> [...] >>>>>>> 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 >>>>>>> #include >>>>>>> #include >>>>>>> +#include >>>>>>> #include >>>>>>> #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