Re: [PATCH v2 4/7] cpufreq: report whether cpufreq supports Frequency Invariance (FI)

[Ionela Voinescu <ionela.voinescu@arm.com>]

Hi Viresh,

On Thursday 30 Jul 2020 at 10:13:46 (+0530), Viresh Kumar wrote:
> On 22-07-20, 10:37, Ionela Voinescu wrote:
> > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > index 3497c1cd6818..1d0b046fe8e9 100644
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -61,6 +61,9 @@ static struct cpufreq_driver *cpufreq_driver;
> >  static DEFINE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_data);
> >  static DEFINE_RWLOCK(cpufreq_driver_lock);
> >  
> > +/* Mark support for the scheduler's frequency invariance engine */
> > +static DEFINE_STATIC_KEY_FALSE(cpufreq_set_freq_scale);
> > +
> >  /* Flag to suspend/resume CPUFreq governors */
> >  static bool cpufreq_suspended;
> >  
> > @@ -69,6 +72,25 @@ static inline bool has_target(void)
> >  	return cpufreq_driver->target_index || cpufreq_driver->target;
> >  }
> >  
> > +static inline
> > +void enable_cpufreq_freq_invariance(struct cpufreq_driver *driver)
> > +{
> > +	if ((driver->target || driver->target_index || driver->fast_switch) &&
> > +	    !driver->setpolicy) {
> 
> Just checking for !driver->setpolicy should be enough here.
> 

Right, cpufreq_register_driver() should check that at least one of them
is present (although currently cpufreq_register_driver() will return
-EINVAL if .fast_switch() alone is present - something to be fixed).

Will do, on both accounts.


> > +		static_branch_enable_cpuslocked(&cpufreq_set_freq_scale);
> > +		pr_debug("%s: Driver %s can provide frequency invariance.",
> > +			 __func__, driver->name);
> 
> I think a simpler print will work well too.
> 
>                 pr_debug("Freq invariance enabled");
> 

I think the right way of reporting this support is important here.

By enabling the static key here, we're not actually enabling frequency
invariance. So printing "Freq invariance enabled" would be very
misleading. Frequency invariance (scheduler concept) being truly enabled
depends on three things:
 - Having a source of information for current frequency and maximum
   frequency (cpufreq, counters)
 - Having arch support for using that information to set the frequency
   scale factor: arch_set_freq_scale(), arch_scale_freq_tick()
 - Having arch support for passing the set frequency scale factor to the
   scheduler and reporting support for frequency invariance:
   arch_scale_freq_capacity(), arch_scale_freq_invariant().

Therefore, cpufreq can only report that the current driver can be a
source of information for frequency invariance "Driver %s can provide
frequency invariance", but it can't guarantee that the other conditions
are accomplished.

So I would recommend to keep this original debug message.

> __func__ isn't really required as this is the only print with that
> kind of info in cpufreq.c.
>

Makes sense!

> > +	} else
> > +		pr_err("%s: Driver %s cannot provide frequency invariance.",
> > +		__func__, driver->name);
> 
> Why not supporting freq-invariance an error ? I will just drop this
> message completely.

Yes, an error does not make sense here. I was thinking to demote it to a
warning instead in my previous reply to Rafael, but removing it
altogether might be better.

Many thanks for the thorough review,
Ionela.

> 
> -- 
> viresh