[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <cb05bde3-ee23-4e52-9df2-07b4c247d60c@intel.com>
Date: Wed, 3 Apr 2024 11:21:26 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Yunsheng Lin <linyunsheng@...wei.com>
CC: "David S. Miller" <davem@...emloft.net>, Eric Dumazet
<edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni
<pabeni@...hat.com>, Lorenzo Bianconi <lorenzo@...nel.org>,
Toke Høiland-Jørgensen <toke@...hat.com>,
<nex.sw.ncis.osdt.itp.upstreaming@...el.com>, <netdev@...r.kernel.org>,
<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net-next 1/2] page_pool: check for PP direct cache
locality later
From: Yunsheng Lin <linyunsheng@...wei.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
Powered by blists - more mailing lists