| Message ID | 20230901130312.247719-4-vincent.guittot@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | consolidate and cleanup CPU capacity | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/cover_letter | success | Series has a cover letter |
| conchuod/tree_selection | success | Guessed tree name to be for-next at HEAD 9a1d204f5c57 |
| conchuod/fixes_present | success | Fixes tag not required for -next series |
| conchuod/maintainers_pattern | success | MAINTAINERS pattern errors before the patch: 2 and now 2 |
| conchuod/verify_signedoff | success | Signed-off-by tag matches author and committer |
| conchuod/kdoc | fail | Errors and warnings before: 0 this patch: 2 |
| conchuod/build_rv64_clang_allmodconfig | success | Errors and warnings before: 9 this patch: 9 |
| conchuod/module_param | success | Was 0 now: 0 |
| conchuod/build_rv64_gcc_allmodconfig | success | Errors and warnings before: 9 this patch: 9 |
| conchuod/build_rv32_defconfig | success | Build OK |
| conchuod/dtb_warn_rv64 | success | Errors and warnings before: 39 this patch: 39 |
| conchuod/header_inline | success | No static functions without inline keyword in header files |
| conchuod/checkpatch | warning | CHECK: Please don't use multiple blank lines |
| conchuod/build_rv64_nommu_k210_defconfig | success | Build OK |
| conchuod/verify_fixes | success | No Fixes tag |
| conchuod/build_rv64_nommu_virt_defconfig | success | Build OK |
On Fri, Sep 01, 2023 at 03:03:11PM +0200, Vincent Guittot wrote: > +#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. Copy-pasting fail? That's not what your parameter is called. > + * > + * Return: the reference CPU frequency. > + */ > +static __always_inline > +unsigned long arch_scale_freq_ref_policy(struct cpufreq_policy *policy)
On Sat, 2 Sept 2023 at 12:57, Conor Dooley <conor@kernel.org> wrote: > > On Fri, Sep 01, 2023 at 03:03:11PM +0200, Vincent Guittot wrote: > > > +#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. > > Copy-pasting fail? That's not what your parameter is called. Yes, I forgot to change parameter and its description > > > + * > > + * Return: the reference CPU frequency. > > + */ > > +static __always_inline > > +unsigned long arch_scale_freq_ref_policy(struct cpufreq_policy *policy)
On Fri, Sep 01, 2023 at 03:03:11PM +0200, Vincent Guittot wrote: > +#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) > +{ > + return arch_scale_freq_ref(policy->cpu); > +} > +#else > +static __always_inline > +unsigned long arch_scale_freq_ref_policy(struct cpufreq_policy *policy) double space ^^ > +{ > + if (arch_scale_freq_invariant()) > + return policy->cpuinfo.max_freq; > + > + superfluous whitespace there. > + return policy->cur; > +} > +#endif static __always_inline unsigned long arch_scale_freq_ref_policy(struct cpufreq_policy *policy) { #ifdef arch_scale_freq_ref return arch_scale_freq_ref(policy->cpu); #endif if (arch_scale_freq_invariant()) return policy->cpuinfo.max_freq; return policy->cur; } Would have the lot in a single function and possibly easier to read? > + > /** > * 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; Leave the variable below per the inverse christmas thing. > > util = map_util_perf(util); > + freq = arch_scale_freq_ref_policy(policy); > freq = map_util_freq(util, freq, max); > > if (freq == sg_policy->cached_raw_freq && !sg_policy->need_freq_update) > -- > 2.34.1 >
On Tue, 5 Sept 2023 at 13:25, Peter Zijlstra <peterz@infradead.org> wrote: > > On Fri, Sep 01, 2023 at 03:03:11PM +0200, Vincent Guittot wrote: > > > +#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) > > +{ > > + return arch_scale_freq_ref(policy->cpu); > > +} > > +#else > > +static __always_inline > > +unsigned long arch_scale_freq_ref_policy(struct cpufreq_policy *policy) > > double space ^^ I was expecting checkpatch.pl to catch it will fix them > > > +{ > > + if (arch_scale_freq_invariant()) > > + return policy->cpuinfo.max_freq; > > + > > + > > superfluous whitespace there. > > > + return policy->cur; > > +} > > +#endif > > static __always_inline > unsigned long arch_scale_freq_ref_policy(struct cpufreq_policy *policy) > { > #ifdef arch_scale_freq_ref > return arch_scale_freq_ref(policy->cpu); > #endif > > if (arch_scale_freq_invariant()) > return policy->cpuinfo.max_freq; > > return policy->cur; > } > > Would have the lot in a single function and possibly easier to read? yes > > > + > > /** > > * 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; > > Leave the variable below per the inverse christmas thing. > > > > > util = map_util_perf(util); > > + freq = arch_scale_freq_ref_policy(policy); > > freq = map_util_freq(util, freq, max); > > > > if (freq == sg_policy->cached_raw_freq && !sg_policy->need_freq_update) > > -- > > 2.34.1 > > > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
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. > +{ > + 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. 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 > >
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 > > > >
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) +{ + return arch_scale_freq_ref(policy->cpu); +} +#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); freq = map_util_freq(util, freq, max); if (freq == sg_policy->cached_raw_freq && !sg_policy->need_freq_update)
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(-)