Re: [PATCH RFC] r8152: Ensure that napi_schedule() is handled

On Sat, 15 May 2021 01:23:02 +0200 Thomas Gleixner wrote:

On Fri, May 14 2021 at 14:41, Jakub Kicinski wrote:

quoted

quoted

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!

quoted

quoted

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().