Re: [PATCH] cpuidle: Always stop scheduler tick on adaptive-tick CPUs

[Rafael J. Wysocki]

On Wed, Jul 17, 2019 at 3:21 PM Frederic Weisbecker <frederic@xxxxxxxxxx> 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@xxxxxxxxxx> wrote:
> > >
> > > On Tue, Jul 16, 2019 at 05:25:10PM +0200, Rafael J. Wysocki wrote:
> > > > From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> > > >
> > > > 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@xxxxxxxxx>
> > > > Tested-by: Thomas Lindroth <thomas.lindroth@xxxxxxxxx>
> > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> > > > ---
> > > >  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!