When nohz_full CPU stops tick in tick_nohz_irq_exit(),
It wouldn't be chosen to perform idle load balancing because it doesn't
call nohz_balance_enter_idle() in tick_nohz_idle_stop_tick() when it
becomes idle.

tick_nohz_idle_stop_tick() is only called in idle state and
nohz_balance_enter_idle() tracks the CPU which is part of nohz.idle_cpus_mask
with rq->nohz_tick_stopped.

Change tick_nohz_idle_stop_tick() to call nohz_balance_enter_idle()
without checking !was_stopped so that nohz_full cpu can be chosen to
perform idle load balancing when it enters idle state.

Signed-off-by: Levi Yun <ppbuk5246@gmail.com>
---
v3:
	- Rewording commit message.

v2:
	- Fix typos in commit message.

 kernel/time/tick-sched.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index 71a792cd8936..31a4cd89782f 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -1228,8 +1228,10 @@ void tick_nohz_idle_stop_tick(void)
 		ts->idle_sleeps++;
 		ts->idle_expires = expires;

-		if (!was_stopped && tick_sched_flag_test(ts, TS_FLAG_STOPPED)) {
-			ts->idle_jiffies = ts->last_jiffies;
+		if (tick_sched_flag_test(ts, TS_FLAG_STOPPED)) {
+			if (!was_stopped)
+				ts->idle_jiffies = ts->last_jiffies;
+
 			nohz_balance_enter_idle(cpu);
 		}
 	} else {
--
2.41.0

> When nohz_full CPU stops tick in tick_nohz_irq_exit(),
> It wouldn't be chosen to perform idle load balancing because it doesn't
> call nohz_balance_enter_idle() in tick_nohz_idle_stop_tick() when it
> becomes idle.
>
> tick_nohz_idle_stop_tick() is only called in idle state and
> nohz_balance_enter_idle() tracks the CPU which is part of nohz.idle_cpus_mask
> with rq->nohz_tick_stopped.
>
> Change tick_nohz_idle_stop_tick() to call nohz_balance_enter_idle()
> without checking !was_stopped so that nohz_full cpu can be chosen to
> perform idle load balancing when it enters idle state.

Would you eventually like to add the tag “Fixes” once more?


…
> +++ b/kernel/time/tick-sched.c
> @@ -1228,8 +1228,10 @@ void tick_nohz_idle_stop_tick(void)
>  		ts->idle_sleeps++;
>  		ts->idle_expires = expires;
>
> -		if (!was_stopped && tick_sched_flag_test(ts, TS_FLAG_STOPPED)) {
> -			ts->idle_jiffies = ts->last_jiffies;
> +		if (tick_sched_flag_test(ts, TS_FLAG_STOPPED)) {
> +			if (!was_stopped)
> +				ts->idle_jiffies = ts->last_jiffies;
> +
>  			nohz_balance_enter_idle(cpu);
>  		}
…

I interpret these diff data in the way that you propose to reorder
two condition checks.

But I wonder still how “good” the presented change description fits to
the suggested source code adjustment.

Regards,
Markus

> Would you eventually like to add the tag “Fixes” once more?

Sorry. I forgot :(

> > +++ b/kernel/time/tick-sched.c
> > @@ -1228,8 +1228,10 @@ void tick_nohz_idle_stop_tick(void)
> >               ts->idle_sleeps++;
> >               ts->idle_expires = expires;
> >
> > -             if (!was_stopped && tick_sched_flag_test(ts, TS_FLAG_STOPPED)) {
> > -                     ts->idle_jiffies = ts->last_jiffies;
> > +             if (tick_sched_flag_test(ts, TS_FLAG_STOPPED)) {
> > +                     if (!was_stopped)
> > +                             ts->idle_jiffies = ts->last_jiffies;
> > +
> >                       nohz_balance_enter_idle(cpu);
> >               }
> …
>
> I interpret these diff data in the way that you propose to reorder
> two condition checks.
>
> But I wonder still how “good” the presented change description fits to
> the suggested source code adjustment.

FWIW it doesn't need to check !was_stopped to
call nohz_balance_enter_idle() in tick_nohz_idle_stop_tick().
Formerly, __tick_nohz_idle_enter() is called in both
tick_nohz_irq_exit() and in do_idle().
That's why it's required for nohz full cpu which already stop the
tick, but not idle
to prevent enter idle balance.
(but it makes nohz full cpu enter nohz idle balance as side effect i think?)

 but after some reorganizing code tick_nohz_idle_stop_tick() becomes
the code called in
only when enter idle.

What I point is that it doesn't need to check !was_stopped to call
nohz_balance_enter_idle()
in tick_nohz_idle_stop_tick().

So, I think it's enough in commit message?

Am I wrong?

Thanks.

>>> +++ b/kernel/time/tick-sched.c
>>> @@ -1228,8 +1228,10 @@ void tick_nohz_idle_stop_tick(void)
>>>               ts->idle_sleeps++;
>>>               ts->idle_expires = expires;
>>>
>>> -             if (!was_stopped && tick_sched_flag_test(ts, TS_FLAG_STOPPED)) {
>>> -                     ts->idle_jiffies = ts->last_jiffies;
>>> +             if (tick_sched_flag_test(ts, TS_FLAG_STOPPED)) {
>>> +                     if (!was_stopped)
>>> +                             ts->idle_jiffies = ts->last_jiffies;
>>> +
>>>                       nohz_balance_enter_idle(cpu);
>>>               }
…
> So, I think it's enough in commit message?
…

We are trying to clarify special implementation details here.
Our corresponding wording preferences are probably different.
I hope that a better common understanding can be achieved also for
another transformation.

* Thus I became curious how you got interested to adjust this software
  component further.

* Will any other data representation become more helpful for the circumstances
  according to calls of a function like “tick_nohz_idle_stop_tick”?

* How do you think about to stress condition ordering concerns around
  the system configuration “nohz_full”?

* How will related changelogs evolve further?


Regards,
Markus

Hi Markus.
.

> * Will any other data representation become more helpful for the circumstances
>   according to calls of a function like “tick_nohz_idle_stop_tick”?

Maybe not in this commit..?


> * How do you think about stress condition ordering concerns around
>   the system configuration “nohz_full”?

Well.. regardless of the stress condition, it wants to fix the
inconsistent behavior
happening when enter "idle state"

Let's think about two cases:
     1. nohz_full cpu stop tick in tick_nohz_irq_exiit() while it runs
only 1 cfs task.
     2. nohz_full cpu which doesn't stop the tick and switches to idle task.

Without this commit, case (1) wouldn't participate in idle balance
when it switches to idle task while its tick is already stopped.

case (2) although nohz_full cpu participates in idle balcancing
because former clock isn't stopped yet.


> * How will related changelogs evolve further?
>

Thanks for the suggestion. But I'll add some more background commit
message then.

Thanks again :)

>> * How do you think about stress condition ordering concerns around
>>   the system configuration “nohz_full”?
>
> Well.. regardless of the stress condition, it wants to fix the
> inconsistent behavior
> happening when enter "idle state"

Were any special test cases or related analysis tools involved
in the discovery of improvable software behaviour?

Regards,
Markus