Re: [PATCH v1] Revert "cpufreq: schedutil: Move max CPU capacity to sugov_policy"

From: Lukasz Luba
Date: Tue Nov 22 2022 - 03:59:24 EST


Hi Rafael and Sam

On 11/21/22 19:18, Rafael J. Wysocki wrote:
On Fri, Nov 18, 2022 at 2:00 AM Sam Wu <wusamuel@xxxxxxxxxx> wrote:

On Wed, Nov 16, 2022 at 3:35 AM Lukasz Luba <lukasz.luba@xxxxxxx> wrote:
Which mainline kernel version you use in pixel6?
I am using kernel version 6.1-rc5.

Could you elaborate a bit how is it possible?
Do you have the sg_policy setup properly (and at right time)?
Do you have the cpu capacity from arch_scale_cpu_capacity()
set correctly and at the right time during this cpufreq
governor setup?

IIRC in Android there is a different code for setting up the
cpufreq sched governor clones. In mainline we don't have to do
those tricks, so this might be the main difference.
This behavior is seen on the mainline kernel. There isn't any vendor code
modifying the behavior, and the schedutil governor is being used.

Could you trace the value that is read from
arch_scale_cpu_capacity() and share it with us?
I suspect this value changes in time in your kernel.
There's an additional CPU capacity normalization step during
init_cpu_capacity_callback() that does not happen until all the CPUs come
online. However, the sugov_start() function can be called for a subset of
CPUs before all the CPUs are brought up and before the normalization of
the CPU capacity values, so there could be a stale value stored
in sugov_policy.max field.

OK, the revert has been applied as 6.1-rc material, thanks!

I was on a business trip last week so couldn't check this.
Now I'm back and I've checked the booting sequence.
Yes, there is some race condition and the mechanism
using blocking_notifier_call_chain() in the cpufreq_online()
doesn't help while we are registering that schedutil
new policy.

I will have to go through those mechanisms and check them.
I agree, for now the best option is to revert the patch.

My apologies for introducing this issues.
Thanks Sam for capturing it.

Regards,
Lukasz