Re: [PATCH V3 3/9] cpufreq: Cap the default transition delay value to 10 ms

[Rafael J. Wysocki]

On Tuesday, July 25, 2017 02:54:46 PM Leonard Crestez wrote:
> On Wed, 2017-07-19 at 15:42 +0530, Viresh Kumar wrote:
> > If transition_delay_us isn't defined by the cpufreq driver, the default
> > value of transition delay (time after which the cpufreq governor will
> > try updating the frequency again) is currently calculated by multiplying
> > transition_latency (nsec) with LATENCY_MULTIPLIER (1000) and then
> > converting this time to usec. That gives the exact same value as
> > transition_latency, just that the time unit is usec instead of nsec.
> > 
> > With acpi-cpufreq for example, transition_latency is set to around 10
> > usec and we get transition delay as 10 ms. Which seems to be a
> > reasonable amount of time to reevaluate the frequency again.
> > 
> > But for platforms where frequency switching isn't that fast (like ARM),
> > the transition_latency varies from 500 usec to 3 ms, and the transition
> > delay becomes 500 ms to 3 seconds. Of course, that is a pretty bad
> > default value to start with.
> > 
> > We can try to come across a better formula (instead of multiplying with
> > LATENCY_MULTIPLIER) to solve this problem, but will that be worth it ?
> > 
> > This patch tries a simple approach and caps the maximum value of default
> > transition delay to 10 ms. Of course, userspace can still come in and
> > change this value anytime or individual drivers can rather provide
> > transition_delay_us instead.
> > 
> > Signed-off-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
> > ---
> >  drivers/cpufreq/cpufreq.c | 15 +++++++++++++--
> >  1 file changed, 13 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > index c426d21822f7..d00cde871c15 100644
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -532,8 +532,19 @@ unsigned int cpufreq_policy_transition_delay_us(struct cpufreq_policy *policy)
> >  		return policy->transition_delay_us;
> >  
> >  	latency = policy->cpuinfo.transition_latency / NSEC_PER_USEC;
> > -	if (latency)
> > -		return latency * LATENCY_MULTIPLIER;
> > +	if (latency) {
> > +		/*
> > +		 * For platforms that can change the frequency very fast (< 10
> > +		 * us), the above formula gives a decent transition delay. But
> > +		 * for platforms where transition_latency is in milliseconds, it
> > +		 * ends up giving unrealistic values.
> > +		 *
> > +		 * Cap the default transition delay to 10 ms, which seems to be
> > +		 * a reasonable amount of time after which we should reevaluate
> > +		 * the frequency.
> > +		 */
> > +		return min(latency * LATENCY_MULTIPLIER, (unsigned int)10000);
> > +	}
> >  
> >  	return LATENCY_MULTIPLIER;
> >  }
> 
> This patch made it's way into linux-next and it seems to cause imx socs
> to almost always hang around their max frequency with the ondemand
> governor, even when almost completely idle. The lowest frequency is
> never reached. This seems wrong?
> 
> This driver calculates transition_latency at probe time, the value is
> not terribly accurate but it reaches values like latency = 109 us, so
> this patch clamps it at about 10% of the value.
> 
> It's worth noting that the default IMX config has HZ=100 and
> NO_HZ_IDLE=y, so maybe doing idle checks at a rate comparable to the
> jiffie tick screws stuff up? I don't understand what ondemand is trying
> to do.

I've dropped this commit from linux-next for now.

Thanks,
Rafael