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

[Thomas Gleixner <tglx@linutronix.de>]

On Fri, May 14 2021 at 14:41, Jakub Kicinski wrote:
> 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?

Yes, they do. See irq_forced_thread_fn(). It has a local_bh_disable() /
local_bh_ enable() pair around the invocation to ensure that.

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

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 :)

Thanks,

        tglx
---
 include/linux/lockdep.h |   21 +++++++++++++++++++++
 include/linux/sched.h   |    1 +
 kernel/smp.c            |    2 ++
 kernel/softirq.c        |   18 ++++++++++++++----
 4 files changed, 38 insertions(+), 4 deletions(-)

--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -636,6 +636,23 @@ do {									\
 		     (!in_softirq() || in_irq() || in_nmi()));		\
 } while (0)
 
+#define lockdep_set_softirq_raise_safe()				\
+do {									\
+	current->softirq_raise_safe = 1;				\
+} while (0)
+
+#define lockdep_clear_softirq_raise_safe()				\
+do {									\
+	current->softirq_raise_safe = 0;				\
+} while (0)
+
+#define lockdep_assert_softirq_raise_ok()				\
+do {									\
+	WARN_ON_ONCE(__lockdep_enabled &&				\
+		     !current->softirq_raise_safe &&			\
+		     !(softirq_count() | hardirq_count()));		\
+} while (0)
+
 #else
 # define might_lock(lock) do { } while (0)
 # define might_lock_read(lock) do { } while (0)
@@ -648,6 +665,10 @@ do {									\
 # define lockdep_assert_preemption_enabled() do { } while (0)
 # define lockdep_assert_preemption_disabled() do { } while (0)
 # define lockdep_assert_in_softirq() do { } while (0)
+
+# define lockdep_set_softirq_raise_safe()	do { } while (0)
+# define lockdep_clear_softirq_raise_safe()	do { } while (0)
+# define lockdep_assert_softirq_raise_ok()	do { } while (0)
 #endif
 
 #ifdef CONFIG_PROVE_RAW_LOCK_NESTING
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1058,6 +1058,7 @@ struct task_struct {
 	u64				curr_chain_key;
 	int				lockdep_depth;
 	unsigned int			lockdep_recursion;
+	unsigned int			softirq_raise_safe;
 	struct held_lock		held_locks[MAX_LOCK_DEPTH];
 #endif
 
--- 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();
 
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -664,12 +664,19 @@ void irq_exit(void)
 	lockdep_hardirq_exit();
 }
 
+static inline void ____raise_softirq_irqoff(unsigned int nr)
+{
+	lockdep_assert_irqs_disabled();
+	trace_softirq_raise(nr);
+	or_softirq_pending(1UL << nr);
+}
+
 /*
  * This function must run with irqs disabled!
  */
 inline void raise_softirq_irqoff(unsigned int nr)
 {
-	__raise_softirq_irqoff(nr);
+	____raise_softirq_irqoff(nr);
 
 	/*
 	 * If we're in an interrupt or softirq, we're done
@@ -693,11 +700,14 @@ void raise_softirq(unsigned int nr)
 	local_irq_restore(flags);
 }
 
+/*
+ * Must be invoked with interrupts disabled and either from softirq serving
+ * context or with local bottom halfs disabled.
+ */
 void __raise_softirq_irqoff(unsigned int nr)
 {
-	lockdep_assert_irqs_disabled();
-	trace_softirq_raise(nr);
-	or_softirq_pending(1UL << nr);
+	lockdep_assert_softirq_raise_ok();
+	____raise_softirq_irqoff(nr);
 }
 
 void open_softirq(int nr, void (*action)(struct softirq_action *))