cupfreq: schedutil: Minor fix and cleanups

cached_raw_freq applies to the entire cpufreq policy and not individual CPUs. Apart from wasting per-cpu memory, it is actually wrong to keep it in struct sugov_cpu as we may end up comparing next_freq with a stale cached_raw_freq of a random CPU.

Move cached_raw_freq to struct sugov_policy.

Signed-off-by Viresh Kumar <[email protected]>

Quoted 8 lines · Viresh Kumar

cached_raw_freq applies to the entire cpufreq policy and not individual CPUs. Apart from wasting per-cpu memory, it is actually wrong to keep it in struct sugov_cpu as we may end up comparing next_freq with a stale cached_raw_freq of a random CPU.

Move cached_raw_freq to struct sugov_policy.

Signed-off-by Viresh Kumar <[email protected]>

Any chance for a Fixes: tag?

The same code is present both within and outside the loop and it doesn't look like it provides any additional benefit.

Quoted 2 lines · Viresh Kumar

The same code is present both within and outside the loop and it doesn't look like it provides any additional benefit.

Well, not quite. This is on purpose.

Note the "if (j == smp_processor_id())" condition within the loop and think about how the current CPU is taken into account. :-)

So one idea is that if SCHED_CPUFREQ_RT_DL is set in flags, we don't even need to start the loop which is quite a cost to simply notice that there's nothing to do.

Also I don't quite agree with adding an extra pair of integer multiplications to that loop just to get rid of the extra args.

Quoted 3 lines · Rafael J. Wysocki

So one idea is that if SCHED_CPUFREQ_RT_DL is set in flags, we don't even need to start the loop which is quite a cost to simply notice that there's nothing to do.

Hmm. Isn't the probability of this flag being set, same for all CPUs in the policy?

If yes, then why do we need to handle the current CPU specially?

Quoted 2 lines

Also I don't quite agree with adding an extra pair of integer multiplications to that loop just to get rid of the extra args.

But that should be cheap enough as we would be multiplying with 1 in one of them and with 0 on the other.

Isn't that better then keeping same code at two places?

Also as I mentioned in the commit log, the number of extra comparisons for the current CPU will be balanced if we have three CPUs in the policy and with every other CPU in the policy, we will end up doing one comparison less. With Quad-core policies, we reduce the number of comparisons by 1 and for octa-core ones, we reduce it by 5.

Quoted 7 lines · Viresh Kumar

Quoted 3 lines · Rafael J. Wysocki

So one idea is that if SCHED_CPUFREQ_RT_DL is set in flags, we don't even need to start the loop which is quite a cost to simply notice that there's nothing to do.

Hmm. Isn't the probability of this flag being set, same for all CPUs in the policy?

No, I don't think so.

So to the point, the code was written this way on purpose and not just by accident as your changelog suggests and

if you want to change it, you need numbers.

Quoted 6 lines · Rafael J. Wysocki

Quoted 5 lines · Viresh Kumar

Quoted 3 lines · Rafael J. Wysocki

So one idea is that if SCHED_CPUFREQ_RT_DL is set in flags, we don't even need to start the loop which is quite a cost to simply notice that there's nothing to do.

Quoted 4 lines

Quoted 2 lines

Hmm. Isn't the probability of this flag being set, same for all CPUs in the policy?

No, I don't think so.

Why do you think so? I thought all CPU in the policy can have the RT/DL flag set and the probability of all of them is just the same.

Quoted 2 lines

So to the point, the code was written this way on purpose and not just by accident as your changelog suggests and

I didn't wanted to convey that really and I knew that it was written on purpose.

Quoted 1 line

if you want to change it, you need numbers.

What kind of numbers can we get for such a change ? I tried to take the running average of the time it takes to execute this routine over 10000 samples, but it varies a lot even with the same build. Any tests like hackbench, etc wouldn't be of any help as well.

Quoted 2 lines · Viresh Kumar

Why do you think so? I thought all CPU in the policy can have the RT/DL flag set and the probability of all of them is just the same.

Well, yes, but if the current CPU has that flag set already, we surely don't need to check the other ones in the policy?

Quoted 7 lines

Quoted 2 lines

So to the point, the code was written this way on purpose and not just by accident as your changelog suggests and

I didn't wanted to convey that really and I knew that it was written on purpose.

Quoted 1 line

if you want to change it, you need numbers.

Quoted 4 lines

What kind of numbers can we get for such a change ? I tried to take the running average of the time it takes to execute this routine over 10000 samples, but it varies a lot even with the same build. Any tests like hackbench, etc wouldn't be of any help as well.

So why do you think it needs to be changed, but really?

Is that because it is particularly hard to follow or similar?

Quoted 6 lines · Rafael J. Wysocki

Quoted 2 lines · Viresh Kumar

Why do you think so? I thought all CPU in the policy can have the RT/DL flag set and the probability of all of them is just the same.

Well, yes, but if the current CPU has that flag set already, we surely don't need to check the other ones in the policy?

That's true for every other CPU in policy too..

Quoted 12 lines

Quoted 11 lines

Quoted 2 lines

So to the point, the code was written this way on purpose and not just by accident as your changelog suggests and

I didn't wanted to convey that really and I knew that it was written on purpose.

Quoted 1 line

if you want to change it, you need numbers.

What kind of numbers can we get for such a change ? I tried to take the running average of the time it takes to execute this routine over 10000 samples, but it varies a lot even with the same build. Any tests like hackbench, etc wouldn't be of any help as well.

Quoted 3 lines

So why do you think it needs to be changed, but really?

Is that because it is particularly hard to follow or similar?

Just that I didn't like keeping the same code at two places (outside and inside the loop) and the benefit it has.

Anyway, its not straight forward to get any numbers supporting my argument. I can claim improvement only theoretically by comparing the number of comparisons that we may end up doing for quad or octa core policies. Lets abandon this patch as I failed to convince you :)

Thanks for applying the other two patches though.

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.

But that would be a 4.12 change anyway.

Quoted 3 lines · Rafael J. Wysocki

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 ?

-------------------------8<-------------------------

 1 file changed, 9 insertions(+), 16 deletions(-)

kernel/sched/cpufreq_schedutil.c +9 -16

index 78468aa051ab..f5ffe241812e 100644
@@ -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);
        }
 

Quoted 1 line

But that would be a 4.12 change anyway.

Sure.