Re: [PATCH 3/3] cpufreq: schedutil: remove redundant code from sugov_next_freq_shared()

[Rafael J. Wysocki]

On Mon, Mar 6, 2017 at 5:45 AM, Viresh Kumar <viresh.kumar@xxxxxxxxxx> wrote:
> On 04-03-17, 01:11, Rafael J. Wysocki wrote:
>> 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.

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

We don't need to chase a pointer to get to the flags for the current
CPU (and same goes for util and max) and what if it is the last one in
the policy->cpus mask?

>> 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.

I'm not sure it will be really that cheap.

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

Well, it isn't IMO, unless you have numbers to support your point.

> 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.

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.

Thanks,
Rafael