Messages in this thread | | | From | Vincent Guittot <> | Date | Mon, 25 Sep 2023 14:06:32 +0200 | Subject | Re: [PATCH 3/4] cpufreq/schedutil: use a fixed reference frequency |
| |
On Thu, 21 Sept 2023 at 11:19, Ionela Voinescu <ionela.voinescu@arm.com> wrote: > > On Friday 01 Sep 2023 at 15:03:11 (+0200), Vincent Guittot wrote: > > cpuinfo_max_freq can change at runtime because of boost as example. This > > implies that the value could not be the same than the one that has been > > used when computing the capacity of a CPU. > > > > The new arch_scale_freq_ref() returns a fixed and coherent frequency > > reference that can be used when computing a frequency based on utilization. > > > > Use this arch_scale_freq_ref() when available and fallback to > > cpuinfo.max_freq otherwise. > > > > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org> > > --- > > kernel/sched/cpufreq_schedutil.c | 29 +++++++++++++++++++++++++++-- > > 1 file changed, 27 insertions(+), 2 deletions(-) > > > > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c > > index 4492608b7d7f..9996ef429e2b 100644 > > --- a/kernel/sched/cpufreq_schedutil.c > > +++ b/kernel/sched/cpufreq_schedutil.c > > @@ -114,6 +114,31 @@ static void sugov_deferred_update(struct sugov_policy *sg_policy) > > } > > } > > > > +#ifdef arch_scale_freq_ref > > +/** > > + * arch_scale_freq_ref_policy - get the reference frequency of a given CPU that > > + * has been used to correlate frequency and compute capacity. > > + * @cpu: the CPU in question. > > + * > > + * Return: the reference CPU frequency. > > + */ > > +static __always_inline > > +unsigned long arch_scale_freq_ref_policy(struct cpufreq_policy *policy) > > This should not be an arch_ function as it's only a wrapper over an > arch_ function and not a function that different architectures might > implement differently usually in architecture specific code.
I expect this function to disappear at some point once all arch will use it that why I named it arch_* but I can rename it
> > > +{ > > + return arch_scale_freq_ref(policy->cpu); > > It might make it easier to read if arch_scale_freq_ref() had a default > implementation that returned 0.
I will use the suggestion made by Peter to have only one function
> > Then this code would simply become: > > freq = arch_scale_freq_ref(policy->cpu); > if (freq) > return freq; > else if (arch_scale_freq_invariant()) > return .. > .. > > This approach is similar to the use of arch_freq_get_on_cpu() in > cpufreq.c, and, as there, having a chosen maximum frequency of 0 would > not be a valid value. > > > +} > > +#else > > +static __always_inline > > +unsigned long arch_scale_freq_ref_policy(struct cpufreq_policy *policy) > > +{ > > + if (arch_scale_freq_invariant()) > > + return policy->cpuinfo.max_freq; > > + > > + > > + return policy->cur; > > +} > > +#endif > > + > > /** > > * get_next_freq - Compute a new frequency for a given cpufreq policy. > > * @sg_policy: schedutil policy object to compute the new frequency for. > > @@ -139,11 +164,11 @@ static void sugov_deferred_update(struct sugov_policy *sg_policy) > > static unsigned int get_next_freq(struct sugov_policy *sg_policy, > > unsigned long util, unsigned long max) > > { > > + unsigned int freq; > > struct cpufreq_policy *policy = sg_policy->policy; > > - unsigned int freq = arch_scale_freq_invariant() ? > > - policy->cpuinfo.max_freq : policy->cur; > > > > util = map_util_perf(util); > > + freq = arch_scale_freq_ref_policy(policy); > > Given its single use here, it would likely be better to place the code > above directly here, rather than create a wrapper over a few lines of > code. > > Hope it helps, > Ionela. > > > freq = map_util_freq(util, freq, max); > > > > if (freq == sg_policy->cached_raw_freq && !sg_policy->need_freq_update) > > -- > > 2.34.1 > > > >
| |