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

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

On Fri, 14 May 2021 23:10:43 +0200 Thomas Gleixner wrote:

quoted

On Fri, May 14 2021 at 13:46, Jakub Kicinski wrote:

quoted

On Fri, 14 May 2021 22:25:50 +0200 Thomas Gleixner wrote:  

quoted

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

quoted

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.

quoted

quoted

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