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.1On 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
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
© 2016 - 2024 Red Hat, Inc.