* [PATCH] cpuidle: Always stop scheduler tick on adaptive-tick CPUs
@ 2019-07-16 15:25 Rafael J. Wysocki
2019-07-16 21:40 ` Frederic Weisbecker
0 siblings, 1 reply; 6+ messages in thread
From: Rafael J. Wysocki @ 2019-07-16 15:25 UTC (permalink / raw)
To: Linux PM; +Cc: Thomas Lindroth, LKML, Peter Zijlstra, Frederic Weisbecker
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Running the scheduler tick on idle adaptive-tick CPUs is not useful
and it may also be not expected by users (as reported by Thomas), so
add a check to cpuidle_idle_call() to always stop the tick on them
regardless of the idle duration predicted by the governor.
Fixes: 554c8aa8ecad ("sched: idle: Select idle state before stopping the tick")
Reported-by: Thomas Lindroth <thomas.lindroth@gmail.com>
Tested-by: Thomas Lindroth <thomas.lindroth@gmail.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
kernel/sched/idle.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
Index: linux-pm/kernel/sched/idle.c
===================================================================
--- linux-pm.orig/kernel/sched/idle.c
+++ linux-pm/kernel/sched/idle.c
@@ -191,7 +191,8 @@ static void cpuidle_idle_call(void)
*/
next_state = cpuidle_select(drv, dev, &stop_tick);
- if (stop_tick || tick_nohz_tick_stopped())
+ if (stop_tick || tick_nohz_tick_stopped() ||
+ !housekeeping_cpu(dev->cpu, HK_FLAG_TICK))
tick_nohz_idle_stop_tick();
else
tick_nohz_idle_retain_tick();
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] cpuidle: Always stop scheduler tick on adaptive-tick CPUs
2019-07-16 15:25 [PATCH] cpuidle: Always stop scheduler tick on adaptive-tick CPUs Rafael J. Wysocki
@ 2019-07-16 21:40 ` Frederic Weisbecker
2019-07-17 7:55 ` Rafael J. Wysocki
0 siblings, 1 reply; 6+ messages in thread
From: Frederic Weisbecker @ 2019-07-16 21:40 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: Linux PM, Thomas Lindroth, LKML, Peter Zijlstra
On Tue, Jul 16, 2019 at 05:25:10PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Running the scheduler tick on idle adaptive-tick CPUs is not useful
Judging by the below change, you mean full dynticks, right?
> and it may also be not expected by users (as reported by Thomas), so
> add a check to cpuidle_idle_call() to always stop the tick on them
> regardless of the idle duration predicted by the governor.
>
> Fixes: 554c8aa8ecad ("sched: idle: Select idle state before stopping the tick")
> Reported-by: Thomas Lindroth <thomas.lindroth@gmail.com>
> Tested-by: Thomas Lindroth <thomas.lindroth@gmail.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> kernel/sched/idle.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> Index: linux-pm/kernel/sched/idle.c
> ===================================================================
> --- linux-pm.orig/kernel/sched/idle.c
> +++ linux-pm/kernel/sched/idle.c
> @@ -191,7 +191,8 @@ static void cpuidle_idle_call(void)
> */
> next_state = cpuidle_select(drv, dev, &stop_tick);
>
> - if (stop_tick || tick_nohz_tick_stopped())
> + if (stop_tick || tick_nohz_tick_stopped() ||
> + !housekeeping_cpu(dev->cpu, HK_FLAG_TICK))
But tick_nohz_tick_stopped() also works on full dynticks CPUs. If the
tick isn't stopped on a full dynticks CPU by the time we reach this path,
it means that the conditions for the tick to be stopped are not met anyway
(eg: more than one task and sched tick is needed, perf event requires the tick,
posix CPU timer, etc...)
Or am I missing something else?
Thanks.
> tick_nohz_idle_stop_tick();
> else
> tick_nohz_idle_retain_tick();
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] cpuidle: Always stop scheduler tick on adaptive-tick CPUs
2019-07-16 21:40 ` Frederic Weisbecker
@ 2019-07-17 7:55 ` Rafael J. Wysocki
2019-07-17 13:21 ` Frederic Weisbecker
0 siblings, 1 reply; 6+ messages in thread
From: Rafael J. Wysocki @ 2019-07-17 7:55 UTC (permalink / raw)
To: Frederic Weisbecker
Cc: Rafael J. Wysocki, Linux PM, Thomas Lindroth, LKML, Peter Zijlstra
On Tue, Jul 16, 2019 at 11:40 PM Frederic Weisbecker
<frederic@kernel.org> wrote:
>
> On Tue, Jul 16, 2019 at 05:25:10PM +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Running the scheduler tick on idle adaptive-tick CPUs is not useful
>
> Judging by the below change, you mean full dynticks, right?
Right.
> > and it may also be not expected by users (as reported by Thomas), so
> > add a check to cpuidle_idle_call() to always stop the tick on them
> > regardless of the idle duration predicted by the governor.
> >
> > Fixes: 554c8aa8ecad ("sched: idle: Select idle state before stopping the tick")
> > Reported-by: Thomas Lindroth <thomas.lindroth@gmail.com>
> > Tested-by: Thomas Lindroth <thomas.lindroth@gmail.com>
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> > kernel/sched/idle.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > Index: linux-pm/kernel/sched/idle.c
> > ===================================================================
> > --- linux-pm.orig/kernel/sched/idle.c
> > +++ linux-pm/kernel/sched/idle.c
> > @@ -191,7 +191,8 @@ static void cpuidle_idle_call(void)
> > */
> > next_state = cpuidle_select(drv, dev, &stop_tick);
> >
> > - if (stop_tick || tick_nohz_tick_stopped())
> > + if (stop_tick || tick_nohz_tick_stopped() ||
> > + !housekeeping_cpu(dev->cpu, HK_FLAG_TICK))
>
> But tick_nohz_tick_stopped() also works on full dynticks CPUs. If the
> tick isn't stopped on a full dynticks CPU by the time we reach this path,
> it means that the conditions for the tick to be stopped are not met anyway
> (eg: more than one task and sched tick is needed, perf event requires the tick,
> posix CPU timer, etc...)
First of all, according to Thomas, the patch does make a difference,
so evidently on his system(s) the full dynticks CPUs enter the idle
loop with running tick.
This means that, indeed, the conditions for the tick to be stopped
have not been met up to that point, but if the (full dynticks) CPU
becomes idle, that's because it has been made idle on purpose
(presumably by a user-space "orchestrator" or the sysadmin), so the
kernel can assume that it will remain idle indefinitely. That, in
turn, is when the tick would be stopped on it regardless of everything
else (even if it wasn't a full dynticks CPU).
I guess I should add the above to the changelog.
> Or am I missing something else?
Well, if full dynticks CPUs are expected to always enter the idle loop
with stopped tick, then something appears to be amiss, but I'm not
sure if that expectation is entirely realistic.
Cheers!
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] cpuidle: Always stop scheduler tick on adaptive-tick CPUs
2019-07-17 7:55 ` Rafael J. Wysocki
@ 2019-07-17 13:21 ` Frederic Weisbecker
2019-07-17 22:27 ` Rafael J. Wysocki
0 siblings, 1 reply; 6+ messages in thread
From: Frederic Weisbecker @ 2019-07-17 13:21 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Rafael J. Wysocki, Linux PM, Thomas Lindroth, LKML, Peter Zijlstra
On Wed, Jul 17, 2019 at 09:55:08AM +0200, Rafael J. Wysocki wrote:
> On Tue, Jul 16, 2019 at 11:40 PM Frederic Weisbecker
> <frederic@kernel.org> wrote:
> >
> > On Tue, Jul 16, 2019 at 05:25:10PM +0200, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >
> > > Running the scheduler tick on idle adaptive-tick CPUs is not useful
> >
> > Judging by the below change, you mean full dynticks, right?
>
> Right.
>
> > > and it may also be not expected by users (as reported by Thomas), so
> > > add a check to cpuidle_idle_call() to always stop the tick on them
> > > regardless of the idle duration predicted by the governor.
> > >
> > > Fixes: 554c8aa8ecad ("sched: idle: Select idle state before stopping the tick")
> > > Reported-by: Thomas Lindroth <thomas.lindroth@gmail.com>
> > > Tested-by: Thomas Lindroth <thomas.lindroth@gmail.com>
> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > ---
> > > kernel/sched/idle.c | 3 ++-
> > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > Index: linux-pm/kernel/sched/idle.c
> > > ===================================================================
> > > --- linux-pm.orig/kernel/sched/idle.c
> > > +++ linux-pm/kernel/sched/idle.c
> > > @@ -191,7 +191,8 @@ static void cpuidle_idle_call(void)
> > > */
> > > next_state = cpuidle_select(drv, dev, &stop_tick);
> > >
> > > - if (stop_tick || tick_nohz_tick_stopped())
> > > + if (stop_tick || tick_nohz_tick_stopped() ||
> > > + !housekeeping_cpu(dev->cpu, HK_FLAG_TICK))
> >
> > But tick_nohz_tick_stopped() also works on full dynticks CPUs. If the
> > tick isn't stopped on a full dynticks CPU by the time we reach this path,
> > it means that the conditions for the tick to be stopped are not met anyway
> > (eg: more than one task and sched tick is needed, perf event requires the tick,
> > posix CPU timer, etc...)
>
> First of all, according to Thomas, the patch does make a difference,
> so evidently on his system(s) the full dynticks CPUs enter the idle
> loop with running tick.
>
> This means that, indeed, the conditions for the tick to be stopped
> have not been met up to that point, but if the (full dynticks) CPU
> becomes idle, that's because it has been made idle on purpose
> (presumably by a user-space "orchestrator" or the sysadmin), so the
> kernel can assume that it will remain idle indefinitely. That, in
> turn, is when the tick would be stopped on it regardless of everything
> else (even if it wasn't a full dynticks CPU).
Well I think we disagree on that assumption that if a nohz_full CPU is put
idle, it will remain there indefinitely. Nohz_full CPUs aren't really special
in this regard, they can sleep on an IO, wait for a short event just like
any other CPU.
The only difference with other CPUs is that they _might_ enter the idle loop
with the tick already stopped. Ok that should be the case most of the time
with regular full dynticks usecases, but there can be initialization work
or stuff that make the CPU run with periodic tick for some time.
Thanks.
>
> I guess I should add the above to the changelog.
>
> > Or am I missing something else?
>
> Well, if full dynticks CPUs are expected to always enter the idle loop
> with stopped tick, then something appears to be amiss, but I'm not
> sure if that expectation is entirely realistic.
>
> Cheers!
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] cpuidle: Always stop scheduler tick on adaptive-tick CPUs
2019-07-17 13:21 ` Frederic Weisbecker
@ 2019-07-17 22:27 ` Rafael J. Wysocki
2019-07-18 0:08 ` Frederic Weisbecker
0 siblings, 1 reply; 6+ messages in thread
From: Rafael J. Wysocki @ 2019-07-17 22:27 UTC (permalink / raw)
To: Frederic Weisbecker
Cc: Rafael J. Wysocki, Rafael J. Wysocki, Linux PM, Thomas Lindroth,
LKML, Peter Zijlstra
On Wed, Jul 17, 2019 at 3:21 PM Frederic Weisbecker <frederic@kernel.org> wrote:
>
> On Wed, Jul 17, 2019 at 09:55:08AM +0200, Rafael J. Wysocki wrote:
> > On Tue, Jul 16, 2019 at 11:40 PM Frederic Weisbecker
> > <frederic@kernel.org> wrote:
> > >
> > > On Tue, Jul 16, 2019 at 05:25:10PM +0200, Rafael J. Wysocki wrote:
> > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > >
> > > > Running the scheduler tick on idle adaptive-tick CPUs is not useful
> > >
> > > Judging by the below change, you mean full dynticks, right?
> >
> > Right.
> >
> > > > and it may also be not expected by users (as reported by Thomas), so
> > > > add a check to cpuidle_idle_call() to always stop the tick on them
> > > > regardless of the idle duration predicted by the governor.
> > > >
> > > > Fixes: 554c8aa8ecad ("sched: idle: Select idle state before stopping the tick")
> > > > Reported-by: Thomas Lindroth <thomas.lindroth@gmail.com>
> > > > Tested-by: Thomas Lindroth <thomas.lindroth@gmail.com>
> > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > ---
> > > > kernel/sched/idle.c | 3 ++-
> > > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > Index: linux-pm/kernel/sched/idle.c
> > > > ===================================================================
> > > > --- linux-pm.orig/kernel/sched/idle.c
> > > > +++ linux-pm/kernel/sched/idle.c
> > > > @@ -191,7 +191,8 @@ static void cpuidle_idle_call(void)
> > > > */
> > > > next_state = cpuidle_select(drv, dev, &stop_tick);
> > > >
> > > > - if (stop_tick || tick_nohz_tick_stopped())
> > > > + if (stop_tick || tick_nohz_tick_stopped() ||
> > > > + !housekeeping_cpu(dev->cpu, HK_FLAG_TICK))
> > >
> > > But tick_nohz_tick_stopped() also works on full dynticks CPUs. If the
> > > tick isn't stopped on a full dynticks CPU by the time we reach this path,
> > > it means that the conditions for the tick to be stopped are not met anyway
> > > (eg: more than one task and sched tick is needed, perf event requires the tick,
> > > posix CPU timer, etc...)
> >
> > First of all, according to Thomas, the patch does make a difference,
> > so evidently on his system(s) the full dynticks CPUs enter the idle
> > loop with running tick.
> >
> > This means that, indeed, the conditions for the tick to be stopped
> > have not been met up to that point, but if the (full dynticks) CPU
> > becomes idle, that's because it has been made idle on purpose
> > (presumably by a user-space "orchestrator" or the sysadmin), so the
> > kernel can assume that it will remain idle indefinitely. That, in
> > turn, is when the tick would be stopped on it regardless of everything
> > else (even if it wasn't a full dynticks CPU).
>
> Well I think we disagree on that assumption that if a nohz_full CPU is put
> idle, it will remain there indefinitely. Nohz_full CPUs aren't really special
> in this regard, they can sleep on an IO, wait for a short event just like
> any other CPU.
Fair enough.
This means that the governor (or rather governors) will need to be
modified to address the issue reported by Thomas.
Fortunately, I have a patch going in that direction too. :-)
Cheers!
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] cpuidle: Always stop scheduler tick on adaptive-tick CPUs
2019-07-17 22:27 ` Rafael J. Wysocki
@ 2019-07-18 0:08 ` Frederic Weisbecker
0 siblings, 0 replies; 6+ messages in thread
From: Frederic Weisbecker @ 2019-07-18 0:08 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Rafael J. Wysocki, Linux PM, Thomas Lindroth, LKML, Peter Zijlstra
On Thu, Jul 18, 2019 at 12:27:09AM +0200, Rafael J. Wysocki wrote:
> On Wed, Jul 17, 2019 at 3:21 PM Frederic Weisbecker <frederic@kernel.org> wrote:
> >
> > On Wed, Jul 17, 2019 at 09:55:08AM +0200, Rafael J. Wysocki wrote:
> > Well I think we disagree on that assumption that if a nohz_full CPU is put
> > idle, it will remain there indefinitely. Nohz_full CPUs aren't really special
> > in this regard, they can sleep on an IO, wait for a short event just like
> > any other CPU.
>
> Fair enough.
>
> This means that the governor (or rather governors) will need to be
> modified to address the issue reported by Thomas.
>
> Fortunately, I have a patch going in that direction too. :-)
Good to hear, please also Cc me on that one, thanks!
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-07-18 0:08 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-16 15:25 [PATCH] cpuidle: Always stop scheduler tick on adaptive-tick CPUs Rafael J. Wysocki
2019-07-16 21:40 ` Frederic Weisbecker
2019-07-17 7:55 ` Rafael J. Wysocki
2019-07-17 13:21 ` Frederic Weisbecker
2019-07-17 22:27 ` Rafael J. Wysocki
2019-07-18 0:08 ` Frederic Weisbecker
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).