Re: [PATCH RFC net-next v2 7/7] net: skbuff: always try to recycle PP pages directly when in softirq

[Alexander Lobakin]

From: Jakub Kicinski <kuba@xxxxxxxxxx>
Date: Tue, 18 Jul 2023 17:40:42 -0700

> On Fri, 14 Jul 2023 19:08:52 +0200 Alexander Lobakin wrote:
>> Suggested-by: Jakub Kicinski <kuba@xxxxxxxxxx> # in_softirq()
> 
> I thought I said something along the lines as "if this is safe you can
> as well" which falls short of a suggestion, cause I don't think it is
> safe :)

Ok, I'll drop you if you insist :D

> 
>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
>> index fc1470aab5cf..1c22fd33be6c 100644
>> --- a/net/core/skbuff.c
>> +++ b/net/core/skbuff.c
>> @@ -902,7 +902,7 @@ bool page_pool_return_skb_page(struct page *page, bool napi_safe)
>>  	 * in the same context as the consumer would run, so there's
>>  	 * no possible race.
>>  	 */
>> -	if (napi_safe) {
>> +	if (napi_safe || in_softirq()) {
>>  		const struct napi_struct *napi = READ_ONCE(pp->p.napi);
>>  
>>  		allow_direct = napi &&
> 
> What if we got here from netpoll? napi budget was 0, so napi_safe is
> false, but in_softirq() can be true or false.

If we're on the same CPU where the NAPI would run and in the same
context, i.e. softirq, in which the NAPI would run, what is the problem?
If there really is a good one, I can handle it here.

> 
> XDP SKB is a toy, I really don't think 3-4% in XDP SKB warrants the
> risk here.

It affects not only XDP skb, I told ya already :D @napi_safe is very
conservative and misses a good bunch of stuff.
I really don't think that very rare corner cases (which can be handled
if needed) warrants the perf miss here.

Thanks,
Olek