[PATCH v3 0/5] Idle Load Balance fixes and softirq enhancements

[PATCH v3 0/5] Idle Load Balance fixes and softirq enhancements

[K Prateek Nayak]

Hello everyone,

This is the third version with minor changes from the last, and some
more benchmarking data down below. Any and all feedback is highly
appreciated.

This series is based on:

    git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git sched/core

at commit 7266f0a6d3bb ("fs/bcachefs: Fix __wait_on_freeing_inode()
definition of waitqueue entry")

Since commit b2a02fc43a1f ("smp: Optimize
send_call_function_single_ipi()"), an idle CPU in TIF_POLLING_NRFLAG can
be pulled out of idle by setting TIF_NEED_RESCHED instead of sending an
actual IPI. This affects at least three scenarios that have been
described below:

1. A need_resched() check within a call function does not necessarily
   indicate a task wakeup since a CPU intending to send an IPI to an
   idle target in TIF_POLLING_NRFLAG mode can simply queue the
   SMP-call-function and set the TIF_NEED_RESCHED flag to pull the
   polling target out of idle. The SMP-call-function will be executed by
   flush_smp_call_function_queue() on the idle-exit path. On x86, where
   mwait_idle_with_hints() sets TIF_POLLING_NRFLAG for long idling,
   this leads to idle load balancer bailing out early since
   need_resched() check in nohz_csd_func() returns true in most
   instances.

2. A TIF_POLLING_NRFLAG idling CPU woken up to process an IPI will end
   up calling schedule() even in cases where the call function does not
   wake up a new task on the idle CPU, thus delaying the idle re-entry.

3. Julia Lawall reported a case where a softirq raised from a
   SMP-call-function on an idle CPU will wake up ksoftirqd since
   flush_smp_call_function_queue() executes in the idle thread's
   context. This can throw off the idle load balancer by making the idle
   CPU appear busy since ksoftirqd just woke on the said CPU [1].

Solution to (2.) was sent independently in [2] since it was not
dependent on the changes enclosed in this series which reworks some
PREEMPT_RT specific bits.

(1.) Was solved by dropping the need_resched() check in nohz_csd_func()
(please refer Patch 2/5 for the full version of the explanation) which
led to a splat on PREEMPT_RT kernels [3].

Since flush_smp_call_function_queue() and the following
do_softirq_post_smp_call_flush() runs with interrupts disabled, it is
not ideal for the IRQ handlers to raise a SOFTIRQ, prolonging the IRQs
disabled section especially on PREEMPT_RT kernels. For the time being,
the WARN_ON_ONCE() in do_softirq_post_smp_call_flush() has been adjusted
to allow raising a SCHED_SOFTIRQ from flush_smp_call_function_queue()
however its merit can be debated on this RFC.

With the above solution, problem discussed in (3.) is even more
prominent with idle load balancing waking up ksoftirqd to unnecessarily
(please refer Patch 5/5 for a detailed explanation). v1 attempted to
solve this by introducing a per-cpu variable to keep track on an
impending call to do_softirq(). Peter suggested reusing the
softirq_ctrl::cnt that PREEMPT_RT uses to prevent wakeup of ksoftirqd
and unifying should_wakeup_ksoftirqd() [4]. Patch 3 and 4 prepares for
this unification and Patch 5 adds and uses a new interface for
flush_smp_call_function_queue() to convey that a call do do_softirq() is
pending and there is no need to wakeup ksoftirqd.

Chenyu had reported a regression when running a modified version of
ipistorm that performs a fixed set of IPIs between two CPUs on his
setup with the whole v1 applied. I've benchmarked this series on both an
AMD and an Intel system to catch any significant regression early.
Following are the numbers from a dual socket Intel Ice Lake Xeon server
(2 x 32C/64T) and 3rd Generation AMD EPYC system (2 x 64C/128T) running
ipistorm between CPU8 and CPU16 (unless stated otherwise with *):

base: tip/sched/core at commit 7266f0a6d3bb ("fs/bcachefs: Fix
      __wait_on_freeing_inode() definition of waitqueue entry")

   ==================================================================
   Test          : ipistorm (modified)
   Units         : % improvement over base kernel in IPI throughput
   Interpretation: Higher is better
   ======================= Intel Ice Lake Xeon ======================
   kernel:					[pct imp]
   performance gov, boost off, idle=poll	  -3.86%
   performance gov, boost off, idle=poll *	  -3.32%
   ==================== 3rd Generation AMD EPYC =====================
   kernel:					[pct imp]
   performance gov, boost on, !PREEMPT_RT	   1.07%
   performance gov, boost on,  PREEMPT_RT	  19.51%
   ==================================================================

* cross node setup used CPU 16 on Node 0 and CPU 17 on Node 1 on the
  dual socket Intel Ice Lake Xeon system.

Improvements on PREEMPT_RT can perhaps be attributed to cacheline
aligning the per-cpu softirq_ctrl variable.

Julia Lawall reported reduction in the number of load balancing attempts
on v6.11 based tip:sched/core at NUMA level. The issue was root-caused
to commit 3dcac251b066 ("sched/core: Introduce SM_IDLE and an idle
re-entry fast-path in __schedule()") which would skip the
newidle_balance() if schedule_idle() was called without any task wakeups
on the idle CPUs to favor a faster idle re-entry. To rule out any
surprises from this series in particular, I tested the bt.B.x benchmark
where she originally observed this behavior on. Following are the
numbers from a dual socket Intel Ice Lake Xeon server (2 x 32C/32T smt
off):

   ==================================================================
   Test          : bt.B.x (OMP variant)
   Units         : % improvement over base kernel in Mop/s throughput
   Interpretation: Higher is better
   ======================= Intel Ice Lake Xeon ======================
   kernel:						[pct imp]
   performance gov, boost off, idle=poll, smt off	  1.09%
   ==================================================================

I did not see any discernable difference with this one over the base
kernel.

[1] https://lore.kernel.org/lkml/fcf823f-195e-6c9a-eac3-25f870cb35ac@inria.fr/
[2] https://lore.kernel.org/lkml/20240809092240.6921-1-kprateek.nayak@amd.com/
[3] https://lore.kernel.org/lkml/225e6d74-ed43-51dd-d1aa-c75c86dd58eb@amd.com/
[4] https://lore.kernel.org/lkml/20240710150557.GB27299@noisy.programming.kicks-ass.net/
---
v2..v3:

o Removed ifdefs around local_lock_t. (Peter)

o Reworded Patch 1 to add more details on raising SCHED_SOFTIRQ from
  flush_smp_call_function_queue() and why it should be okay on
  PREEMPT_RT.

o Updated the trace data in Patch 5.

o More benchmarking.

v2: https://lore.kernel.org/lkml/20240904111223.1035-1-kprateek.nayak@amd.com/

v1..v2:

o Broke the PREEMPT_RT unification and idle load balance fixes into
  separate series (this) and post the SM_IDLE fast-path enhancements
  separately.

o Worked around the splat on PREEMPT_RT kernel caused by raising
  SCHED_SOFTIRQ from nohz_csd_func() in context of
  flush_smp_call_function_queue() which is undesirable on PREEMPT_RT
  kernels. (Please refer to commit 1a90bfd22020 ("smp: Make softirq
  handling RT safe in flush_smp_call_function_queue()")

o Reuse softirq_ctrl::cnt from PREEMPT_RT to prevent unnecessary
  wakeups of ksoftirqd. (Peter)
  This unifies should_wakeup_ksoftirqd() and adds an interface to
  indicate an impending call to do_softirq (set_do_softirq_pending())
  and clear it just before fulfilling the promise
  (clr_do_softirq_pending()).

o More benchmarking.

v1: https://lore.kernel.org/lkml/20240710090210.41856-1-kprateek.nayak@amd.com/
--
K Prateek Nayak (5):
  softirq: Allow raising SCHED_SOFTIRQ from SMP-call-function on RT
    kernel
  sched/core: Remove the unnecessary need_resched() check in
    nohz_csd_func()
  softirq: Mask reads of softirq_ctrl.cnt with SOFTIRQ_MASK for
    PREEMPT_RT
  softirq: Unify should_wakeup_ksoftirqd()
  softirq: Avoid unnecessary wakeup of ksoftirqd when a call to
    do_sofirq() is pending

 kernel/sched/core.c |  2 +-
 kernel/sched/smp.h  |  9 +++++
 kernel/smp.c        |  2 +
 kernel/softirq.c    | 93 +++++++++++++++++++++++++++------------------
 4 files changed, 67 insertions(+), 39 deletions(-)

-- 
2.34.1

[PATCH v3 1/5] softirq: Allow raising SCHED_SOFTIRQ from SMP-call-function on RT kernel

[K Prateek Nayak]

do_softirq_post_smp_call_flush() on PREEMPT_RT kernels carries a
WARN_ON_ONCE() for any SOFTIRQ being raised from an SMP-call-function.
Since do_softirq_post_smp_call_flush() is called with preempt disabled,
raising a SOFTIRQ during flush_smp_call_function_queue() can lead to
longer preempt disabled sections.

Since commit b2a02fc43a1f ("smp: Optimize
send_call_function_single_ipi()") IPIs to an idle CPU in
TIF_POLLING_NRFLAG mode can be optimized out by instead setting
TIF_NEED_RESCHED bit in idle task's thread_info and relying on the
flush_smp_call_function_queue() in the idle-exit path to run the
SMP-call-function.

To trigger an idle load balancing, the scheduler queues
nohz_csd_function() responsible for triggering an idle load balancing on
a target nohz idle CPU and sends an IPI. Only now, this IPI is optimized
out and the SMP-call-function is executed from
flush_smp_call_function_queue() in do_idle() which can raise a
SCHED_SOFTIRQ to trigger the balancing.

So far, this went undetected since, the need_resched() check in
nohz_csd_function() would make it bail out of idle load balancing early
as the idle thread does not clear TIF_POLLING_NRFLAG before calling
flush_smp_call_function_queue(). The need_resched() check was added with
the intent to catch a new task wakeup, however, it has recently
discovered to be unnecessary and will be removed soon. As such,
nohz_csd_function() will raise a SCHED_SOFTIRQ from
flush_smp_call_function_queue() to trigger an idle load balance on an
idle target.

nohz_csd_function() bails out early if "idle_cpu()" check for the
target CPU returns false and should not delay a newly woken up task from
running. Account for this and prevent a WARN_ON_ONCE() when
SCHED_SOFTIRQ is raised from flush_smp_call_function_queue().

Signed-off-by: K Prateek Nayak <kprateek.nayak@amd.com>
---
v2..v3:

o Added more details on SCHED_SOFTIRQ and how the idle load balancer
  will bail out if there is a new task that has woken up on the idle CPU
  and should not add more latency for task to run on CPU.
---
 kernel/softirq.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/kernel/softirq.c b/kernel/softirq.c
index b756d6b3fd09..d89be0affe46 100644
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -280,17 +280,24 @@ static inline void invoke_softirq(void)
 		wakeup_softirqd();
 }
 
+#define SCHED_SOFTIRQ_MASK	BIT(SCHED_SOFTIRQ)
+
 /*
  * flush_smp_call_function_queue() can raise a soft interrupt in a function
- * call. On RT kernels this is undesired and the only known functionality
- * in the block layer which does this is disabled on RT. If soft interrupts
- * get raised which haven't been raised before the flush, warn so it can be
+ * call. On RT kernels this is undesired and the only known functionalities
+ * are in the block layer which is disabled on RT, and in the scheduler for
+ * idle load balancing. If soft interrupts get raised which haven't been
+ * raised before the flush, warn if it is not a SCHED_SOFTIRQ so it can be
  * investigated.
  */
 void do_softirq_post_smp_call_flush(unsigned int was_pending)
 {
-	if (WARN_ON_ONCE(was_pending != local_softirq_pending()))
+	unsigned int is_pending = local_softirq_pending();
+
+	if (unlikely(was_pending != is_pending)) {
+		WARN_ON_ONCE(was_pending != (is_pending & ~SCHED_SOFTIRQ_MASK));
 		invoke_softirq();
+	}
 }
 
 #else /* CONFIG_PREEMPT_RT */
-- 
2.34.1

[PATCH v3 2/5] sched/core: Remove the unnecessary need_resched() check in nohz_csd_func()

[K Prateek Nayak]

The need_resched() check currently in nohz_csd_func() can be tracked
to have been added in scheduler_ipi() back in 2011 via commit
ca38062e57e9 ("sched: Use resched IPI to kick off the nohz idle balance")

Since then, it has travelled quite a bit but it seems like an idle_cpu()
check currently is sufficient to detect the need to bail out from an
idle load balancing. To justify this removal, consider all the following
case where an idle load balancing could race with a task wakeup:

o Since commit f3dd3f674555b ("sched: Remove the limitation of WF_ON_CPU
  on wakelist if wakee cpu is idle") a target perceived to be idle
  (target_rq->nr_running == 0) will return true for
  ttwu_queue_cond(target) which will offload the task wakeup to the idle
  target via an IPI.

  In all such cases target_rq->ttwu_pending will be set to 1 before
  queuing the wake function.

  If an idle load balance races here, following scenarios are possible:

  - The CPU is not in TIF_POLLING_NRFLAG mode in which case an actual
    IPI is sent to the CPU to wake it out of idle. If the
    nohz_csd_func() queues before sched_ttwu_pending(), the idle load
    balance will bail out since idle_cpu(target) returns 0 since
    target_rq->ttwu_pending is 1. If the nohz_csd_func() is queued after
    sched_ttwu_pending() it should see rq->nr_running to be non-zero and
    bail out of idle load balancing.

  - The CPU is in TIF_POLLING_NRFLAG mode and instead of an actual IPI,
    the sender will simply set TIF_NEED_RESCHED for the target to put it
    out of idle and flush_smp_call_function_queue() in do_idle() will
    execute the call function. Depending on the ordering of the queuing
    of nohz_csd_func() and sched_ttwu_pending(), the idle_cpu() check in
    nohz_csd_func() should either see target_rq->ttwu_pending = 1 or
    target_rq->nr_running to be non-zero if there is a genuine task
    wakeup racing with the idle load balance kick.

o The waker CPU perceives the target CPU to be busy
  (targer_rq->nr_running != 0) but the CPU is in fact going idle and due
  to a series of unfortunate events, the system reaches a case where the
  waker CPU decides to perform the wakeup by itself in ttwu_queue() on
  the target CPU but target is concurrently selected for idle load
  balance (XXX: Can this happen? I'm not sure, but we'll consider the
  mother of all coincidences to estimate the worst case scenario).

  ttwu_do_activate() calls enqueue_task() which would increment
  "rq->nr_running" post which it calls wakeup_preempt() which is
  responsible for setting TIF_NEED_RESCHED (via a resched IPI or by
  setting TIF_NEED_RESCHED on a TIF_POLLING_NRFLAG idle CPU) The key
  thing to note in this case is that rq->nr_running is already non-zero
  in case of a wakeup before TIF_NEED_RESCHED is set which would
  lead to idle_cpu() check returning false.

In all cases, it seems that need_resched() check is unnecessary when
checking for idle_cpu() first since an impending wakeup racing with idle
load balancer will either set the "rq->ttwu_pending" or indicate a newly
woken task via "rq->nr_running".

Chasing the reason why this check might have existed in the first place,
I came across  Peter's suggestion on the fist iteration of Suresh's
patch from 2011 [1] where the condition to raise the SCHED_SOFTIRQ was:

	sched_ttwu_do_pending(list);

	if (unlikely((rq->idle == current) &&
	    rq->nohz_balance_kick &&
	    !need_resched()))
		raise_softirq_irqoff(SCHED_SOFTIRQ);

Since the condition to raise the SCHED_SOFIRQ was preceded by
sched_ttwu_do_pending() (which is equivalent of sched_ttwu_pending()) in
the current upstream kernel, the need_resched() check was necessary to
catch a newly queued task. Peter suggested modifying it to:

	if (idle_cpu() && rq->nohz_balance_kick && !need_resched())
		raise_softirq_irqoff(SCHED_SOFTIRQ);

where idle_cpu() seems to have replaced "rq->idle == current" check.

Even back then, the idle_cpu() check would have been sufficient to catch
a new task being enqueued. Since commit b2a02fc43a1f ("smp: Optimize
send_call_function_single_ipi()") overloads the interpretation of
TIF_NEED_RESCHED for TIF_POLLING_NRFLAG idling, remove the
need_resched() check in nohz_csd_func() to raise SCHED_SOFTIRQ based
on Peter's suggestion.

Link: https://lore.kernel.org/all/1317670590.20367.38.camel@twins/ [1]
Link: https://lore.kernel.org/lkml/20240615014521.GR8774@noisy.programming.kicks-ass.net/
Fixes: b2a02fc43a1f ("smp: Optimize send_call_function_single_ipi()")
Suggested-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: K Prateek Nayak <kprateek.nayak@amd.com>
---
v2..v3:

o No changes.
---
 kernel/sched/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 43e453ab7e20..424c652a9ddc 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -1237,7 +1237,7 @@ static void nohz_csd_func(void *info)
 	WARN_ON(!(flags & NOHZ_KICK_MASK));
 
 	rq->idle_balance = idle_cpu(cpu);
-	if (rq->idle_balance && !need_resched()) {
+	if (rq->idle_balance) {
 		rq->nohz_idle_balance = flags;
 		raise_softirq_irqoff(SCHED_SOFTIRQ);
 	}
-- 
2.34.1

[PATCH v3 3/5] softirq: Mask reads of softirq_ctrl.cnt with SOFTIRQ_MASK for PREEMPT_RT

[K Prateek Nayak]

On PREEMPT_RT kernels, softirq_ctrl.cnt tracks the softirq count and set
the task::softirqs_disabled_cnt accordingly. This count is always
'changed by SOFTIRQ_OFFSET when entering and leaving softirq processing'
or 'SOFTIRQ_DISABLE_OFFSET (= 2 * SOFTIRQ_OFFSET) on local_bh_disable or
local_bh_enable", as stated in the comment about softirq_ctrl
declaration.

Soon, the softirq_ctrl.cnt will also be used by !PREEMPT_RT kernels to
track an impending call to do_softirq() in order to prevent pointless
wakeup of ksoftirqd which will use the lower bits of softirq_ctrl.cnt.

Mask all the current reads of softirq_ctrl.cnt on PREEMPT_RT kernels
with SOFTIRQ_MASK to track only multiples of SOFTIRQ_OFFSET based
changes.

No functional change intended.

Signed-off-by: K Prateek Nayak <kprateek.nayak@amd.com>
---
v2..v3:

o No changes.
---
 kernel/softirq.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/kernel/softirq.c b/kernel/softirq.c
index d89be0affe46..fc8c3a7dbe35 100644
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -138,7 +138,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 +155,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 +164,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 +185,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 +193,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 +213,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
-- 
2.34.1

[PATCH v3 4/5] softirq: Unify should_wakeup_ksoftirqd()

[K Prateek Nayak]

Define softirq_ctrl::cnt for !PREEMPT_RT kernels too and unify
should_wakeup_ksoftirqd() to return based on softirq_ctrl::cnt.
Since these counts can change quite frequently when running interrupt
heavy benchmark, declare per-cpu softirq_ctrl as cacheline aligned.

No functional changes intended since !PREEMPT_RT kernels do not
increment the softirq_ctrl::cnt (yet) and should always return true
mimicking the current behavior.

Signed-off-by: K Prateek Nayak <kprateek.nayak@amd.com>
---
v2..v3:

o Removed ifdefs around local_lock_t. (Peter)
---
 kernel/softirq.c | 51 ++++++++++++++++++++++--------------------------
 1 file changed, 23 insertions(+), 28 deletions(-)

diff --git a/kernel/softirq.c b/kernel/softirq.c
index fc8c3a7dbe35..0730c2b43ae4 100644
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -88,23 +88,6 @@ 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
-
 /*
  * RT accounts for BH disabled sections in task::softirqs_disabled_cnt and
  * also in per CPU softirq_ctrl::cnt. This is necessary to allow tasks in a
@@ -122,10 +105,32 @@ struct softirq_ctrl {
 	int		cnt;
 };
 
-static DEFINE_PER_CPU(struct softirq_ctrl, softirq_ctrl) = {
+static DEFINE_PER_CPU_ALIGNED(struct softirq_ctrl, softirq_ctrl) = {
 	.lock	= INIT_LOCAL_LOCK(softirq_ctrl.lock),
 };
 
+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
  *
@@ -270,11 +275,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())
@@ -419,11 +419,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)) {
-- 
2.34.1

[PATCH v3 5/5] softirq: Avoid unnecessary wakeup of ksoftirqd when a call to do_sofirq() is pending

[K Prateek Nayak]

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] dN.1.: nohz_csd_func: Raising SCHED_SOFTIRQ for nohz_idle_balance
       <idle>-0       [000] dN.1.: softirq_raise: vec=7 [action=SCHED]
       <idle>-0       [000] .Ns1.: softirq_entry: 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@inria.fr>
Closes: https://lore.kernel.org/lkml/fcf823f-195e-6c9a-eac3-25f870cb35ac@inria.fr/ [1]
Suggested-by: Peter Zijlstra <peterz@infradead.org> # Reuse softirq_ctrl.cnt from PREEMPT_RT
Signed-off-by: K Prateek Nayak <kprateek.nayak@amd.com>
---
v2..v3:

o Updated the traces with one where SCHED_SOFTIRQ is raised from
  flush_smp_call_function_queue() and not from hard IRQ context.
---
 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 f25e20617b7e..be0f84d62475 100644
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -612,7 +612,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 0730c2b43ae4..3a6b3e67ea24 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 {
 	local_lock_t	lock;
@@ -109,6 +113,16 @@ static DEFINE_PER_CPU_ALIGNED(struct softirq_ctrl, softirq_ctrl) = {
 	.lock	= INIT_LOCAL_LOCK(softirq_ctrl.lock),
 };
 
+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

Re: [PATCH v3 5/5] softirq: Avoid unnecessary wakeup of ksoftirqd when a call to do_sofirq() is pending

[Sebastian Andrzej Siewior]

On 2024-10-14 09:03:39 [+0000], K Prateek Nayak wrote:
> 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.

Stupid question. You talk about the invocation from nohz_csd_func(),
right?.
Given that this is an IPI and always invoked from an IRQ then the
softirq is invoked on IRQ-exit. If it is flushed from
flush_smp_call_function_queue() then the softirq is handled via
do_softirq_post_smp_call_flush(). In that case couldn't you just tell
nohz_csd_func() to use __raise_softirq_irqoff(SCHED_SOFTIRQ) ? This
should solve this, right?

> diff --git a/kernel/softirq.c b/kernel/softirq.c
> index 0730c2b43ae4..3a6b3e67ea24 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.
>   */

The comment that you are extending and the comment regarding
SOFTIRQ_OFFSET were nearby. I don't like that those two are now far
apart.

>  struct softirq_ctrl {
>  	local_lock_t	lock;
> @@ -109,6 +113,16 @@ static DEFINE_PER_CPU_ALIGNED(struct softirq_ctrl, softirq_ctrl) = {
>  	.lock	= INIT_LOCAL_LOCK(softirq_ctrl.lock),
>  };
>  
> +inline void set_do_softirq_pending(void)
> +{
> +	__this_cpu_inc(softirq_ctrl.cnt);
> +}
> +
> +inline void clr_do_softirq_pending(void)

there should be no inline here.

> +{
> +	__this_cpu_dec(softirq_ctrl.cnt);
> +}
> +
>  static inline bool should_wake_ksoftirqd(void)
>  {
>  	return !this_cpu_read(softirq_ctrl.cnt);

Sebastian

Re: [PATCH v3 5/5] softirq: Avoid unnecessary wakeup of ksoftirqd when a call to do_sofirq() is pending

[K Prateek Nayak]

Hello Sebastian,

Thank you for reviewing the series!

On 10/25/2024 10:33 PM, Sebastian Andrzej Siewior wrote:
> On 2024-10-14 09:03:39 [+0000], K Prateek Nayak wrote:
>> 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.
> 
> Stupid question. You talk about the invocation from nohz_csd_func(),
> right?.
> Given that this is an IPI and always invoked from an IRQ then the
> softirq is invoked on IRQ-exit.

Yes, there is no issues in that case.

> If it is flushed from
> flush_smp_call_function_queue() then the softirq is handled via
> do_softirq_post_smp_call_flush(). In that case couldn't you just tell
> nohz_csd_func() to use __raise_softirq_irqoff(SCHED_SOFTIRQ) ? This
> should solve this, right?

I cannot think of any reason why it wouldn't work. Let me check real
quick and update the series if it works. Thanks a ton for the
suggestion!

> 
>> diff --git a/kernel/softirq.c b/kernel/softirq.c
>> index 0730c2b43ae4..3a6b3e67ea24 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.
>>    */
> 
> The comment that you are extending and the comment regarding
> SOFTIRQ_OFFSET were nearby. I don't like that those two are now far
> apart.

Noted. If the above suggestion doesn't work, I'll rearrange this bit and
refresh the series.

> 
>>   struct softirq_ctrl {
>>   	local_lock_t	lock;
>> @@ -109,6 +113,16 @@ static DEFINE_PER_CPU_ALIGNED(struct softirq_ctrl, softirq_ctrl) = {
>>   	.lock	= INIT_LOCAL_LOCK(softirq_ctrl.lock),
>>   };
>>   
>> +inline void set_do_softirq_pending(void)
>> +{
>> +	__this_cpu_inc(softirq_ctrl.cnt);
>> +}
>> +
>> +inline void clr_do_softirq_pending(void)
> 
> there should be no inline here.

Ack. Will fix in the subsequent version if the alternate approach
doesn't work.

> 
>> +{
>> +	__this_cpu_dec(softirq_ctrl.cnt);
>> +}
>> +
>>   static inline bool should_wake_ksoftirqd(void)
>>   {
>>   	return !this_cpu_read(softirq_ctrl.cnt);
> 
> Sebastian

-- 
Thanks and Regards,
Prateek