Messages in this thread | | | From | "Rafael J. Wysocki" <> | Date | Tue, 21 Mar 2017 13:30:37 +0100 | Subject | Re: [RFC][PATCH v2 2/2] cpufreq: schedutil: Avoid decreasing frequency of busy CPUs |
| |
On Tue, Mar 21, 2017 at 7:40 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote: > On 20-03-17, 22:46, Rafael J. Wysocki wrote: >> Index: linux-pm/kernel/sched/cpufreq_schedutil.c > >> +static void sugov_update_commit(struct sugov_cpu *sg_cpu, >> + struct sugov_policy *sg_policy, >> + u64 time, unsigned int next_freq) >> { >> struct cpufreq_policy *policy = sg_policy->policy; >> >> + if (sugov_cpu_is_busy(sg_cpu) && next_freq < sg_policy->next_freq) >> + next_freq = sg_policy->next_freq; >> + > > In the earlier version you said that we want to be opportunistic and > don't want to do heavy computation and so check only for current CPU. > > But in this version, all those computations are already done by now. > Why shouldn't we check all CPUs in the policy now? I am asking as we > will still have the same problem, we are trying to work-around if the > current CPU isn't busy but others sharing the policy are.
This isn't the way I'm looking at that.
This is an easy (and relatively cheap) check to make for the *current* *CPU* and our frequency selection algorithm turns out to have problems, so it would be kind of unreasonable to not use the opportunity to fix up the value coming from it - if we can do that easily enough.
For the other CPUs in the policy that would require extra synchronization etc., so not that easy any more.
> Also, why not return directly from within the if block? To run > trace_cpu_frequency()?
Yes.
> I don't remember exactly, but why don't we run that for !fast-switch > case?
That's an interesting question.
We do that in the fast switch case, because otherwise utilities get confused if the frequency is not updated for a long enough time.
I'm not really sure why they don't get confused in the other case, though. [In that case the core calls trace_cpu_frequency() for us, but only if we actually run the async work.]
It looks like it wouldn't hurt to always run trace_cpu_frequency() when we want to bail out early for next_freq == sg_policy->next_freq.
Let me prepare a patch for that. :-)
> We can simplify the code a bit if we check for no freq change at > the top of the routine.
Right.
Thanks, Rafael
| |