When nohz_full CPU stops tick in tick_nohz_irq_exit(),
It wouldn't be chosen to perform idle load balancing bacause 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 is part of nohz.idle_cpus_mask
with rq->nohz_tick_stopped.
So, nohz_balance_enter_idle() could be called safely without !was_stooped
check.
Signed-off-by: Levi Yun <[email protected]>
---
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 bacause it doesn't
…
> So, nohz_balance_enter_idle() could be called safely without !was_stooped
> check.
* Please avoid typos in this change description.
* How do you think about to add the tag “Fixes”?
* Would you like to use imperative wordings for an improved changelog?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.9-rc7#n94
Regards,
Markus
When nohz_full CPU stops tick in tick_nohz_irq_exit(),
It wouldn't be chosen to perform idle load balancing bacause 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 is part of nohz.idle_cpus_mask
with rq->nohz_tick_stopped.
So, nohz_balance_enter_idle() could be called safely without !was_stooped
check.
Fixes: 0e7767687fda ("time: tick-sched: Reorganize idle tick management code")
Signed-off-by: Levi Yun <[email protected]>
---
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 bacause it doesn't
…
> So, nohz_balance_enter_idle() could be called safely without !was_stooped
> check.
* Why did you repeat typos in this patch iteration?
* Would you like to take the relevance of imperative wordings for changelogs
better into account?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.9-rc7#n94
> ---
> kernel/time/tick-sched.c | 6 ++++--
Will further patch version descriptions become helpful behind the marker line?
Regards,
Markus
Hi Markus.
> * Why did you repeat typos in this patch iteration?
> * Would you like to take the relevance of imperative wordings for changelogs
better into account?
Sorry for my mistake, I've fixed it but, I sent an old one :(
Thanks
On Wed, May 8, 2024 at 7:38 PM Markus Elfring <[email protected]> wrote:
>
> > When nohz_full CPU stops tick in tick_nohz_irq_exit(),
> > It wouldn't be chosen to perform idle load balancing bacause it doesn't
> …
> > So, nohz_balance_enter_idle() could be called safely without !was_stooped
> > check.
>
> * Why did you repeat typos in this patch iteration?
>
> * Would you like to take the relevance of imperative wordings for changelogs
> better into account?
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.9-rc7#n94
>
>
> > ---
> > kernel/time/tick-sched.c | 6 ++++--
>
> Will further patch version descriptions become helpful behind the marker line?
>
> Regards,
> Markus
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 <[email protected]>
---
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 :)
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.
Formerly, __tick_nohz_idle_enter() is called in both
tick_nohz_irq_exit() and in do_idle().
That's why commit a0db971e4eb6 ("nohz: Move idle balancer registration
to the idle path") prevents nohz_full cpu which isn't yet
idle state but tick is stopped from entering idle balance.
However, this prevents nohz_full cpu which already stops tick from
entering idle balacne when this cpu really becomes idle state.
Currently, tick_nohz_idle_stop_tick() is only called in idle state and
it calls nohz_balance_enter_idle(). this function tracks the CPU
which is part of nohz.idle_cpus_mask with rq->nohz_tick_stopped properly.
Therefore, 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.
Fixes: a0db971e4eb6 ("nohz: Move idle balancer registration to the idle path")
Signed-off-by: Levi Yun <[email protected]>
---
v4:
- Add fixes tags.
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
>> * 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
…
> Currently, tick_nohz_idle_stop_tick() is only called in idle state and
> it calls nohz_balance_enter_idle(). this function tracks the CPU
> which is part of nohz.idle_cpus_mask with rq->nohz_tick_stopped properly.
…
* How did you notice improvement possibilities for such data processing?
* I hope that a few remaining wording “weaknesses” can be adjusted accordingly.
Regards,
Markus
Markus, kernel-janitors doesn't need to be a part of these discussions.
Could you please stop adding us to the CC list?
regards,
dan carpenter