The first sentence of the comment explaining run_rebalance_domains()
is historic and not true anymore:

    * run_rebalance_domains is triggered when needed from the scheduler tick.

... contradicted/modified by the second sentence:

    * Also triggered for NOHZ idle balancing (with NOHZ_BALANCE_KICK set).

Avoid that kind of confusion straight away and explain from what
places sched_balance_softirq() is triggered.

Signed-off-by: Ingo Molnar <mingo@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Vincent Guittot <vincent.guittot@linaro.org>
Cc: Dietmar Eggemann <dietmar.eggemann@arm.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Valentin Schneider <vschneid@redhat.com>
---
 kernel/sched/fair.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 4c46bffb6a7a..18b7d2999cff 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -12409,8 +12409,13 @@ static int newidle_balance(struct rq *this_rq, struct rq_flags *rf)
 }
 
 /*
- * run_rebalance_domains is triggered when needed from the scheduler tick.
- * Also triggered for NOHZ idle balancing (with NOHZ_BALANCE_KICK set).
+ * The run_rebalance_domains() softirq handler is triggered via SCHED_SOFTIRQ
+ * from two places:
+ *
+ *  - the scheduler_tick(),
+ *
+ *  - from the SMP cross-call function nohz_csd_func(),
+ *    used by NOHZ idle balancing (with NOHZ_BALANCE_KICK set).
  */
 static __latent_entropy void run_rebalance_domains(struct softirq_action *h)
 {
-- 
2.40.1

On 04/03/24 10:48, Ingo Molnar wrote:
> The first sentence of the comment explaining run_rebalance_domains()
> is historic and not true anymore:
>
>     * run_rebalance_domains is triggered when needed from the scheduler tick.
>
> ... contradicted/modified by the second sentence:
>
>     * Also triggered for NOHZ idle balancing (with NOHZ_BALANCE_KICK set).
>
> Avoid that kind of confusion straight away and explain from what
> places sched_balance_softirq() is triggered.
>
> Signed-off-by: Ingo Molnar <mingo@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Vincent Guittot <vincent.guittot@linaro.org>
> Cc: Dietmar Eggemann <dietmar.eggemann@arm.com>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Valentin Schneider <vschneid@redhat.com>
> ---
>  kernel/sched/fair.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 4c46bffb6a7a..18b7d2999cff 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -12409,8 +12409,13 @@ static int newidle_balance(struct rq *this_rq, struct rq_flags *rf)
>  }
>
>  /*
> - * run_rebalance_domains is triggered when needed from the scheduler tick.
> - * Also triggered for NOHZ idle balancing (with NOHZ_BALANCE_KICK set).
> + * The run_rebalance_domains() softirq handler is triggered via SCHED_SOFTIRQ
> + * from two places:
> + *
> + *  - the scheduler_tick(),
> + *
> + *  - from the SMP cross-call function nohz_csd_func(),
> + *    used by NOHZ idle balancing (with NOHZ_BALANCE_KICK set).

Bit of a nit but the CSD is also triggered via the scheduler_tick():

  scheduler_tick()
  `\
    trigger_load_balance()
    `\
      raise_softirq(SCHED_SOFTIRQ)

  scheduler_tick()
  `\
    trigger_load_balance()
    `\
      nohz_balance_kick()
      `\
        kick_ilb()
        `\
          smp_call_function_single_async(ilb_cpu, &cpu_rq(ilb_cpu)->nohz_csd);

I got to the below which is still somewhat confusing, thoughts?

"""
The run_rebalance_domains() softirq handler is triggered via SCHED_SOFTIRQ
from two places:

- directly from trigger_load_balance() in scheduler_tick(), for periodic
  load balance

- indirectly from kick_ilb() (invoked down the scheduler_tick() too), which
  issues an SMP cross-call to nohz_csd_func() which will itself raise the
  softirq, for NOHZ idle balancing.
"""

>   */
>  static __latent_entropy void run_rebalance_domains(struct softirq_action *h)
>  {
> --
> 2.40.1

On Tue, 5 Mar 2024 at 11:50, Valentin Schneider <vschneid@redhat.com> wrote:
>
> On 04/03/24 10:48, Ingo Molnar wrote:
> > The first sentence of the comment explaining run_rebalance_domains()
> > is historic and not true anymore:
> >
> >     * run_rebalance_domains is triggered when needed from the scheduler tick.
> >
> > ... contradicted/modified by the second sentence:
> >
> >     * Also triggered for NOHZ idle balancing (with NOHZ_BALANCE_KICK set).
> >
> > Avoid that kind of confusion straight away and explain from what
> > places sched_balance_softirq() is triggered.
> >
> > Signed-off-by: Ingo Molnar <mingo@kernel.org>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Vincent Guittot <vincent.guittot@linaro.org>
> > Cc: Dietmar Eggemann <dietmar.eggemann@arm.com>
> > Cc: Linus Torvalds <torvalds@linux-foundation.org>
> > Cc: Valentin Schneider <vschneid@redhat.com>
> > ---
> >  kernel/sched/fair.c | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index 4c46bffb6a7a..18b7d2999cff 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -12409,8 +12409,13 @@ static int newidle_balance(struct rq *this_rq, struct rq_flags *rf)
> >  }
> >
> >  /*
> > - * run_rebalance_domains is triggered when needed from the scheduler tick.
> > - * Also triggered for NOHZ idle balancing (with NOHZ_BALANCE_KICK set).
> > + * The run_rebalance_domains() softirq handler is triggered via SCHED_SOFTIRQ
> > + * from two places:
> > + *
> > + *  - the scheduler_tick(),
> > + *
> > + *  - from the SMP cross-call function nohz_csd_func(),
> > + *    used by NOHZ idle balancing (with NOHZ_BALANCE_KICK set).
>
> Bit of a nit but the CSD is also triggered via the scheduler_tick():
>
>   scheduler_tick()
>   `\
>     trigger_load_balance()
>     `\
>       raise_softirq(SCHED_SOFTIRQ)
>
>   scheduler_tick()
>   `\
>     trigger_load_balance()
>     `\
>       nohz_balance_kick()
>       `\
>         kick_ilb()
>         `\
>           smp_call_function_single_async(ilb_cpu, &cpu_rq(ilb_cpu)->nohz_csd);
>
> I got to the below which is still somewhat confusing, thoughts?
>




> """
> The run_rebalance_domains() softirq handler is triggered via SCHED_SOFTIRQ
> from two places:
>
> - directly from trigger_load_balance() in scheduler_tick(), for periodic
>   load balance
>
> - indirectly from kick_ilb() (invoked down the scheduler_tick() too), which
>   issues an SMP cross-call to nohz_csd_func() which will itself raise the
>   softirq, for NOHZ idle balancing.

I'm not sure that we should provide too many details of the path as
this might change in the future. What about the below ?

 - directly from the local scheduler_tick() for periodic load balance

- indirectly from a remote scheduler_tick() for NOHZ idle balancing
through the SMP cross-call nohz_csd_func()

> """
>
> >   */
> >  static __latent_entropy void run_rebalance_domains(struct softirq_action *h)
> >  {
> > --
> > 2.40.1
>

* Vincent Guittot <vincent.guittot@linaro.org> wrote:

> > """
> > The run_rebalance_domains() softirq handler is triggered via SCHED_SOFTIRQ
> > from two places:
> >
> > - directly from trigger_load_balance() in scheduler_tick(), for periodic
> >   load balance
> >
> > - indirectly from kick_ilb() (invoked down the scheduler_tick() too), which
> >   issues an SMP cross-call to nohz_csd_func() which will itself raise the
> >   softirq, for NOHZ idle balancing.
> 
> I'm not sure that we should provide too many details of the path as
> this might change in the future. What about the below ?
> 
>  - directly from the local scheduler_tick() for periodic load balance
> 
> - indirectly from a remote scheduler_tick() for NOHZ idle balancing
> through the SMP cross-call nohz_csd_func()

Okay - I updated it to:

  /*
   * This softirq handler is triggered via SCHED_SOFTIRQ from two places:
   *
   * - directly from the local scheduler_tick() for periodic load balancing
   *
   * - indirectly from a remote scheduler_tick() for NOHZ idle balancing
   *   through the SMP cross-call nohz_csd_func()
   */
  static __latent_entropy void run_rebalance_domains(struct softirq_action *h)

Does this work with everyone?

Thanks,

	Ingo

On Fri, 8 Mar 2024 at 11:15, Ingo Molnar <mingo@kernel.org> wrote:
>
>
> * Vincent Guittot <vincent.guittot@linaro.org> wrote:
>
> > > """
> > > The run_rebalance_domains() softirq handler is triggered via SCHED_SOFTIRQ
> > > from two places:
> > >
> > > - directly from trigger_load_balance() in scheduler_tick(), for periodic
> > >   load balance
> > >
> > > - indirectly from kick_ilb() (invoked down the scheduler_tick() too), which
> > >   issues an SMP cross-call to nohz_csd_func() which will itself raise the
> > >   softirq, for NOHZ idle balancing.
> >
> > I'm not sure that we should provide too many details of the path as
> > this might change in the future. What about the below ?
> >
> >  - directly from the local scheduler_tick() for periodic load balance
> >
> > - indirectly from a remote scheduler_tick() for NOHZ idle balancing
> > through the SMP cross-call nohz_csd_func()
>
> Okay - I updated it to:
>
>   /*
>    * This softirq handler is triggered via SCHED_SOFTIRQ from two places:
>    *
>    * - directly from the local scheduler_tick() for periodic load balancing
>    *
>    * - indirectly from a remote scheduler_tick() for NOHZ idle balancing
>    *   through the SMP cross-call nohz_csd_func()
>    */
>   static __latent_entropy void run_rebalance_domains(struct softirq_action *h)
>
> Does this work with everyone?

yes looks good for me

>
> Thanks,
>
>         Ingo

On 08/03/24 12:57, Vincent Guittot wrote:
> On Fri, 8 Mar 2024 at 11:15, Ingo Molnar <mingo@kernel.org> wrote:
>>
>> Okay - I updated it to:
>>
>>   /*
>>    * This softirq handler is triggered via SCHED_SOFTIRQ from two places:
>>    *
>>    * - directly from the local scheduler_tick() for periodic load balancing
>>    *
>>    * - indirectly from a remote scheduler_tick() for NOHZ idle balancing
>>    *   through the SMP cross-call nohz_csd_func()
>>    */
>>   static __latent_entropy void run_rebalance_domains(struct softirq_action *h)
>>
>> Does this work with everyone?
>
> yes looks good for me
>

Ditto!