On Thu, 10 Feb 2022 13:22:32 +0100 Sebastian Andrzej Siewior wrote: > On 2022-02-07 08:47:17 [-0800], Jakub Kicinski wrote: > > On Sat, 5 Feb 2022 21:36:05 +0100 Sebastian Andrzej Siewior wrote: > > > Don't we end up in the same situation as netif_rx() vs netix_rx_ni()? > > > > Sort of. TBH my understanding of the motivation is a bit vague. > > IIUC you want to reduce the API duplication so drivers know what to > > do[1]. I believe the quote from Eric you put in the commit message > > pertains to HW devices, where using netif_rx() is quite anachronistic. > > But software devices like loopback, veth or tunnels may want to go via > > backlog for good reasons. Would it make it better if we called > > netif_rx() netif_rx_backlog() instead? Or am I missing the point? > > So we do netif_rx_backlog() with the bh disable+enable and > __netif_rx_backlog() without it and export both tree wide? At a risk of confusing people about the API we could also name the "non-super-optimized" version netif_rx(), like you had in your patch. Grepping thru the drivers there's ~250 uses so maybe we don't wanna touch all that code. No strong preference, I just didn't expect to see __netif_rx_backlog(), but either way works. > It would make it more obvious indeed. Could we add > WARN_ON_ONCE(!(hardirq_count() | softirq_count())) > to the shortcut to catch the "you did it wrong folks"? This costs me > about 2ns. Modulo lockdep_..(), so we don't have to run this check on prod kernels? > TL;DR > > The netix_rx_ni() is problematic on RT and I tried to do something about > it. I remembered from the in_atomic() cleanup that a few drivers got it > wrong (one way or another). We added also netif_rx_any_context() which > is used by some of the drivers (which is yet another entry point) while > the few other got fixed. > Then I stumbled over the thread where the entry (netif_rx() vs > netif_rx_ni()) was wrong and Dave suggested to have one entry point for > them all. This sounded like a good idea since it would eliminate the > several API entry points where things can go wrong and my RT trouble > would vanish in one go. > The part with deprecated looked promising but I didn't take into account > that the overhead for legitimate users (like the backlog or the software > tunnels you mention) is not acceptable. I see. So IIUC primary motivation is replacing preempt disable with bh disable but the cleanup seemed like a good idea.
