Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <6d79614b-1258-08be-1bd8-cb02ffd9dc6e@intel.com>
Date:   Fri, 30 Jun 2023 14:39:07 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.12.0
Subject: Re: [PATCH RFC net-next 1/4] net: skbuff: don't include
 <net/page_pool.h> to <linux/skbuff.h>
Content-Language: en-US
To:     Alexander H Duyck <alexander.duyck@gmail.com>
CC:     "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Maciej Fijalkowski <maciej.fijalkowski@intel.com>,
        Larysa Zaremba <larysa.zaremba@intel.com>,
        Yunsheng Lin <linyunsheng@huawei.com>,
        Alexander Duyck <alexanderduyck@fb.com>,
        "Jesper Dangaard Brouer" <hawk@kernel.org>,
        Ilias Apalodimas <ilias.apalodimas@linaro.org>,
        <netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>
References: <20230629152305.905962-1-aleksander.lobakin@intel.com>
 <20230629152305.905962-2-aleksander.lobakin@intel.com>
 <b7dda3fe88bb3d302b1cbb2016387d5e98e2f946.camel@gmail.com>
From:   Alexander Lobakin <aleksander.lobakin@intel.com>
In-Reply-To: <b7dda3fe88bb3d302b1cbb2016387d5e98e2f946.camel@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?cm5DeXZxR09jZEREWGw2clRsYmsxeXAza09McnF4WThvMHQvT3RKVE11Vzho?=
 =?utf-8?B?bE9EMHFsS3VCd3loekh1ZnJ2OXZxK0dVOG9naEpGTXZ6WjFUWlB4RlRhQlli?=
 =?utf-8?B?Q1k1Zmt1NHBGenluRDlMRDh2REFBNFZRVEFXaFNSYXN0NHBFODZGSHJvaVNl?=
 =?utf-8?B?N1JTMHErTFdIRE1qRS93b1E3M0psQ1kyLzIzRmdxVUlMVTZacE5YZmZ0SDVF?=
 =?utf-8?B?S0x0UHR0cnF3VEhkdFQ5SGVBMlY5eTVvQ0pNeGFzTEI5dzQwQkxJeXdkTDRp?=
 =?utf-8?B?d2NHdWRYZm10NnowZmtheTdiSkVuSnpvMXl4YnFsV1ZjdDNucEI3aW1jcmIy?=
 =?utf-8?B?NG16VHVmMExnaW5wbmp4SU5HbjNSa29QOFhpbVdqNTRYbEtrUkN6UGoxV0Vt?=
 =?utf-8?B?Vks5ZGtSNjZabCs3T1RzVFllUHBsL2NUeVJLQ1dabTViNXJjV0x2TkFCNVNG?=
 =?utf-8?B?VW00UGl1NWNuRDRNWVJvUjV2cmpOb1NlMTdHeUs1SGlIUzVJQlA2eDM5Yzdh?=
 =?utf-8?B?Q09aSHF2TUN4R01TS2xORTI2cERMSUNNaTVOSlhSV2ZVOGZNb0JPUmFjUTFC?=
 =?utf-8?B?cEE0SXpwVTMxSndrMUdsNzdid3JHRGRVT08zZlUvMGx0cFRCZWRnWkpaR05P?=
 =?utf-8?B?Rk5ERk1yK1BBMmhscUszREt6SDJwZlA5dmgzRjdYbm0xVFdHRmQ4ODZkS2gz?=
 =?utf-8?B?M3FhWkVIWUtCQ1kwUUhDemVleG1ObCtYd1hGcFRidkRRZ3daVUZiTm40amJs?=
 =?utf-8?B?Rkc1ZTBENEZaMFRra1kwWWUvK3o4MFNPR2RSYmNQRmVsaVhHNCtPRE8vWE5Z?=
 =?utf-8?B?QXU2Uk9Lcy9VSEV5bUFCeFpQUG03NWhESnNMQUpOOWp0bm1lWDFxbktZZWtx?=
 =?utf-8?B?RGVmWG1lcGI0WVJ6cGx1Zkl3MjhXTHZJZURTRjYzVnpLZXp6QStLN1RvL0pk?=
 =?utf-8?B?bjE2SGpabmxndTlScEFVMy95Q2lnb3hQRm9ETTFjMTN2NHh4a3lLd1pHWW55?=
 =?utf-8?B?dXBJYWFnSFloTHp5Q1JINktOL2srM0lTOHVxYnk4c3N3TlAxZ1ZUaWdNWlF6?=
 =?utf-8?B?OGJZa1QwTDcwald1dEFXaU10NWVTam9vd1RlME1LbWRtSmdlMFpUS0R0a211?=
 =?utf-8?B?ZTdxMmVIa3RDM3FaWk1IYUt2RndocEdLZmhCLzZiWnBUNFJxeU9KRzJpTmhL?=
 =?utf-8?B?bkxQSWZuNXREbmVCZEsxMnBvTEFSRzg2THRMTjVYWDlhZ09qUXo2SWNpWXVX?=
 =?utf-8?B?c2J3SlVBbnlpMytZMmpOVWFONkg0T1drVm8yNlU5UjN4ZzhteDZkZTFKc1hm?=
 =?utf-8?B?eVcwQmZ5S1Q2Z2NtWWFtZ3Y0emhvSUdiQUttazJveDAyQlE0eldrWFIvbEp1?=
 =?utf-8?B?UzZIYWUxcHNKcXk3QXdOcmt4dnRHOHRQL0dEVWdPanA3MTBxVDgzemVDSDA4?=
 =?utf-8?B?elFMQVJGcGdDSHNuRFFMdGFoYnJVdE12d2pFU2g1OUZoNTFMcTVGNDdDeFNJ?=
 =?utf-8?B?OWZ3ZVJERngrcURIamg1T2ZNaTJqNXU1Tkx5K1pJZHU4UWRlRGhmL3JIclVB?=
 =?utf-8?B?Y1dsdmJVWnRmRmdMRmpOVjUrMVU0TVhiclpkdWVpdWQyTExFMWRucVVWWndE?=
 =?utf-8?B?YlZQdkJndlY4a0t4U1dvWkorSWRvTFJwVGxiSWo0TjViWGRzUjVyWnNSN2hH?=
 =?utf-8?B?UFB3WlQ3WmlneXlHZlN3MGF5aUhkcVZKRlZpY3ZrWUNXUVVmczlYSEZaSkJD?=
 =?utf-8?B?b1o5ajJxTTlMUFVib2l6RksxYkFUdWhhYVFGUmRPNnNvOS93TDZpUGpjSlRG?=
 =?utf-8?B?RVRreG1nR0hJWmRpa29nNGw4d2UzMUNvM3lNd2pxVHNnc2NKZDkyKzcyV2Iy?=
 =?utf-8?B?SFBubFRyRDNSMGlPUFdHUEVkMzQ5ckExNkhLZ2xZVE9FRUdUUDhxZDRORW5k?=
 =?utf-8?B?SSsxbWY5MUdjNkRaKytqdi8yd2RUbklDNEROM3RTRmdvTEU1a2thOFdBcmRi?=
 =?utf-8?B?c0xXT05mQ1F5SUhqTWJOMWlscTV1cmVSeG5ieXJ1b1dkeDlMYWk5dGcvTjFP?=
 =?utf-8?B?WnhDTTU1K3RyaEFCUFNBZWg2WVlsUFpMSm5FcE16OGY5TWpRc013NmRTTnVT?=
 =?utf-8?B?RThKQmdGVi9NMzQ1SFplS3ZndnhIWTZ6Tzg2YTlnUHhtV1lIQ3VzVFo3RkQr?=
 =?utf-8?B?L1E9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 42ad393e-8c04-4c80-0f96-08db796714f8
X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB3625.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 30 Jun 2023 12:39:44.6709
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: dPz7i05A8B4A0veBhlMbuQhagSxS6KOufZAbltns0sV/mWkRwaJXxQhLgnZQYMSrB8pY6VE/kZqF50b5rzneZWVVaNT8JHNxUE3tCDcovDg=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR11MB4547
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.5 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_MED,SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE,
        URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

From: Alexander H Duyck <alexander.duyck@gmail.com>
Date: Thu, 29 Jun 2023 09:55:15 -0700

> On Thu, 2023-06-29 at 17:23 +0200, Alexander Lobakin wrote:
>> Currently, touching <net/page_pool.h> triggers a rebuild of more than
>> a half of the kernel. That's because it's included in <linux/skbuff.h>.
>> And each new include to page_pool.h adds more [useless] data for the
>> toolchain to process per each source file from that pile.

[...]

>> +bool page_pool_return_skb_page(struct page *page, bool napi_safe)
>> +{
>> +	struct napi_struct *napi;
>> +	struct page_pool *pp;
>> +	bool allow_direct;
>> +
>> +	page = compound_head(page);
>> +	pp = page->pp;
> 
> So this is just assuming that any page we pass thru is a page pool
> page. The problem is there may be some other pointer stored here that
> could cause issues.

But that is exactly what you suggested in the previous revision's
thread... Hey! :D

"I suspect we could look at pulling parts of it out as well. The
pp_magic check should always be succeeding unless we have pages getting
routed the wrong way somewhere. So maybe we should look at pulling it
out and moving it to another part of the path such as
__page_pool_put_page() and making it a bit more visible to catch those
cases".

Anyway, since some drivers still mix PP pages with non-PP ones (mt76
IIRC, maybe more), I feel the check for magic is still relevant. I just
believed you and forgot about that T.T

> 
> I would suggest creating an accessor as mentioned above to verify it is
> a page pool page before you access page->pp.
> 
>> +
>> +	/* Allow direct recycle if we have reasons to believe that we are
[...]

Thanks,
Olek