Received-SPF: pass (google.com: domain of linux-kernel+bounces-67008-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <15766d46-162c-4c99-84a0-7a4bc60c3289@intel.com>
Date: Thu, 15 Feb 2024 14:45:18 +0100
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH net-next] page_pool: disable direct recycling based on
 pool->cpuid on destroy
Content-Language: en-US
To: Lorenzo Bianconi <lorenzo@kernel.org>
CC: =?UTF-8?Q?Toke_H=C3=B8iland-J=C3=B8rgensen?= <toke@redhat.com>, "David S.
 Miller" <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, "Jakub
 Kicinski" <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	<netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>
References: <20240215113905.96817-1-aleksander.lobakin@intel.com>
 <87v86qc4qd.fsf@toke.dk> <8aa809c0-585f-4750-98d4-e19165c6ff73@intel.com>
 <Zc4Tht2TAkFOhq9Y@lore-desk>
From: Alexander Lobakin <aleksander.lobakin@intel.com>
In-Reply-To: <Zc4Tht2TAkFOhq9Y@lore-desk>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?cVR5UzBtNmtlaWVZaDNxMUFmZkxsSURuVm9aUUVIUUVBZjhPRDQ2b2ZDZWRO?=
 =?utf-8?B?VHR4MlZtdk9CUUZPRVZUZTd0TjhXNENTWG5Ia1NlaTAvZHMvanQzazl6MUdH?=
 =?utf-8?B?dWpWaU1udDZnZWppbGdaNHBrYzhkYW12d0dEUHcxNU9ZWGxObVhPbHpMZGRR?=
 =?utf-8?B?UlIzVy9qbTlvajE5TWs5SVM0VGJ3KzJHejcxQW5TancvWmpsek9reDU1SUlB?=
 =?utf-8?B?b0ZqUWljRmxwd0h4OHIzTVNJd2tPMmpERXhYZHp2NWt2UGtOdE51ZXg3amZa?=
 =?utf-8?B?RGYwZWcvZkw1SWl5MlVJbVp4WXk4NVRjd0kzUGorMS82WEpOY3BCSlVzR084?=
 =?utf-8?B?Y0w1ZW82aTJyd3dBOHR5TE1JTTZZZmhSaWp3R0dtSzkwNnpPVlBWZGFITFhr?=
 =?utf-8?B?RGtWaHZSU3MwVkh5Mjc5aUlDVFBtMGUvejJFOGFwQkVraTA1WTVXRVc1anJT?=
 =?utf-8?B?WlhsL3l6RjI2UmIzWHRaQU1vRXEvVXN4SVhER2RtNzNjcEc1RFhCYkJJK1o1?=
 =?utf-8?B?VmUvRmgyZUdQcUNGMThlWDFROGliSXA0NCtEa1RSZWRrOVNhYXlzR0hzRGZ2?=
 =?utf-8?B?SGJMZThxN0hLZjZwVUVEWkxURDlqV1JySGNVZEgyQmxETS9NL0JGN3hwa0Z6?=
 =?utf-8?B?czJlajlXTG5QWUVOdzZyRWNwZEJuZ0tUcEFDYXBNQjdXQXRqMVY1c29OeHA2?=
 =?utf-8?B?QVRPYm8xcFQrNzkvdklINUJZZ3VtS2JXRmpka0Vxa0JIaDJsREVpR2VuNkRZ?=
 =?utf-8?B?b3pJUlZUZmROSTFLd2N2TzNJUVpoN0o5ZkRNTjRyYVQvNUJqTFNIYWVoWEV6?=
 =?utf-8?B?cG1EMXpqVjNXQjNRcnI3MXdncVlocmRzZzdlNzQ0RWNSaFRjK011eFZ6US9w?=
 =?utf-8?B?UzIzMW1jRzN6bUxMRzFJYTdCMnIwV25yc0FSTThwcjhwRnVzUjgyV1BiYUhK?=
 =?utf-8?B?dzkzOFpzRjREcDNQY3Z3a2FkcU9FVUI4eGIvMWczV0plQUlicTdxaDFqeUF1?=
 =?utf-8?B?cGIyeU1EVzRhNzRlT1lNUEo1TnZTSnN3LzhZQkhVYXdXK0FnNVE2UnNiSzRo?=
 =?utf-8?B?UEJCbis5OE9BN242NkcrYUxicHBNbHdFTFJWbmw0dlhEYjh5UDNNRGd6TWRE?=
 =?utf-8?B?VjcreXU1TmxxUWFFalhVOVN3SWc5R0NrbkFFcU1LSzBTWnZaTDhHTzRpWndW?=
 =?utf-8?B?R2QvR2wzenBSZFNqYzE0SHgwQlI4a0dydG5DRWt3bm9lUldjd0tINmZPUmFw?=
 =?utf-8?B?dXBiZEhia1RVZHNOWWxmdVUyUVVFODkwU1hHMldHVzR6S1dMZEIvSUJWWHdZ?=
 =?utf-8?B?TnhJWU5wMENQRm41ZElwRm54MVBScG96K2FpdVcxVW5NQktWYU1seXJTbWZo?=
 =?utf-8?B?WE5WUk5TQWlqNUZ0R3BhVWlWczlZeFBsZVVOSyttQlJVQ1BTQ2xuQmpvUXRt?=
 =?utf-8?B?U2VhQW10Yzl5ZXNiT1FZaklzSDQ5bHFlRklwNDBZOFMxdUJ0b1pXTmNZN1Ro?=
 =?utf-8?B?a0dnSGNpeXA2c0tYTStLMXUvTGh4cVlyUlp3bE1KRG5aMUhoeFBzQk9TcVg0?=
 =?utf-8?B?dVBRWEh6Wld6M3Z0dWtDazFSRW5CSkFMclJlMjJyQUZZTUg5b0hCeHVXazRQ?=
 =?utf-8?B?eFhJV24vOWF2TnJPOUZlM0NPRFlsTlFRcjlzSnJEQmVrUEY4M29xZkVLS1lN?=
 =?utf-8?B?L3J0ZkdQeGovUE5iZ0tjN2wxRkFhMHZYYlpGUUVsS05LVTRiSCs3TjhLRGlP?=
 =?utf-8?B?MzhhZ2YzTWxJOHJSWW83TDBHTCt4TW1uZTQvZkhnUUxnVmhRcmRDMDZaMitF?=
 =?utf-8?B?MERrSEhEekt1YjA1cUNQVzhtdmdaZVpteEdzczZicFdMNVZna0xyK3hWWU1O?=
 =?utf-8?B?c2V4ZTVSbFhDTVl1K1FFaWEvQWNpd3IxenJiV1RSYzRGMkhtTEhPaVgyL2Ev?=
 =?utf-8?B?RGViZmM4blV4MldnRlkwakEzNWlCYnpsYVpOQ3o4N0FEUGZLQzVKZFBHMlFz?=
 =?utf-8?B?TVVsRnFKV2g4TTFLNDRSUkNTOHRuSDVOa0lRV1BsR3JJa2xwMHVqUzc4WUJJ?=
 =?utf-8?B?RGlCMmk0VmNKWDhxaWtWSzhtUUZ1SktkTk12NWJTL0t5VTNtSkppN296TXVk?=
 =?utf-8?B?dWh2YUtLTnIyYjd4ZHNVSE5PYi9FT3B0RDFKb0pYcUpIU21LRDNPUGMvdkNy?=
 =?utf-8?B?akE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 64491fd1-8ee8-4c7c-40e3-08dc2e2c5b40
X-MS-Exchange-CrossTenant-AuthSource: DS0PR11MB8718.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 Feb 2024 13:45:22.5390
 (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: P0HYzu6JGDOCEkzqCzEyTycnQUhUKx0aP8yBdmM6RZuCP/skAQ46U5jYaNESWbSu2oAvrbvW67S9Q56F44zFJMlTci0ix1AzFayDkLx7lAk=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH3PR11MB8434
X-OriginatorOrg: intel.com

From: Lorenzo Bianconi <lorenzo@kernel.org>
Date: Thu, 15 Feb 2024 14:37:10 +0100

>> From: Toke Høiland-Jørgensen <toke@redhat.com>
>> Date: Thu, 15 Feb 2024 13:05:30 +0100
>>
>>> Alexander Lobakin <aleksander.lobakin@intel.com> writes:
>>>
>>>> Now that direct recycling is performed basing on pool->cpuid when set,
>>>> memory leaks are possible:
>>>>
>>>> 1. A pool is destroyed.
>>>> 2. Alloc cache is emptied (it's done only once).
>>>> 3. pool->cpuid is still set.
>>>> 4. napi_pp_put_page() does direct recycling basing on pool->cpuid.
>>>> 5. Now alloc cache is not empty, but it won't ever be freed.
>>>
>>> Did you actually manage to trigger this? pool->cpuid is only set for the
>>> system page pool instance which is never destroyed; so this seems a very
>>> theoretical concern?
>>
>> To both Lorenzo and Toke:
>>
>> Yes, system page pools are never destroyed, but we might latter use
>> cpuid in non-persistent PPs. Then there will be memory leaks.
>> I was able to trigger this by creating bpf/test_run page_pools with the
>> cpuid set to test direct recycling of live frames.
> 
> what about avoiding the page to be destroyed int this case? I do not like the

I think I didn't get what you wanted to say here :s

Rewriting cpuid doesn't introduce any new checks on hotpath. Destroying
the pool is slowpath and we shouldn't hurt hotpath to handle it.

> idea of overwriting the cpuid field for it.

We also overwrite pp->p.napi field a couple lines below. It happens only
when destroying the pool, we don't care about the fields at this point.

> 
> Regards,
> Lorenzo
> 
>>
>>>
>>> I guess we could still do this in case we find other uses for setting
>>> the cpuid; I don't think the addition of the READ_ONCE() will have any
>>> measurable overhead on the common arches?
>>
>> READ_ONCE() is cheap, but I thought it's worth mentioning in the
>> commitmsg anyway :)
>>
>>>
>>> -Toke

Thanks,
Olek