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

From: Thomas Gleixner <tglx@linutronix.de>
Date: Fri, 14 May 2021 11:46:08 +0200

The driver invokes napi_schedule() in several places from task
context. napi_schedule() raises the NET_RX softirq bit and relies on the
calling context to ensure that the softirq is handled. That's usually on
return from interrupt or on the outermost local_bh_enable().

But that's not the case here which causes the soft interrupt handling to be
delayed to the next interrupt or local_bh_enable(). If the task in which
context this is invoked is the last runnable task on a CPU and the CPU goes
idle before an interrupt arrives or a local_bh_disable/enable() pair
handles the pending soft interrupt then the NOHZ idle code emits the
following warning.

  NOHZ tick-stop error: Non-RCU local softirq work is pending, handler #08!!!

Prevent this by wrapping the napi_schedule() invocation from task context
into a local_bh_disable/enable() pair.

Reported-by: Michal Svec <msvec@suse.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---

Note: That's not the first incident of this. Shouldn't napi_schedule()
      have a debug check (under lockdep) to catch this?

---
 drivers/net/usb/r8152.c |   19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

On Sat, May 15, 2021 at 01:23:02AM +0200, Thomas Gleixner wrote:
> 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 :)

Yeah, lovely stuff :-)


> +#define lockdep_assert_softirq_raise_ok()				\
> +do {									\
> +	WARN_ON_ONCE(__lockdep_enabled &&				\
> +		     !current->softirq_raise_safe &&			\
> +		     !(softirq_count() | hardirq_count()));		\
> +} while (0)

> --- a/kernel/smp.c
> +++ b/kernel/smp.c
> @@ -691,7 +691,9 @@ void flush_smp_call_function_from_idle(v
>  	cfd_seq_store(this_cpu_ptr(&cfd_seq_local)->idle, CFD_SEQ_NOCPU,
>  		      smp_processor_id(), CFD_SEQ_IDLE);
>  	local_irq_save(flags);
> +	lockdep_set_softirq_raise_safe();
>  	flush_smp_call_function_queue(true);
> +	lockdep_clear_softirq_raise_safe();
>  	if (local_softirq_pending())
>  		do_softirq();

I think it might make more sense to raise hardirq_count() in/for
flush_smp_call_function_queue() callers that aren't already from hardirq
context. That's this site and smpcfd_dying_cpu().

Then we can do away with this new special case.

On Sat, May 15 2021 at 15:09, Peter Zijlstra wrote:
> On Sat, May 15, 2021 at 01:23:02AM +0200, Thomas Gleixner wrote:
>> --- a/kernel/smp.c
>> +++ b/kernel/smp.c
>> @@ -691,7 +691,9 @@ void flush_smp_call_function_from_idle(v
>>  	cfd_seq_store(this_cpu_ptr(&cfd_seq_local)->idle, CFD_SEQ_NOCPU,
>>  		      smp_processor_id(), CFD_SEQ_IDLE);
>>  	local_irq_save(flags);
>> +	lockdep_set_softirq_raise_safe();
>>  	flush_smp_call_function_queue(true);
>> +	lockdep_clear_softirq_raise_safe();
>>  	if (local_softirq_pending())
>>  		do_softirq();
>
> I think it might make more sense to raise hardirq_count() in/for
> flush_smp_call_function_queue() callers that aren't already from hardirq
> context. That's this site and smpcfd_dying_cpu().
>
> Then we can do away with this new special case.

Right.

Though I just checked smpcfd_dying_cpu(). That ones does not run
softirqs after flushing the function queue and it can't do that because
that's in the CPU dying phase with interrupts disabled where the CPU is
already half torn down.

Especially as softirq processing enables interrupts, which might cause
even more havoc.

Anyway how is it safe to run arbitrary functions there after the CPU
removed itself from the online mask? That's daft to put it mildly.

Thanks,

        tglx