Messages in this thread | | | Date | Wed, 3 Apr 2024 11:21:26 +0200 | Subject | Re: [PATCH net-next 1/2] page_pool: check for PP direct cache locality later | From | Alexander Lobakin <> |
| |
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
| |