Received-SPF: pass (google.com: domain of linux-kernel+bounces-129411-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <cb05bde3-ee23-4e52-9df2-07b4c247d60c@intel.com>
Date: Wed, 3 Apr 2024 11:21:26 +0200
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH net-next 1/2] page_pool: check for PP direct cache
 locality later
To: Yunsheng Lin <linyunsheng@huawei.com>
CC: "David S. Miller" <davem@davemloft.net>, Eric Dumazet
	<edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni
	<pabeni@redhat.com>, Lorenzo Bianconi <lorenzo@kernel.org>,
	=?UTF-8?Q?Toke_H=C3=B8iland-J=C3=B8rgensen?= <toke@redhat.com>,
	<nex.sw.ncis.osdt.itp.upstreaming@intel.com>, <netdev@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>
References: <20240329165507.3240110-1-aleksander.lobakin@intel.com>
 <20240329165507.3240110-2-aleksander.lobakin@intel.com>
 <5699d031-d6d2-9cae-7025-2589eb60c0a0@huawei.com>
From: Alexander Lobakin <aleksander.lobakin@intel.com>
Content-Language: en-US
In-Reply-To: <5699d031-d6d2-9cae-7025-2589eb60c0a0@huawei.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?V0FEbEVCamNjVEx0WVIxNzFsVDB6ZnUwUHdoREp4YXV4Z1pwVks1ZW9RWWNp?=
 =?utf-8?B?TkYrZGlwK1dDSXZCeDhnK09Na1c5NFhtV2pQUHltV2VyaEJ0MElxUFJISStY?=
 =?utf-8?B?d1dqUGpETUZpRzRqU3QxeGdJSElyeUdWMkczRWxjK0VJQ281OEtZTFlDT0hu?=
 =?utf-8?B?MHdvT1BNTDR5QlpOTnV2VlVNY0FpZnp2UzhvNmpWVnpwTitDeU9QcnlLd0pv?=
 =?utf-8?B?VlhMWms3T2YvYk1NdDA1UUtnZ0tEUVVlS2g4Q3lZTGJhbXVFam94WENiQm92?=
 =?utf-8?B?TVhLcHhMcWYyWGhzYmVsZTFNaG5RdEpPVVZqdlkyUmxlZ3dwcHoxdnBRanZJ?=
 =?utf-8?B?Nmp6OFpvTEVKMzRiSDkwVXkvREF6TWwrNjRuMVYrMnRTWEZyQzAyeUJlN1dG?=
 =?utf-8?B?RkJoczRVK0VydmVUYVpXVzRQeE4wQmpZd3BMWUY3UXdxbWpzKys3SzlGMWtJ?=
 =?utf-8?B?Qmdteno5YkF5WlVpWEpEU1JOMkJTbUdMdzZWZEJrRGFqNzJ4UndJNVBSc0k5?=
 =?utf-8?B?djMrakVYMWhmZkRuRXUyZ3Fqdk83VUJnbzlibG5tczJUTWJJTUNJdHpOVnpp?=
 =?utf-8?B?SURRTW0vTDBUSEMrdW1TTlY4RklpM3d4b0IzRGtzRWZYVENsa2pYV3M2aEh2?=
 =?utf-8?B?b3lqbGo4M0k4VURIYUVPNDFoOWdOQUtic1Y0K01KMFhWZVhiUDlnTTBPbGI3?=
 =?utf-8?B?OXI4ZHEvSTVHNEZLNUttNXVORHlSSGtZU3g1WnVzekRocjArbUs2dk04Y1d5?=
 =?utf-8?B?Zm8xTkdCVGVoSG9wVXJPdVVBV29rT0wwVldqNWppZExHcHVnbFAzd3FLS2cw?=
 =?utf-8?B?M09pOWdUSnd3S0ZrOFplU3lneWhoQ2IyK21ITk54Nnl2aWoxZ29iVDIveklU?=
 =?utf-8?B?eXV1RUxjQi9FU2N4REpFdGJ4L3hhdHlkV3crY2wyZjhSTDBmV1ZWWFNzWjFm?=
 =?utf-8?B?WXdJV1JORVIvUzRwZ0pRT3ZlaTJaakdPNy8yc25jTUszSGx3cjh5NEZOaHps?=
 =?utf-8?B?Ylc1RjhNNmVCUnI2Z3JnUlJxS1kxekJrMTdhNFJyMXEyNnhWQ245eW9yS0da?=
 =?utf-8?B?T2Y4TTVWajZZMDRHQ2VGT3o4NzRtYUhXanRYeDZpWERkbDAzRjNZRzBCUFdv?=
 =?utf-8?B?SHJqQTJ5eXVOV2p3dW5ZYTdMY3kvei9jVjZXaWkxcFYrNGFGS0dSZEZObXVD?=
 =?utf-8?B?elhaVVZ4bFdmeXhUYzR4ZFJqTzBIdVhDVW9jQ0pCU2ZiUWVwRldMZzl5NUhz?=
 =?utf-8?B?V09FdUI4dXV2MnduMVRoTTh3Si9Rd2I3OG5QOHlQdU9jc1YzZjdvOEVNT2I3?=
 =?utf-8?B?WWJCL1ZmN3YweWlkelJnRGM1MmJKMHFSZSs0UHdnSC9hTXQ5eGo0elZtdy82?=
 =?utf-8?B?QTIxZE94SmpObG4yTDl5SVBvZEpnbG9OYzF3SHRYTjdiWjhxT2w3VUJ3ZkNZ?=
 =?utf-8?B?MVZSK01zZis2eDBRTVM1bFQ3dHYxM21SNEtpZWJCN0ErZzlPSlN4OHU1WlJI?=
 =?utf-8?B?NUtzYytOV3ZLVWM4UWd2N1A4RFRlOU4wWWVFVUgzYlZuNE1xV2hsVytlM2hn?=
 =?utf-8?B?d0NPM0pzUkt2dzNDSHFnRzhmcXF5eDZZbkxLcU0wc1pxS29Ld09vVDFWTDhO?=
 =?utf-8?B?NG5oRnZhQWdSM1lqSUYwbHppM1VaYnQxVkhqNkthYzkzVWJKSFNMWHkyME9X?=
 =?utf-8?B?aDVzUGx5RVhLSk8wQlhhUnZsK1FNTUhEZTNibkR0eGpjOHJqaEVYS1drMXNZ?=
 =?utf-8?B?bXdlZ0Y1cUc0b3l1aVJtWU12LzRVMVVxZjZkckdTQWVIeXVjUEwvS0czTmZH?=
 =?utf-8?B?L1c0WnA0UkRSWmR3TS9RTWhNMzNrTlZxbVZMUkcyTmFsN1hkaGE4ZHc2TXlq?=
 =?utf-8?B?ZGNkOWdBK0xtaEwyS1h5WU5pMmprVUZwa1k0bUpxbWdCMGcxZVlhR3lCb2k5?=
 =?utf-8?B?WDdWOUpCbzE4ZjNzS2RBQmtVczdUWUVIdmd0QjJ3bGk0VjFxZE0zSnJDY2wv?=
 =?utf-8?B?SWdzZ3VlRzliRXJqQmo2K2IzTDhSZXNRQ0dHUW5ySTBxcDg2czRUb3dQSTVy?=
 =?utf-8?B?ZXcvdVg2OEFreWdxMUdCdWpPVGJkbXNPRlBPOHdTeGVEQlBsWEs1QlltajFF?=
 =?utf-8?B?TVpnOTc4d1d0V09yWDNDQ1MvYnVvWmJyUnJMT3FzMzIxSGRqNUlOS1Z1M1Fu?=
 =?utf-8?B?anc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 01e69200-922c-4d99-f095-08dc53bf9a30
X-MS-Exchange-CrossTenant-AuthSource: DS0PR11MB8718.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 Apr 2024 09:22:37.2592
 (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: viaDArQUcKug5esfDPPydMc0wUeRztxFTdu3leNUGkE/9xFci4rjbnyTHffzQ2vpawVxy+dwMEDe27fJXD0H4iJNFSe0sfUCNokPnKCfUfU=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR11MB7123
X-OriginatorOrg: intel.com

From: Yunsheng Lin <linyunsheng@huawei.com>
Date: Sat, 30 Mar 2024 20:41:24 +0800

> On 2024/3/30 0:55, Alexander Lobakin wrote:
>> Since we have pool->p.napi (Jakub) and pool->cpuid (Lorenzo) to check
>> whether it's safe to use direct recycling, we can use both globally for
>> each page instead of relying solely on @allow_direct argument.
>> Let's assume that @allow_direct means "I'm sure it's local, don't waste
>> time rechecking this" and when it's false, try the mentioned params to
>> still recycle the page directly. If neither is true, we'll lose some
>> CPU cycles, but then it surely won't be hotpath. On the other hand,
>> paths where it's possible to use direct cache, but not possible to
>> safely set @allow_direct, will benefit from this move.
>> The whole propagation of @napi_safe through a dozen of skb freeing
>> functions can now go away, which saves us some stack space.

[...]

>>  void page_pool_put_unrefed_page(struct page_pool *pool, struct page *page,
>>  				unsigned int dma_sync_size, bool allow_direct)
>>  {
>> +	if (!allow_direct)
> 
> It seems we are changing some semantics here, in_softirq() is checked
> even if allow_direct is true before this patch. And it seems in_softirq()
> is not checked if allow_direct is true after this patch? I think we might
> need some assertion to ensure @allow_direct really means "I'm sure it's
> local, don't waste time rechecking this". As my understanding, it is really
> hard to debug this kind of problem, so in_softirq() is always checking.

It's implied that setting @allow_direct to true means "we're certainly
able to do that, we're certainly in the softirq context". I haven't seen
any code which would set it to true outside of softirq context and it's
counter-intuitive TBH.

> 
> Perhaps add something like WARN_ONCE() or DEBUG_NET_WARN_ON_ONCE for
> allow_direct being true case to catch the API misuse?
> 
>> +		allow_direct = page_pool_napi_local(pool);
>> +
>>  	page = __page_pool_put_page(pool, page, dma_sync_size, allow_direct);
>>  	if (page && !page_pool_recycle_in_ring(pool, page)) {
>>  		/* Cache full, fallback to free pages */
>> @@ -969,7 +994,7 @@ void page_pool_use_xdp_mem(struct page_pool *pool, void (*disconnect)(void *),
>>  static void page_pool_disable_direct_recycling(struct page_pool *pool)
>>  {
>>  	/* Disable direct recycling based on pool->cpuid.
>> -	 * Paired with READ_ONCE() in napi_pp_put_page().
>> +	 * Paired with READ_ONCE() in page_pool_napi_local().
>>  	 */
>>  	WRITE_ONCE(pool->cpuid, -1);
>>  
> 

Thanks,
Olek