Re: [PATCH net-next 1/2] page_pool: check for PP direct cache locality later

[Alexander Lobakin <aleksander.lobakin@intel.com>]

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