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