Messages in this thread | | | Subject | Re: [RFC PATCH v4 3/6] sched/cpufreq: Hook em_pd_get_higher_power() into get_next_freq() | From | Douglas Raillard <> | Date | Thu, 23 Jan 2020 17:52:53 +0000 |
| |
On 1/23/20 4:16 PM, Quentin Perret wrote: > On Wednesday 22 Jan 2020 at 17:35:35 (+0000), Douglas RAILLARD wrote: >> @@ -210,9 +211,16 @@ static unsigned int get_next_freq(struct sugov_policy *sg_policy, >> struct cpufreq_policy *policy = sg_policy->policy; >> unsigned int freq = arch_scale_freq_invariant() ? >> policy->cpuinfo.max_freq : policy->cur; >> + struct em_perf_domain *pd = sugov_policy_get_pd(sg_policy); >> >> freq = map_util_freq(util, freq, max); >> >> + /* >> + * Try to get a higher frequency if one is available, given the extra >> + * power we are ready to spend. >> + */ >> + freq = em_pd_get_higher_freq(pd, freq, 0); > > I find it sad that the call just below to cpufreq_driver_resolve_freq() > and cpufreq_frequency_table_target() iterates the OPPs all over again. > It's especially a shame since most existing users of the EM stuff do > have a cpufreq frequency table. > > Have you looked at hooking this inside cpufreq_driver_resolve_freq() > instead ? If we have a well-formed EM available, the call to > cpufreq_frequency_table_target() feels redundant, so we might want to > skip it.
We can't really move the call to em_pd_get_higher_freq() into cpufreq_driver_resolve_freq() since that's a schedutil-specific feature, and we would loose the !sg_policy->need_freq_update optimization.
Maybe we can add a flag to cpufreq_driver_resolve_freq() that promises that the frequency is already a valid one. We have to be careful though, since a number of things can make that untrue: - em_pd_get_higher_freq() will return the passed freq verbatim if it's higher than the max freq, so em_pd_get_higher_freq() will have to set the flag itself in case that logic changes. - policy limits can change the value - future things could tinker with the freq and forget to reset the flag.
If you think it's worth it I can make these changes.
> Thoughts ? > > Quentin >
Cheers, Douglas
| |