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: Thu, 20 Jul 2023 12:20:15 -0700

> On Thu, 20 Jul 2023 20:13:07 +0200 Alexander Lobakin wrote:
>> IOW, it reports we're in softirq no bloody matter if interrupts are
>> enabled or not. Either I did something wrong or the entire in_*irq()
>> family, including interrupt_context_level(), doesn't protect from
>> anything at all and doesn't work the way that most devs expect it to work?
>>
>> (or was it just me? :D)
>>
>> I guess the only way to be sure is to always check irqs_disabled() when
>> in_softirq() returns true.
> 
> We can as well check
> 	(in_softirq() && !irqs_disabled() && !in_hardirq())
> ?

Yes, something like that. Messy, but I see no other options...

So, I guess you want to add an assertion to make sure that we're *not*
in this:

in_hardirq() || irqs_disabled()

Does this mean that after it's added, my patch is sane? :p

> 
> The interrupt_context_level() thing is fairly new, I think.
> Who knows what happens to it going forward...

Well, it counts the number of active hard interrupts, but doesn't take
into account that if there are no hardirqs we can still disable them
manually. Meh.
Should I try to send a patch for it? :D

> 
>>>> Right now page pool only supports BH and process contexts. IOW the
>>>> "else" branch of if (in_softirq()) in page pool is expecting to be
>>>> in process context.
>>>>
>>>> Supporting hard irq would mean we need to switch to _irqsave() locking.
>>>> That's likely way too costly.
>>>>
>>>> Or stash the freed pages away and free them lazily.
>>>>
>>>> Or add a lockdep warning and hope nobody will ever free a page-pool
>>>> backed skb from hard IRQ context :)  
>>>
>>> I told you under the previous version that this function is not supposed
>>> to be called under hardirq context, so we don't need to check for it :D
>>> But I was assuming nobody would try to do that. Seems like not really
>>> (netcons) if you want to sanitize this...
> 
> netcons or anyone who freed socket-less skbs from hardirq.
> Until pp recycling was added freeing an skb from hardirq was legal,
> AFAICT.

I don't think so. Why do we have dev_kfree_skb_any() then? It checks for

in_hardirq() || irqs_disabled()

and if it's true, defers the skb to process it by backlog task.
"Regular" skb freeing functions don't do that. The _any() variant lives
here for a long time IIRC, so it's not something recent.

Thanks,
Olek