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

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

[K Prateek Nayak]

Hello everyone,

This is the forth 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 1a6151017ee5 ("sched: psi: pass enqueue/dequeue flags to psi
callbacks directly")

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
preempt disabled section especially on PREEMPT_RT kernels. Patch 1 works
around this by allowing processing of SCHED_SOFTIRQ without raising a
warning since the idle load balancer has checkpoints to detect wakup of
tasks on the idle CPU in which case it immediately bails out of the load
balaning process and proceeds to call schedule()

With the above solution, problem discussed in (3.) is even more
prominent with idle load balancing waking up ksoftirqd to unnecessarily.
Previous versions attempted to solve this by introducing a per-cpu
variable to keep track on an impending call to do_softirq(). Sebastian
suggested using __raise_softirq_irqoff() for SCHED_SOFTIRQ since it
avoids wakeup of ksoftirqd and since SCHED_SOFTIRQ is being raised from
the IPI handler, it is guaranteed to be serviced on IRQ exit or via
do_softirq_post_smp_call_flush()

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. Since this version gets rid of the
per-CPU variable updates at flush_smp_call_function_queue() boundary,
the results from testing is identical to the base kernel.

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	  0.89%
   ==================================================================

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/
---
v3..v4:

o Use __raise_softirq_irqoff() for SCHED_SOFTIRQ. (Sebastian)

o Updated benchmark numbers and reworded the cover letter.

v3: https://lore.kernel.org/lkml/20241014090339.2478-1-kprateek.nayak@amd.com/

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 (3):
  softirq: Allow raising SCHED_SOFTIRQ from SMP-call-function on RT
    kernel
  sched/core: Remove the unnecessary need_resched() check in
    nohz_csd_func()
  sched/core: Prevent wakeup of ksoftirqd during idle load balance

 kernel/sched/core.c | 15 +++++++++++++--
 kernel/softirq.c    | 15 +++++++++++----
 2 files changed, 24 insertions(+), 6 deletions(-)

-- 
2.34.1

[PATCH v4 1/3] 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>
---
v3..v4:

o No changes.
---
 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

Re: [PATCH v4 1/3] softirq: Allow raising SCHED_SOFTIRQ from SMP-call-function on RT kernel

[Sebastian Andrzej Siewior]

On 2024-10-30 07:15:55 [+0000], K Prateek Nayak wrote:
…
> --- 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();

This behaviour also happens with threadirqs on !PREEMPT_RT but without
the warning. I haven't checked it but I expect invoke_softirq() to wake
ksoftirqd here, too.
This only happens because of 2/3 in the series as far as I can tell.

Now I am curious to hear from the sched/ NOHZ folks if it makes sense to
invoke SCHED_SOFTIRQ from within ksoftirqd because unlike on an idle CPU
the CPU is now not seen as idle due to ksoftirqd running on the CPU.
There is code that checks rq->nr_running and/ or idle_cpu().

> +	}
>  }

Sebastian

[PATCH v4 2/3] 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>
---
v3..v4:

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 c57a79e34911..aaf99c0bcb49 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -1242,7 +1242,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 v4 3/3] sched/core: Prevent wakeup of ksoftirqd during idle load balance

[K Prateek Nayak]

Scheduler raises a SCHED_SOFTIRQ to trigger a load balancing event on
from the IPI handler on the idle CPU. Since the softirq can be raised
from flush_smp_call_function_queue(), it can end up waking up ksoftirqd,
which can give an illusion of the idle CPU being busy when doing an idle
load balancing.

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 the softirq raised is guranteed to be serviced in irq_exit() or
via do_softirq_post_smp_call_flush(), set SCHED_SOFTIRQ without checking
the need to wakeup ksoftirq for idle load balancing.

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: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: K Prateek Nayak <kprateek.nayak@amd.com>
---
v3..v4:

o New patch based on Sebastian's suggestion.
---
 kernel/sched/core.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index aaf99c0bcb49..2ee3621d6e7e 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -1244,7 +1244,18 @@ static void nohz_csd_func(void *info)
 	rq->idle_balance = idle_cpu(cpu);
 	if (rq->idle_balance) {
 		rq->nohz_idle_balance = flags;
-		raise_softirq_irqoff(SCHED_SOFTIRQ);
+
+		/*
+		 * Don't wakeup ksoftirqd when raising SCHED_SOFTIRQ
+		 * since the idle load balancer may mistake wakeup of
+		 * ksoftirqd as a genuine task wakeup and bail out from
+		 * load balancing early. Since it is guaranteed that
+		 * pending softirqs will be handled soon, either on
+		 * irq_exit() or via do_softirq_post_smp_call_flush(),
+		 * raise SCHED_SOFTIRQ without checking the need to
+		 * wakeup ksoftirqd.
+		 */
+		__raise_softirq_irqoff(SCHED_SOFTIRQ);
 	}
 }
 
-- 
2.34.1

Re: [PATCH v4 3/3] sched/core: Prevent wakeup of ksoftirqd during idle load balance

[Sebastian Andrzej Siewior]

On 2024-10-30 07:15:57 [+0000], K Prateek Nayak wrote:
> Scheduler raises a SCHED_SOFTIRQ to trigger a load balancing event on
> from the IPI handler on the idle CPU. Since the softirq can be raised
> from flush_smp_call_function_queue(), it can end up waking up ksoftirqd,
> which can give an illusion of the idle CPU being busy when doing an idle
> load balancing.
> 
> 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 the softirq raised is guranteed to be serviced in irq_exit() or
> via do_softirq_post_smp_call_flush(), set SCHED_SOFTIRQ without checking
> the need to wakeup ksoftirq for idle load balancing.
> 
> 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.

| Use __raise_softirq_irqoff() to raise the softirq. The SMP function call
| is always invoked on the requested CPU in an interrupt handler. It is
| guaranteed that soft interrupts are handled at the end.

You could extend it

| If the SMP function is invoked from an idle CPU via
| flush_smp_call_function_queue() then the HARD-IRQ flag is not set and
| raise_softirq_irqoff() wakes needlessly ksoftirqd because soft
| interrupts are handled before ksoftirqd get on the CPU.

This on its own is a reasonable optimisation. A different question would
be if flush_smp_call_function_queue() should pretend to be in-IRQ like a
regular IPI but…

Reviewed-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

> 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: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Signed-off-by: K Prateek Nayak <kprateek.nayak@amd.com>
> ---
> v3..v4:
> 
> o New patch based on Sebastian's suggestion.
> ---
>  kernel/sched/core.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index aaf99c0bcb49..2ee3621d6e7e 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -1244,7 +1244,18 @@ static void nohz_csd_func(void *info)
>  	rq->idle_balance = idle_cpu(cpu);
>  	if (rq->idle_balance) {
>  		rq->nohz_idle_balance = flags;
> -		raise_softirq_irqoff(SCHED_SOFTIRQ);
> +
> +		/*
> +		 * Don't wakeup ksoftirqd when raising SCHED_SOFTIRQ
> +		 * since the idle load balancer may mistake wakeup of
> +		 * ksoftirqd as a genuine task wakeup and bail out from
> +		 * load balancing early. Since it is guaranteed that
> +		 * pending softirqs will be handled soon, either on
> +		 * irq_exit() or via do_softirq_post_smp_call_flush(),
> +		 * raise SCHED_SOFTIRQ without checking the need to
> +		 * wakeup ksoftirqd.
> +		 */

/*
 * This is always invoked from an interrupt handler, simply raise the
 * softirq.
 */

should be enough IMHO. But *I* would even skip that, since it is
obvious.

> +		__raise_softirq_irqoff(SCHED_SOFTIRQ);
>  	}
>  }

Sebastian

Re: [PATCH v4 3/3] sched/core: Prevent wakeup of ksoftirqd during idle load balance

[K Prateek Nayak]

Hello Sebastian,

On 11/8/2024 5:47 PM, Sebastian Andrzej Siewior wrote:
> On 2024-10-30 07:15:57 [+0000], K Prateek Nayak wrote:
>> Scheduler raises a SCHED_SOFTIRQ to trigger a load balancing event on
>> from the IPI handler on the idle CPU. Since the softirq can be raised
>> from flush_smp_call_function_queue(), it can end up waking up ksoftirqd,
>> which can give an illusion of the idle CPU being busy when doing an idle
>> load balancing.
>>
>> 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 the softirq raised is guranteed to be serviced in irq_exit() or
>> via do_softirq_post_smp_call_flush(), set SCHED_SOFTIRQ without checking
>> the need to wakeup ksoftirq for idle load balancing.
>>
>> 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.
> 
> | Use __raise_softirq_irqoff() to raise the softirq. The SMP function call
> | is always invoked on the requested CPU in an interrupt handler. It is
> | guaranteed that soft interrupts are handled at the end.
> 
> You could extend it
> 
> | If the SMP function is invoked from an idle CPU via
> | flush_smp_call_function_queue() then the HARD-IRQ flag is not set and
> | raise_softirq_irqoff() wakes needlessly ksoftirqd because soft
> | interrupts are handled before ksoftirqd get on the CPU.

I'll reword the log as suggested in the next version.

> 
> This on its own is a reasonable optimisation. A different question would
> be if flush_smp_call_function_queue() should pretend to be in-IRQ like a
> regular IPI but…

I thought about it initially but seeing optimizations and checks around
"hardirq_stack" and checks to reuse it in certain context led be to
believe that there may be more nuances that I do not have a full picture
of, and I went ahead with this simpler solution.

> 
> Reviewed-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Thank you for the review!

> 
>> 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: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>> Signed-off-by: K Prateek Nayak <kprateek.nayak@amd.com>
>> ---
>> v3..v4:
>>
>> o New patch based on Sebastian's suggestion.
>> ---
>>   kernel/sched/core.c | 13 ++++++++++++-
>>   1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
>> index aaf99c0bcb49..2ee3621d6e7e 100644
>> --- a/kernel/sched/core.c
>> +++ b/kernel/sched/core.c
>> @@ -1244,7 +1244,18 @@ static void nohz_csd_func(void *info)
>>   	rq->idle_balance = idle_cpu(cpu);
>>   	if (rq->idle_balance) {
>>   		rq->nohz_idle_balance = flags;
>> -		raise_softirq_irqoff(SCHED_SOFTIRQ);
>> +
>> +		/*
>> +		 * Don't wakeup ksoftirqd when raising SCHED_SOFTIRQ
>> +		 * since the idle load balancer may mistake wakeup of
>> +		 * ksoftirqd as a genuine task wakeup and bail out from
>> +		 * load balancing early. Since it is guaranteed that
>> +		 * pending softirqs will be handled soon, either on
>> +		 * irq_exit() or via do_softirq_post_smp_call_flush(),
>> +		 * raise SCHED_SOFTIRQ without checking the need to
>> +		 * wakeup ksoftirqd.
>> +		 */
> 
> /*
>   * This is always invoked from an interrupt handler, simply raise the
>   * softirq.
>   */
> 
> should be enough IMHO. But *I* would even skip that, since it is
> obvious.

I'll remove it in the subsequent version. I'll wait a bit before sending
it to see if folks have any suggestion on the parallel thread regarding
handling SCHED_SOFTIRQ from ksoftirqd.

> 
>> +		__raise_softirq_irqoff(SCHED_SOFTIRQ);
>>   	}
>>   }
> 
> Sebastian

-- 
Thanks and Regards,
Prateek