On Fri, 14 May 2021 23:10:43 +0200 Thomas Gleixner wrote: > On Fri, May 14 2021 at 13:46, Jakub Kicinski wrote: > > On Fri, 14 May 2021 22:25:50 +0200 Thomas Gleixner wrote: > >> Except that some instruction cycle beancounters might complain about > >> the extra conditional for the sane cases. > >> > >> But yes, I'm fine with that as well. That's why this patch is marked RFC :) > > > > When we're in the right context (irq/bh disabled etc.) the cost is just > > read of preempt_count() and jump, right? And presumably preempt_count() > > is in the cache already, because those sections aren't very long. Let me > > make this change locally and see if it is in any way perceivable. > > Right. Just wanted to mention it :) > > > Obviously if anyone sees a way to solve the problem without much > > ifdefinery and force_irqthreads checks that'd be great - I don't. > > This is not related to force_irqthreads at all. This very driver invokes > it from plain thread context. I see, but a driver calling __napi_schedule_irqoff() from its IRQ handler _would_ be an issue, right? Or do irq threads trigger softirq processing on exit? > > I'd rather avoid pushing this kind of stuff out to the drivers. > > You could have napi_schedule_intask() or something like that which would > do the local_bh_disable()/enable() dance around the invocation of > napi_schedule(). That would also document it clearly in the drivers. A > quick grep shows a bunch of instances which could be replaced: > > drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c-5704- local_bh_disable(); > drivers/net/ethernet/mellanox/mlx4/en_netdev.c-1830- local_bh_disable(); > drivers/net/usb/r8152.c-1552- local_bh_disable(); > drivers/net/virtio_net.c-1355- local_bh_disable(); > drivers/net/wireless/intel/iwlwifi/pcie/rx.c-1650- local_bh_disable(); > drivers/net/wireless/intel/iwlwifi/pcie/rx.c-2015- local_bh_disable(); > drivers/net/wireless/intel/iwlwifi/pcie/rx.c-2225- local_bh_disable(); > drivers/net/wireless/intel/iwlwifi/pcie/rx.c-2235- local_bh_disable(); > drivers/s390/net/qeth_core_main.c-3515- local_bh_disable(); Very well aware, I've just sent a patch for mlx5 last week :) My initial reaction was the same as yours - we should add lockdep check, and napi_schedule_intask(). But then I started wondering if it's all for nothing on rt or with force_irqthreads, and therefore we should just eat the extra check.
