Since commit b2a02fc43a1f4 ("smp: Optimize
send_call_function_single_ipi()"), sending an actual interrupt to an
idle CPU in TIF_POLLING_NRFLAG mode can be avoided by queuing the SMP
call function on the call function queue of the CPU and setting the
TIF_NEED_RESCHED bit in idle task's thread info. The call function is
handled in the idle exit path when do_idle() calls
flush_smp_call_function_queue().

However, since flush_smp_call_function_queue() is executed in idle
thread's context, in_interrupt() check within a call function will
return false. raise_softirq() uses this check to decide whether to wake
ksoftirqd, since, a softirq raised from an interrupt context will be
handled at irq exit. In all other cases, raise_softirq() wakes up
ksoftirqd to handle the softirq on !PREEMPT_RT kernel.

Since flush_smp_call_function_queue() calls
do_softirq_post_smp_call_flush(), waking up ksoftirqd is not necessary
since the softirqs raised by the call functions will be handled soon
after the call function queue is flushed. Mark
__flush_smp_call_function_queue() within flush_smp_call_function_queue()
with "will_do_softirq_post_flush" and use "do_softirq_pending()" to
notify raise_softirq() an impending call to do_softirq() and avoid
waking up ksoftirqd.

Adding a trace_printk() in nohz_csd_func() at the spot of raising
SCHED_SOFTIRQ and enabling trace events for sched_switch, sched_wakeup,
and softirq_entry (for SCHED_SOFTIRQ vector alone) helps observing the
current behavior:

	  <idle>-0       [000] dN.1. nohz_csd_func: Raise SCHED_SOFTIRQ for idle balance
	  <idle>-0       [000] dN.4. sched_wakeup: comm=ksoftirqd/0 pid=16 prio=120 target_cpu=000
	  <idle>-0       [000] .Ns1. softirq_entry: vec=7 [action=SCHED]
	  <idle>-0       [000] d..2. sched_switch: prev_comm=swapper/0 ==> next_comm=ksoftirqd/0
     ksoftirqd/0-16      [000] d..2. sched_switch: prev_comm=ksoftirqd/0 ==> next_comm=swapper/0

ksoftirqd is woken up before the idle thread calls
do_softirq_post_smp_call_flush() which can make the runqueue appear
busy and prevent the idle load balancer from pulling task from an
overloaded runqueue towards itself[1]. Following are the observations
with the changes when enabling the same set of events:

	  <idle>-0       [000] dN.1.   106.134226: nohz_csd_func: Raise SCHED_SOFTIRQ for idle balance
	  <idle>-0       [000] .Ns1.   106.134227: softirq_entry: vec=7 [action=SCHED]
	  ...

No unnecessary ksoftirqd wakeups are seen from idle task's context to
service the softirq. When doing performance testing, it was noticed that
per-CPU "will_do_softirq_post_flush" variable needs to be defined as
cacheline aligned to minimize performance overheads of the writes in
flush_smp_call_function_queue(). Following is the IPI throughput
measured using a modified version of ipistorm that performs a fixed set
of IPIs between two CPUs on a dual socket 3rd Generation EPYC system
(2 x 64C/128T) (boost on, C2 disabled) by running ipistorm between CPU8
and CPU16:

cmdline: insmod ipistorm.ko numipi=100000 single=1 offset=8 cpulist=8 wait=1

   ==================================================================
   Test          : ipistorm (modified)
   Units         : Normalized runtime
   Interpretation: Lower is better
   Statistic     : AMean
   ==================================================================
   kernel:					time [pct imp]
   tip:sched/core				1.00 [baseline]
   tip:sched/core + SM_IDLE			0.25 [75.11%]
   tip:sched/core + SM_IDLE + unaligned var	0.47 [53.74%] *
   tip:sched/core + SM_IDLE + aligned var	0.25 [75.04%]

* The version where "will_do_softirq_post_flush" was not cacheline
  aligned takes twice as long as the cacheline aligned version to
  perform a fixed set of IPIs.

Fixes: b2a02fc43a1f ("smp: Optimize send_call_function_single_ipi()")
Reported-by: Julia Lawall <julia.lawall@inria.fr>
Closes: https://lore.kernel.org/lkml/fcf823f-195e-6c9a-eac3-25f870cb35ac@inria.fr/ [1]
Signed-off-by: K Prateek Nayak <kprateek.nayak@amd.com>
---
 kernel/sched/smp.h |  2 ++
 kernel/smp.c       | 32 ++++++++++++++++++++++++++++++++
 kernel/softirq.c   | 10 +++++++++-
 3 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/kernel/sched/smp.h b/kernel/sched/smp.h
index 21ac44428bb0..3731e79fe19b 100644
--- a/kernel/sched/smp.h
+++ b/kernel/sched/smp.h
@@ -9,7 +9,9 @@ extern void sched_ttwu_pending(void *arg);
 extern bool call_function_single_prep_ipi(int cpu);
 
 #ifdef CONFIG_SMP
+extern bool do_softirq_pending(void);
 extern void flush_smp_call_function_queue(void);
 #else
+static inline bool do_softirq_pending(void) { return false; }
 static inline void flush_smp_call_function_queue(void) { }
 #endif
diff --git a/kernel/smp.c b/kernel/smp.c
index f085ebcdf9e7..2eab5e1d5cef 100644
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -559,6 +559,36 @@ static void __flush_smp_call_function_queue(bool warn_cpu_offline)
 	}
 }
 
+/* Indicate an impending call to do_softirq_post_smp_call_flush() */
+static DEFINE_PER_CPU_ALIGNED(bool, will_do_softirq_post_flush);
+
+static __always_inline void __set_will_do_softirq_post_flush(void)
+{
+	this_cpu_write(will_do_softirq_post_flush, true);
+}
+
+static __always_inline void __clr_will_do_softirq_post_flush(void)
+{
+	this_cpu_write(will_do_softirq_post_flush, false);
+}
+
+/**
+ * do_softirq_pending - Check if do_softirq_post_smp_call_flush() will
+ *			be called after the invocation of
+ *			__flush_smp_call_function_queue()
+ *
+ * When flush_smp_call_function_queue() executes in the context of idle,
+ * migration thread, a softirq raised from the smp-call-function ends up
+ * waking ksoftirqd despite an impending softirq processing via
+ * do_softirq_post_smp_call_flush().
+ *
+ * Indicate an impending do_softirq() to should_wake_ksoftirqd() despite
+ * not being in an interrupt context.
+ */
+__always_inline bool do_softirq_pending(void)
+{
+	return this_cpu_read(will_do_softirq_post_flush);
+}
 
 /**
  * flush_smp_call_function_queue - Flush pending smp-call-function callbacks
@@ -583,7 +613,9 @@ void flush_smp_call_function_queue(void)
 	local_irq_save(flags);
 	/* Get the already pending soft interrupts for RT enabled kernels */
 	was_pending = local_softirq_pending();
+	__set_will_do_softirq_post_flush();
 	__flush_smp_call_function_queue(true);
+	__clr_will_do_softirq_post_flush();
 	if (local_softirq_pending())
 		do_softirq_post_smp_call_flush(was_pending);
 
diff --git a/kernel/softirq.c b/kernel/softirq.c
index 02582017759a..b39eeed03042 100644
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -34,6 +34,8 @@
 #define CREATE_TRACE_POINTS
 #include <trace/events/irq.h>
 
+#include "sched/smp.h"
+
 /*
    - No shared variables, all the data are CPU local.
    - If a softirq needs serialization, let it serialize itself
@@ -413,7 +415,13 @@ static inline void ksoftirqd_run_end(void)
 
 static inline bool should_wake_ksoftirqd(void)
 {
-	return true;
+	/*
+	 * Avoid waking up ksoftirqd when a softirq is raised from a
+	 * call-function executed by flush_smp_call_function_queue()
+	 * in idle, migration thread's context since it'll soon call
+	 * do_softirq_post_smp_call_flush().
+	 */
+	return !do_softirq_pending();
 }
 
 static inline void invoke_softirq(void)
-- 
2.34.1

On Wed, Jul 10, 2024 at 09:02:10AM +0000, K Prateek Nayak wrote:

> diff --git a/kernel/sched/smp.h b/kernel/sched/smp.h
> index 21ac44428bb0..3731e79fe19b 100644
> --- a/kernel/sched/smp.h
> +++ b/kernel/sched/smp.h
> @@ -9,7 +9,9 @@ extern void sched_ttwu_pending(void *arg);
>  extern bool call_function_single_prep_ipi(int cpu);
>  
>  #ifdef CONFIG_SMP
> +extern bool do_softirq_pending(void);
>  extern void flush_smp_call_function_queue(void);
>  #else
> +static inline bool do_softirq_pending(void) { return false; }
>  static inline void flush_smp_call_function_queue(void) { }
>  #endif
> diff --git a/kernel/smp.c b/kernel/smp.c
> index f085ebcdf9e7..2eab5e1d5cef 100644
> --- a/kernel/smp.c
> +++ b/kernel/smp.c
> @@ -559,6 +559,36 @@ static void __flush_smp_call_function_queue(bool warn_cpu_offline)
>  	}
>  }
>  
> +/* Indicate an impending call to do_softirq_post_smp_call_flush() */
> +static DEFINE_PER_CPU_ALIGNED(bool, will_do_softirq_post_flush);
> +
> +static __always_inline void __set_will_do_softirq_post_flush(void)
> +{
> +	this_cpu_write(will_do_softirq_post_flush, true);
> +}
> +
> +static __always_inline void __clr_will_do_softirq_post_flush(void)
> +{
> +	this_cpu_write(will_do_softirq_post_flush, false);
> +}
> +
> +/**
> + * do_softirq_pending - Check if do_softirq_post_smp_call_flush() will
> + *			be called after the invocation of
> + *			__flush_smp_call_function_queue()
> + *
> + * When flush_smp_call_function_queue() executes in the context of idle,
> + * migration thread, a softirq raised from the smp-call-function ends up
> + * waking ksoftirqd despite an impending softirq processing via
> + * do_softirq_post_smp_call_flush().
> + *
> + * Indicate an impending do_softirq() to should_wake_ksoftirqd() despite
> + * not being in an interrupt context.
> + */
> +__always_inline bool do_softirq_pending(void)
> +{
> +	return this_cpu_read(will_do_softirq_post_flush);
> +}
>  
>  /**
>   * flush_smp_call_function_queue - Flush pending smp-call-function callbacks
> @@ -583,7 +613,9 @@ void flush_smp_call_function_queue(void)
>  	local_irq_save(flags);
>  	/* Get the already pending soft interrupts for RT enabled kernels */
>  	was_pending = local_softirq_pending();
> +	__set_will_do_softirq_post_flush();
>  	__flush_smp_call_function_queue(true);
> +	__clr_will_do_softirq_post_flush();
>  	if (local_softirq_pending())
>  		do_softirq_post_smp_call_flush(was_pending);
>  
> diff --git a/kernel/softirq.c b/kernel/softirq.c
> index 02582017759a..b39eeed03042 100644
> --- a/kernel/softirq.c
> +++ b/kernel/softirq.c
> @@ -34,6 +34,8 @@
>  #define CREATE_TRACE_POINTS
>  #include <trace/events/irq.h>
>  
> +#include "sched/smp.h"
> +
>  /*
>     - No shared variables, all the data are CPU local.
>     - If a softirq needs serialization, let it serialize itself
> @@ -413,7 +415,13 @@ static inline void ksoftirqd_run_end(void)
>  
>  static inline bool should_wake_ksoftirqd(void)
>  {
> -	return true;
> +	/*
> +	 * Avoid waking up ksoftirqd when a softirq is raised from a
> +	 * call-function executed by flush_smp_call_function_queue()
> +	 * in idle, migration thread's context since it'll soon call
> +	 * do_softirq_post_smp_call_flush().
> +	 */
> +	return !do_softirq_pending();
>  }

On first reading I wonder why you've not re-used and hooked into the
PREEMPT_RT variant of should_wake_ksoftirqd(). That already has a per
CPU variable to do exactly this.

Hello Peter,

Thank you for the feedback.

On 7/10/2024 8:35 PM, Peter Zijlstra wrote:
> On Wed, Jul 10, 2024 at 09:02:10AM +0000, K Prateek Nayak wrote:
> 
>> [..snip..]
> 
> On first reading I wonder why you've not re-used and hooked into the
> PREEMPT_RT variant of should_wake_ksoftirqd(). That already has a per
> CPU variable to do exactly this.

With this RFC, I intended to check if everyone was onboard with the idea
and of the use-case. One caveat with re-using the existing
"softirq_ctrl.cnt" hook that PREEMPT_RT uses is that we'll need to
expose the functions that increment and decrement it, for it to be used
in kernel/smp.c. I'll make those changes in v2 and we can see if there
are sufficient WARN_ON() to catch any incorrect usage in !PREEMPT_RT
case.

-- 
Thanks and Regards,
Prateek

Hello Peter,

On 7/10/2024 11:50 PM, K Prateek Nayak wrote:
> Hello Peter,
> 
> Thank you for the feedback.
> 
> On 7/10/2024 8:35 PM, Peter Zijlstra wrote:
>> On Wed, Jul 10, 2024 at 09:02:10AM +0000, K Prateek Nayak wrote:
>>
>>> [..snip..]
>>
>> On first reading I wonder why you've not re-used and hooked into the
>> PREEMPT_RT variant of should_wake_ksoftirqd(). That already has a per
>> CPU variable to do exactly this.
> 
> With this RFC, I intended to check if everyone was onboard with the idea
> and of the use-case. One caveat with re-using the existing
> "softirq_ctrl.cnt" hook that PREEMPT_RT uses is that we'll need to
> expose the functions that increment and decrement it, for it to be used
> in kernel/smp.c. I'll make those changes in v2 and we can see if there
> are sufficient WARN_ON() to catch any incorrect usage in !PREEMPT_RT
> case.
> 

Reusing the PREEMPT_RT bits, I was able to come up with the approach
below. Following are some nuances with this approach:

- Although I don't believe "set_do_softirq_pending()" can be nested,
   since it is used only from flush_smp_call_function_queue() which is
   called with IRQs disabled, I'm still using inc and dec since I'm not
   sure if it can be nested in a local_bh_{disable,enable}() or if
   those can be called from SMP-call-function.

- I used the same modified version of ipistorm to test the changes on
   top of v6.10-rc6-rt11 with LOCKDEP enabled and did not see any splats
   during the run of the benchmark. If there are better tests that
   stress this part on RT kernels, I'm all ears.

- I've abandoned any micro-optimization since I see different numbers
   on different machines and am sticking to the simplest approach since
   it works and is an improvement over the baseline.

I'll leave the diff below:

diff --git a/kernel/sched/smp.h b/kernel/sched/smp.h
index 21ac44428bb0..83f70626ff1e 100644
--- a/kernel/sched/smp.h
+++ b/kernel/sched/smp.h
@@ -9,7 +9,16 @@ extern void sched_ttwu_pending(void *arg);
  extern bool call_function_single_prep_ipi(int cpu);
  
  #ifdef CONFIG_SMP
+/*
+ * Used to indicate a pending call to do_softirq() from
+ * flush_smp_call_function_queue()
+ */
+extern void set_do_softirq_pending(void);
+extern void clr_do_softirq_pending(void);
+
  extern void flush_smp_call_function_queue(void);
  #else
+static inline void set_do_softirq_pending(void) { }
+static inline void clr_do_softirq_pending(void) { }
  static inline void flush_smp_call_function_queue(void) { }
  #endif
diff --git a/kernel/smp.c b/kernel/smp.c
index f085ebcdf9e7..c191bad912f6 100644
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -583,7 +583,9 @@ void flush_smp_call_function_queue(void)
  	local_irq_save(flags);
  	/* Get the already pending soft interrupts for RT enabled kernels */
  	was_pending = local_softirq_pending();
+	set_do_softirq_pending();
  	__flush_smp_call_function_queue(true);
+	clr_do_softirq_pending();
  	if (local_softirq_pending())
  		do_softirq_post_smp_call_flush(was_pending);
  
diff --git a/kernel/softirq.c b/kernel/softirq.c
index 00e32e279fa9..8308687fc7b9 100644
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -88,22 +88,7 @@ EXPORT_PER_CPU_SYMBOL_GPL(hardirqs_enabled);
  EXPORT_PER_CPU_SYMBOL_GPL(hardirq_context);
  #endif
  
-/*
- * SOFTIRQ_OFFSET usage:
- *
- * On !RT kernels 'count' is the preempt counter, on RT kernels this applies
- * to a per CPU counter and to task::softirqs_disabled_cnt.
- *
- * - count is changed by SOFTIRQ_OFFSET on entering or leaving softirq
- *   processing.
- *
- * - count is changed by SOFTIRQ_DISABLE_OFFSET (= 2 * SOFTIRQ_OFFSET)
- *   on local_bh_disable or local_bh_enable.
- *
- * This lets us distinguish between whether we are currently processing
- * softirq and whether we just have bh disabled.
- */
-#ifdef CONFIG_PREEMPT_RT
+#define DO_SOFTIRQ_PENDING_MASK	GENMASK(SOFTIRQ_SHIFT - 1, 0)
  
  /*
   * RT accounts for BH disabled sections in task::softirqs_disabled_cnt and
@@ -116,16 +101,56 @@ EXPORT_PER_CPU_SYMBOL_GPL(hardirq_context);
   *
   * The per CPU counter prevents pointless wakeups of ksoftirqd in case that
   * the task which is in a softirq disabled section is preempted or blocks.
+ *
+ * The bottom bits of softirq_ctrl::cnt is used to indicate an impending call
+ * to do_softirq() to prevent pointless wakeups of ksoftirqd since the CPU
+ * promises to handle softirqs soon.
   */
  struct softirq_ctrl {
+#ifdef CONFIG_PREEMPT_RT
  	local_lock_t	lock;
+#endif
  	int		cnt;
  };
  
  static DEFINE_PER_CPU(struct softirq_ctrl, softirq_ctrl) = {
+#ifdef CONFIG_PREEMPT_RT
  	.lock	= INIT_LOCAL_LOCK(softirq_ctrl.lock),
+#endif
  };
  
+inline void set_do_softirq_pending(void)
+{
+	__this_cpu_inc(softirq_ctrl.cnt);
+}
+
+inline void clr_do_softirq_pending(void)
+{
+	__this_cpu_dec(softirq_ctrl.cnt);
+}
+
+static inline bool should_wake_ksoftirqd(void)
+{
+	return !this_cpu_read(softirq_ctrl.cnt);
+}
+
+/*
+ * SOFTIRQ_OFFSET usage:
+ *
+ * On !RT kernels 'count' is the preempt counter, on RT kernels this applies
+ * to a per CPU counter and to task::softirqs_disabled_cnt.
+ *
+ * - count is changed by SOFTIRQ_OFFSET on entering or leaving softirq
+ *   processing.
+ *
+ * - count is changed by SOFTIRQ_DISABLE_OFFSET (= 2 * SOFTIRQ_OFFSET)
+ *   on local_bh_disable or local_bh_enable.
+ *
+ * This lets us distinguish between whether we are currently processing
+ * softirq and whether we just have bh disabled.
+ */
+#ifdef CONFIG_PREEMPT_RT
+
  /**
   * local_bh_blocked() - Check for idle whether BH processing is blocked
   *
@@ -138,7 +163,7 @@ static DEFINE_PER_CPU(struct softirq_ctrl, softirq_ctrl) = {
   */
  bool local_bh_blocked(void)
  {
-	return __this_cpu_read(softirq_ctrl.cnt) != 0;
+	return (__this_cpu_read(softirq_ctrl.cnt) & SOFTIRQ_MASK) != 0;
  }
  
  void __local_bh_disable_ip(unsigned long ip, unsigned int cnt)
@@ -155,7 +180,8 @@ void __local_bh_disable_ip(unsigned long ip, unsigned int cnt)
  			/* Required to meet the RCU bottomhalf requirements. */
  			rcu_read_lock();
  		} else {
-			DEBUG_LOCKS_WARN_ON(this_cpu_read(softirq_ctrl.cnt));
+			DEBUG_LOCKS_WARN_ON(this_cpu_read(softirq_ctrl.cnt) &
+					    SOFTIRQ_MASK);
  		}
  	}
  
@@ -163,7 +189,7 @@ void __local_bh_disable_ip(unsigned long ip, unsigned int cnt)
  	 * Track the per CPU softirq disabled state. On RT this is per CPU
  	 * state to allow preemption of bottom half disabled sections.
  	 */
-	newcnt = __this_cpu_add_return(softirq_ctrl.cnt, cnt);
+	newcnt = __this_cpu_add_return(softirq_ctrl.cnt, cnt) & SOFTIRQ_MASK;
  	/*
  	 * Reflect the result in the task state to prevent recursion on the
  	 * local lock and to make softirq_count() & al work.
@@ -184,7 +210,7 @@ static void __local_bh_enable(unsigned int cnt, bool unlock)
  	int newcnt;
  
  	DEBUG_LOCKS_WARN_ON(current->softirq_disable_cnt !=
-			    this_cpu_read(softirq_ctrl.cnt));
+			    (this_cpu_read(softirq_ctrl.cnt) & SOFTIRQ_MASK));
  
  	if (IS_ENABLED(CONFIG_TRACE_IRQFLAGS) && softirq_count() == cnt) {
  		raw_local_irq_save(flags);
@@ -192,7 +218,7 @@ static void __local_bh_enable(unsigned int cnt, bool unlock)
  		raw_local_irq_restore(flags);
  	}
  
-	newcnt = __this_cpu_sub_return(softirq_ctrl.cnt, cnt);
+	newcnt = __this_cpu_sub_return(softirq_ctrl.cnt, cnt) & SOFTIRQ_MASK;
  	current->softirq_disable_cnt = newcnt;
  
  	if (!newcnt && unlock) {
@@ -212,7 +238,7 @@ void __local_bh_enable_ip(unsigned long ip, unsigned int cnt)
  	lockdep_assert_irqs_enabled();
  
  	local_irq_save(flags);
-	curcnt = __this_cpu_read(softirq_ctrl.cnt);
+	curcnt = __this_cpu_read(softirq_ctrl.cnt) & SOFTIRQ_MASK;
  
  	/*
  	 * If this is not reenabling soft interrupts, no point in trying to
@@ -253,7 +279,7 @@ void softirq_preempt(void)
  	if (WARN_ON_ONCE(!preemptible()))
  		return;
  
-	if (WARN_ON_ONCE(__this_cpu_read(softirq_ctrl.cnt) != SOFTIRQ_OFFSET))
+	if (WARN_ON_ONCE((__this_cpu_read(softirq_ctrl.cnt) & SOFTIRQ_MASK) != SOFTIRQ_OFFSET))
  		return;
  
  	__local_bh_enable(SOFTIRQ_OFFSET, true);
@@ -282,11 +308,6 @@ static inline void ksoftirqd_run_end(void)
  static inline void softirq_handle_begin(void) { }
  static inline void softirq_handle_end(void) { }
  
-static inline bool should_wake_ksoftirqd(void)
-{
-	return !this_cpu_read(softirq_ctrl.cnt);
-}
-
  static inline void invoke_softirq(void)
  {
  	if (should_wake_ksoftirqd())
@@ -424,11 +445,6 @@ static inline void ksoftirqd_run_end(void)
  	local_irq_enable();
  }
  
-static inline bool should_wake_ksoftirqd(void)
-{
-	return true;
-}
-
  static inline void invoke_softirq(void)
  {
  	if (!force_irqthreads() || !__this_cpu_read(ksoftirqd)) {
--

Some of the (softirq_ctrl.cnt & SOFTIRQ_MASK) masking might be
unnecessary but I'm not familiar enough with all these bits to make a
sound call. Any and all comments are appreciated :)

--
Thanks and Regards,
Prateek