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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <30c4b5c7-3c83-44f6-a469-f46e463635e5@kernel.org>
Date: Mon, 4 Aug 2025 16:17:46 +0200
From: Jesper Dangaard Brouer <hawk@...nel.org>
To: Stanislav Fomichev <stfomichev@...il.com>,
 Jakub Kicinski <kuba@...nel.org>
Cc: davem@...emloft.net, netdev@...r.kernel.org, edumazet@...gle.com,
 pabeni@...hat.com, andrew+netdev@...n.ch, horms@...nel.org,
 David Wei <dw@...idwei.uk>, michael.chan@...adcom.com,
 pavan.chebbi@...adcom.com, ilias.apalodimas@...aro.org,
 almasrymina@...gle.com, sdf@...ichev.me
Subject: Re: [PATCH net] net: page_pool: allow enabling recycling late, fix
 false positive warning



On 01/08/2025 23.36, Stanislav Fomichev wrote:
> On 08/01, Jakub Kicinski wrote:
>> On Fri, 1 Aug 2025 13:55:09 -0700 Stanislav Fomichev wrote:
>>>> +static void bnxt_enable_rx_page_pool(struct bnxt_rx_ring_info *rxr)
>>>> +{
>>>> +	page_pool_enable_direct_recycling(rxr->head_pool, &rxr->bnapi->napi);
>>>> +	page_pool_enable_direct_recycling(rxr->page_pool, &rxr->bnapi->napi);
>>>
>>> We do bnxt_separate_head_pool check for the disable_direct_recycling
>>> of head_pool. Is it safe to skip the check here because we always allocate two
>>> pps from queue_mgmt callbacks? (not clear for me from a quick glance at
>>> bnxt_alloc_rx_page_pool)
>>
>> It's safe (I hope) because the helper is duplicate-call-friendly:
>>
>> +void page_pool_enable_direct_recycling(struct page_pool *pool,
>> +				       struct napi_struct *napi)
>> +{
>> +	if (READ_ONCE(pool->p.napi) == napi)   <<< right here
>> +		return;
>> +	WARN_ON_ONCE(!napi || pool->p.napi);

Why only warn once?
(this is setup code path, so it should not get invoked a lot)

>> +
>> +	mutex_lock(&page_pools_lock);
>> +	WRITE_ONCE(pool->p.napi, napi);
>> +	mutex_unlock(&page_pools_lock);
>> +}
>>
>> We already have a refcount in page pool, I'm planning to add
>> page_pool_get() in net-next and remove the
>>
>> 	if (bnxt_separate_head_pool)
>>
>> before page_pool_destroy(), too.
> 
> Ah, I see, I missed that fact that page_pool and head_pool point to the
> same address when we don't have separate pools. Makes sense, thanks!

If you depend on this side-effect of the API, we better document that
this is the intend of the API.  E.g.
  Calling this function several time with same page_pool and napi is safe
and only enables it on the first call.  Other-wise it is no allowed to 
enable an already enabled page_pool.

(... that's what the WARN is about right?)

The bnxt driver code use-case for doing this seems scary and hard to
follow, e.g. having 'page_pool' and 'head_pool' point to the same
address, sometimes...  I would also add a comment about this in the
driver, but I find this optional and up-to the driver maintainer.

--Jesper

pw-bot: cr

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ