[<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