Message-ID: <20201201152224.GU3371@techsingularity.net>
Date:   Tue, 1 Dec 2020 15:22:24 +0000
From:   Mel Gorman <mgorman@...hsingularity.net>
To:     "Rafael J. Wysocki" <rafael@...nel.org>
Cc:     Marcelo Tosatti <mtosatti@...hat.com>,
        Daniel Lezcano <daniel.lezcano@...aro.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linux PM <linux-pm@...r.kernel.org>
Subject: Re: [PATCH] cpuidle: Select polling interval based on a c-state with
 a longer target residency

On Tue, Dec 01, 2020 at 04:08:02PM +0100, Rafael J. Wysocki wrote:
> > > Also this is about certain drivers only which support the "polling
> > > idle state" (the ACPI one and intel_idle only AFAICS).  So I'm not
> > > sure about the framework-level tunable here.
> > >
> > > Moreover, to be precise, that value is the maximum time to do the
> > > polling (in one go) in the case when requesting any "physical" idle
> > > states is likely to hurt energy-efficiency or latency.  In particular,
> > > it doesn't mean that idle CPUs will do the idle polling every time.
> > >
> >
> > At first I was nodding along and thinking "sure". Then I started
> > thinking about what the configuration space then looks like and how a
> > user might reasonably interpret it. You were right during the review of
> > the first version, it's a mess because it's driver specific and difficult
> > to interpret even on a per-driver basis because there is no control of
> > when a rescheduling event may occur.
> 
> Indeed.
> 
> > You suggest making poll=0 would be valid but that might be interpreted
> > as being equivalent to idle=poll on x86 which is not the same thing.
> > processor_idle and intel_idle would have understandable semantics if the
> > parameter was maxpoll but it's not as understandable for haltpoll.
> 
> Well, my point was basically that if the plan was to add a boot
> parameter to control the polling behavior, it would be prudent to also
> allow the admin to specify that they didn't want any polling at all.
> 
> But frankly I was hoping to drive you away from that idea which seems
> to have worked. :-)
> 

Yes, it most certainly worked. Thanks for repeating yourself in a different
way so that your concern could penetrate my thick skull :D

> > Finally, the parameter partially ties us into the current
> > implementation. For example, the polling loop is based on clock time but
> > we know looking up the clock is costly in itself so it's very granular
> > based on the magic "check every 200 loops" logic meaning we can go over
> > the expected maxiumum polling inverval. If we ever changed that into a
> > calibration loop to estimate the number of loops then the polling interval
> > changes slightly even for the same parameter as we no longer depend on the
> > granularity of calling local_clock. If we ever decided to use adaptive
> > polling similar to haltpoll then the behaviour changes again resulting
> > in bugs because the driver.poll parameter means something new.
> 
> Right.
> 
> > Using min_cstate was definitely a hazard because it showed up in both
> > microbenchmarks and real workloads but you were right, lets only
> > introduce a tunable when and if there is no other choice in the matter.
> >
> > So, informally the following patch is the next candidate. I'm happy to
> > resend it as a separate mail if you prefer and think the patch is ok.
> 
> I actually can apply it right away, so no need to resend.
> 

Thanks very much.

-- 
Mel Gorman
SUSE Labs

Powered by blists - more mailing lists