Messages in this thread | | | From | "Rafael J. Wysocki" <> | Date | Wed, 8 Mar 2017 13:54:30 +0100 | Subject | Re: [PATCH 3/3] cpufreq: schedutil: remove redundant code from sugov_next_freq_shared() |
| |
On Wed, Mar 8, 2017 at 12:15 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote: > On 08-03-17, 11:50, Rafael J. Wysocki wrote: >> So overall, maybe you can move the flags check to >> sugov_update_shared(), so that you don't need to pass flags to >> sugov_next_freq_shared(), and then do what you did to util and max. > > Just to confirm, below is what you are suggesting ?
Yes, it is.
> -------------------------8<------------------------- > > 1 file changed, 9 insertions(+), 16 deletions(-) > > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c > index 78468aa051ab..f5ffe241812e 100644 > --- a/kernel/sched/cpufreq_schedutil.c > +++ b/kernel/sched/cpufreq_schedutil.c > @@ -217,30 +217,19 @@ static void sugov_update_single(struct update_util_data *hook, u64 time, > sugov_update_commit(sg_policy, time, next_f); > } > > -static unsigned int sugov_next_freq_shared(struct sugov_cpu *sg_cpu, > - unsigned long util, unsigned long max, > - unsigned int flags) > +static unsigned int sugov_next_freq_shared(struct sugov_cpu *sg_cpu) > { > struct sugov_policy *sg_policy = sg_cpu->sg_policy; > struct cpufreq_policy *policy = sg_policy->policy; > - unsigned int max_f = policy->cpuinfo.max_freq; > u64 last_freq_update_time = sg_policy->last_freq_update_time; > + unsigned long util = 0, max = 1; > unsigned int j; > > - if (flags & SCHED_CPUFREQ_RT_DL) > - return max_f; > - > - sugov_iowait_boost(sg_cpu, &util, &max); > - > for_each_cpu(j, policy->cpus) { > - struct sugov_cpu *j_sg_cpu; > + struct sugov_cpu *j_sg_cpu = &per_cpu(sugov_cpu, j); > unsigned long j_util, j_max; > s64 delta_ns; > > - if (j == smp_processor_id()) > - continue; > - > - j_sg_cpu = &per_cpu(sugov_cpu, j); > /* > * If the CPU utilization was last updated before the previous > * frequency update and the time elapsed between the last update > @@ -254,7 +243,7 @@ static unsigned int sugov_next_freq_shared(struct sugov_cpu *sg_cpu, > continue; > } > if (j_sg_cpu->flags & SCHED_CPUFREQ_RT_DL) > - return max_f; > + return policy->cpuinfo.max_freq; > > j_util = j_sg_cpu->util; > j_max = j_sg_cpu->max; > @@ -289,7 +278,11 @@ static void sugov_update_shared(struct update_util_data *hook, u64 time, > sg_cpu->last_update = time; > > if (sugov_should_update_freq(sg_policy, time)) { > - next_f = sugov_next_freq_shared(sg_cpu, util, max, flags); > + if (flags & SCHED_CPUFREQ_RT_DL) > + next_f = sg_policy->policy->cpuinfo.max_freq; > + else > + next_f = sugov_next_freq_shared(sg_cpu); > + > sugov_update_commit(sg_policy, time, next_f); > } > >> But that would be a 4.12 change anyway. > > Sure.
And IMO the subject/changelog should not talk about "redundant code", because the code in question is not in fact redundant, but about refactoring the code to eliminate one conditional from the for_each_cpu() loop and to make that loop treat all CPUs in the same way (more symmetrically).
Thanks, Rafael
| |