On Sat, 15 May 2021 01:23:02 +0200 Thomas Gleixner wrote: > On Fri, May 14 2021 at 14:41, Jakub Kicinski wrote: > >> 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? > > Yes, they do. See irq_forced_thread_fn(). It has a local_bh_disable() / > local_bh_ enable() pair around the invocation to ensure that. Ah, excellent! > >> 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. > > We can make that work but sure I'm not going to argue when you decide to > just go for raise_softirq_irqsoff(). > > I just hacked that check up which is actually useful beyond NAPI. It's > straight forward except for that flush_smp_call_function_from_idle() > oddball, which immeditately triggered that assert because block mq uses > __raise_softirq_irqsoff() in a smp function call... > > See below. Peter might have opinions though :) Looks good to me, since my thinking that RT complicates things here was wrong I'm perfectly happy with the lockdep + napi_schedule_intask().
