Message-ID: <20240904111223.1035-6-kprateek.nayak@amd.com>
Date: Wed, 4 Sep 2024 11:12:23 +0000
From: K Prateek Nayak <kprateek.nayak@....com>
To: Ingo Molnar <mingo@...hat.com>, Peter Zijlstra <peterz@...radead.org>,
	Juri Lelli <juri.lelli@...hat.com>, Vincent Guittot
	<vincent.guittot@...aro.org>, Dietmar Eggemann <dietmar.eggemann@....com>,
	Steven Rostedt <rostedt@...dmis.org>, Ben Segall <bsegall@...gle.com>, "Mel
 Gorman" <mgorman@...e.de>, Valentin Schneider <vschneid@...hat.com>, "Thomas
 Gleixner" <tglx@...utronix.de>
CC: Leonardo Bras <leobras@...hat.com>, "Paul E. McKenney"
	<paulmck@...nel.org>, Rik van Riel <riel@...riel.com>, Thorsten Blum
	<thorsten.blum@...lux.com>, Zqiang <qiang.zhang1211@...il.com>, Tejun Heo
	<tj@...nel.org>, Lai Jiangshan <jiangshanlai@...il.com>, Caleb Sander Mateos
	<csander@...estorage.com>, <linux-kernel@...r.kernel.org>, K Prateek Nayak
	<kprateek.nayak@....com>, "Gautham R . Shenoy" <gautham.shenoy@....com>,
	"Chen Yu" <yu.c.chen@...el.com>, Julia Lawall <Julia.Lawall@...ia.fr>,
	"Sebastian Andrzej Siewior" <bigeasy@...utronix.de>
Subject: [RFC PATCH v2 5/5] softirq: Avoid unnecessary wakeup of ksoftirqd when a call to do_sofirq() is pending

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.

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: Raising SCHED_SOFTIRQ from nohz_csd_func
       <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] .Ns1.:  softirq_exit: vec=7  [action=SCHED]
       <idle>-0   [000] d..2.:  sched_switch: prev_comm=swapper/0 prev_pid=0 prev_prio=120 prev_state=R ==> next_comm=ksoftirqd/0 next_pid=16 next_prio=120
  ksoftirqd/0-16  [000] d..2.:  sched_switch: prev_comm=ksoftirqd/0 prev_pid=16 prev_prio=120 prev_state=S ==> next_comm=swapper/0 next_pid=0 next_prio=120
       ...

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

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.

Introduce two new APIs:

- set_do_softirq_pending(): Increments per-cpu softirq_ctrl::cnt by 1 to
  indicate a pending call to do_softirq().

- clr_do_softirq_pending(): Decrements per-cpu softirq_ctrl::cnt by 1
  just before calling do_softirq().

Call set_do_softirq_pending() before __flush_smp_call_function_queue()
within flush_smp_call_function_queue() to indicate a pending call to
do_softirq() and clr_do_softirq_pending() before calling
do_softirq_post_smp_call_flush() to mark the promise being fulfilled.
Since this impending call is tracked by softirq_ctrl::cnt,
should_wakeup_ksoftirqd() will return false and prevent a pointless
wakeup of ksoftirqd.

Following are the observations with the changes when enabling the same
set of events:

       <idle>-0  [000] d.h1.:  nohz_csd_func: Raising SCHED_SOFTIRQ for nohz_idle_balance
       <idle>-0  [000] ..s1.:  softirq_entry: vec=7 [action=SCHED]
       <idle>-0  [000] ..s1.:  softirq_exit:  vec=7 [action=SCHED]
       ...

No unnecessary ksoftirqd wakeups are seen from idle task's context to
service the softirq.

Fixes: b2a02fc43a1f ("smp: Optimize send_call_function_single_ipi()")
Reported-by: Julia Lawall <julia.lawall@...ia.fr>
Closes: https://lore.kernel.org/lkml/fcf823f-195e-6c9a-eac3-25f870cb35ac@inria.fr/ [1]
Suggested-by: Peter Zijlstra <peterz@...radead.org> # Reuse softirq_ctrl.cnt from PREEMPT_RT
Signed-off-by: K Prateek Nayak <kprateek.nayak@....com>
---
v1..v2:

- Re-used PREEMPT_RT softirq_ctrl::cnt to prevent unnecessary wakeup of
  ksoftirqd. (Peter)

- Broken off from approach discussed in
  https://lore.kernel.org/lkml/880f13fd-753d-2c5a-488a-d75c99e8dfa3@amd.com/
---
 kernel/sched/smp.h |  9 +++++++++
 kernel/smp.c       |  2 ++
 kernel/softirq.c   | 14 ++++++++++++++
 3 files changed, 25 insertions(+)

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 aaffecdad319..65a47b33a57e 100644
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -584,7 +584,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 d8902fbcdebf..935a2d8091b5 100644
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -99,6 +99,10 @@ 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
@@ -113,6 +117,16 @@ static DEFINE_PER_CPU_ALIGNED(struct softirq_ctrl, softirq_ctrl) = {
 #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);
-- 
2.34.1

Powered by blists - more mailing lists