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

  This reverts commit 6d5afdc97ea71958287364a1f1d07e59ef151b11.

On a Pixel 6 device, it is observed that this commit increases
latency by approximately 50ms, or 20%, in migrating a task
that requires full CPU utilization from a LITTLE CPU to Fmax
on a big CPU. Reverting this change restores the latency back
to its original baseline value.

Fixes: 6d5afdc97ea7 ("cpufreq: schedutil: Move max CPU capacity to sugov_policy")
Cc: Lukasz Luba <lukasz.luba@arm.com>
Cc: Saravana Kannan <saravanak@google.com>
Cc: Isaac J. Manjarres <isaacmanjarres@google.com>
Signed-off-by: Sam Wu <wusamuel@google.com>
---
 kernel/sched/cpufreq_schedutil.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)
  

On Thu, Nov 10, 2022 at 11:57 AM Sam Wu <wusamuel@google.com> wrote:
>
> This reverts commit 6d5afdc97ea71958287364a1f1d07e59ef151b11.
>
> On a Pixel 6 device, it is observed that this commit increases
> latency by approximately 50ms, or 20%, in migrating a task
> that requires full CPU utilization from a LITTLE CPU to Fmax
> on a big CPU. Reverting this change restores the latency back
> to its original baseline value.
>
> Fixes: 6d5afdc97ea7 ("cpufreq: schedutil: Move max CPU capacity to sugov_policy")
> Cc: Lukasz Luba <lukasz.luba@arm.com>
> Cc: Saravana Kannan <saravanak@google.com>
> Cc: Isaac J. Manjarres <isaacmanjarres@google.com>
> Signed-off-by: Sam Wu <wusamuel@google.com>

Rafael, can we pick this up please?

Lukasz, no objections to the idea itself, but it's causing regression
and we'd like to revert and then fix it.

-Saravana

> ---
>  kernel/sched/cpufreq_schedutil.c | 30 +++++++++++++++---------------
>  1 file changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> index 9161d1136d01..1207c78f85c1 100644
> --- a/kernel/sched/cpufreq_schedutil.c
> +++ b/kernel/sched/cpufreq_schedutil.c
> @@ -25,9 +25,6 @@ struct sugov_policy {
>         unsigned int            next_freq;
>         unsigned int            cached_raw_freq;
>
> -       /* max CPU capacity, which is equal for all CPUs in freq. domain */
> -       unsigned long           max;
> -
>         /* The next fields are only needed if fast switch cannot be used: */
>         struct                  irq_work irq_work;
>         struct                  kthread_work work;
> @@ -51,6 +48,7 @@ struct sugov_cpu {
>
>         unsigned long           util;
>         unsigned long           bw_dl;
> +       unsigned long           max;
>
>         /* The field below is for single-CPU policies only: */
>  #ifdef CONFIG_NO_HZ_COMMON
> @@ -160,6 +158,7 @@ static void sugov_get_util(struct sugov_cpu *sg_cpu)
>  {
>         struct rq *rq = cpu_rq(sg_cpu->cpu);
>
> +       sg_cpu->max = arch_scale_cpu_capacity(sg_cpu->cpu);
>         sg_cpu->bw_dl = cpu_bw_dl(rq);
>         sg_cpu->util = effective_cpu_util(sg_cpu->cpu, cpu_util_cfs(sg_cpu->cpu),
>                                           FREQUENCY_UTIL, NULL);
> @@ -254,7 +253,6 @@ static void sugov_iowait_boost(struct sugov_cpu *sg_cpu, u64 time,
>   */
>  static void sugov_iowait_apply(struct sugov_cpu *sg_cpu, u64 time)
>  {
> -       struct sugov_policy *sg_policy = sg_cpu->sg_policy;
>         unsigned long boost;
>
>         /* No boost currently required */
> @@ -282,8 +280,7 @@ static void sugov_iowait_apply(struct sugov_cpu *sg_cpu, u64 time)
>          * sg_cpu->util is already in capacity scale; convert iowait_boost
>          * into the same scale so we can compare.
>          */
> -       boost = sg_cpu->iowait_boost * sg_policy->max;
> -       boost >>= SCHED_CAPACITY_SHIFT;
> +       boost = (sg_cpu->iowait_boost * sg_cpu->max) >> SCHED_CAPACITY_SHIFT;
>         boost = uclamp_rq_util_with(cpu_rq(sg_cpu->cpu), boost, NULL);
>         if (sg_cpu->util < boost)
>                 sg_cpu->util = boost;
> @@ -340,7 +337,7 @@ static void sugov_update_single_freq(struct update_util_data *hook, u64 time,
>         if (!sugov_update_single_common(sg_cpu, time, flags))
>                 return;
>
> -       next_f = get_next_freq(sg_policy, sg_cpu->util, sg_policy->max);
> +       next_f = get_next_freq(sg_policy, sg_cpu->util, sg_cpu->max);
>         /*
>          * Do not reduce the frequency if the CPU has not been idle
>          * recently, as the reduction is likely to be premature then.
> @@ -376,7 +373,6 @@ static void sugov_update_single_perf(struct update_util_data *hook, u64 time,
>                                      unsigned int flags)
>  {
>         struct sugov_cpu *sg_cpu = container_of(hook, struct sugov_cpu, update_util);
> -       struct sugov_policy *sg_policy = sg_cpu->sg_policy;
>         unsigned long prev_util = sg_cpu->util;
>
>         /*
> @@ -403,8 +399,7 @@ static void sugov_update_single_perf(struct update_util_data *hook, u64 time,
>                 sg_cpu->util = prev_util;
>
>         cpufreq_driver_adjust_perf(sg_cpu->cpu, map_util_perf(sg_cpu->bw_dl),
> -                                  map_util_perf(sg_cpu->util),
> -                                  sg_policy->max);
> +                                  map_util_perf(sg_cpu->util), sg_cpu->max);
>
>         sg_cpu->sg_policy->last_freq_update_time = time;
>  }
> @@ -413,19 +408,25 @@ static unsigned int sugov_next_freq_shared(struct sugov_cpu *sg_cpu, u64 time)
>  {
>         struct sugov_policy *sg_policy = sg_cpu->sg_policy;
>         struct cpufreq_policy *policy = sg_policy->policy;
> -       unsigned long util = 0;
> +       unsigned long util = 0, max = 1;
>         unsigned int j;
>
>         for_each_cpu(j, policy->cpus) {
>                 struct sugov_cpu *j_sg_cpu = &per_cpu(sugov_cpu, j);
> +               unsigned long j_util, j_max;
>
>                 sugov_get_util(j_sg_cpu);
>                 sugov_iowait_apply(j_sg_cpu, time);
> +               j_util = j_sg_cpu->util;
> +               j_max = j_sg_cpu->max;
>
> -               util = max(j_sg_cpu->util, util);
> +               if (j_util * max > j_max * util) {
> +                       util = j_util;
> +                       max = j_max;
> +               }
>         }
>
> -       return get_next_freq(sg_policy, util, sg_policy->max);
> +       return get_next_freq(sg_policy, util, max);
>  }
>
>  static void
> @@ -751,7 +752,7 @@ static int sugov_start(struct cpufreq_policy *policy)
>  {
>         struct sugov_policy *sg_policy = policy->governor_data;
>         void (*uu)(struct update_util_data *data, u64 time, unsigned int flags);
> -       unsigned int cpu = cpumask_first(policy->cpus);
> +       unsigned int cpu;
>
>         sg_policy->freq_update_delay_ns = sg_policy->tunables->rate_limit_us * NSEC_PER_USEC;
>         sg_policy->last_freq_update_time        = 0;
> @@ -759,7 +760,6 @@ static int sugov_start(struct cpufreq_policy *policy)
>         sg_policy->work_in_progress             = false;
>         sg_policy->limits_changed               = false;
>         sg_policy->cached_raw_freq              = 0;
> -       sg_policy->max                          = arch_scale_cpu_capacity(cpu);
>
>         sg_policy->need_freq_update = cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS);
>
> --
> 2.38.1.431.g37b22c650d-goog
>
  

Hi Sam,

On 11/10/22 19:57, Sam Wu wrote:
> This reverts commit 6d5afdc97ea71958287364a1f1d07e59ef151b11.
> 
> On a Pixel 6 device, it is observed that this commit increases
> latency by approximately 50ms, or 20%, in migrating a task
> that requires full CPU utilization from a LITTLE CPU to Fmax
> on a big CPU. Reverting this change restores the latency back
> to its original baseline value.

Which mainline kernel version you use in pixel6?

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.

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.

Regards,
Lukasz
  

On 11/15/22 22:35, Saravana Kannan wrote:
> On Thu, Nov 10, 2022 at 11:57 AM Sam Wu <wusamuel@google.com> wrote:
>>
>> This reverts commit 6d5afdc97ea71958287364a1f1d07e59ef151b11.
>>
>> On a Pixel 6 device, it is observed that this commit increases
>> latency by approximately 50ms, or 20%, in migrating a task
>> that requires full CPU utilization from a LITTLE CPU to Fmax
>> on a big CPU. Reverting this change restores the latency back
>> to its original baseline value.
>>
>> Fixes: 6d5afdc97ea7 ("cpufreq: schedutil: Move max CPU capacity to sugov_policy")
>> Cc: Lukasz Luba <lukasz.luba@arm.com>
>> Cc: Saravana Kannan <saravanak@google.com>
>> Cc: Isaac J. Manjarres <isaacmanjarres@google.com>
>> Signed-off-by: Sam Wu <wusamuel@google.com>
> 
> Rafael, can we pick this up please?
> 
> Lukasz, no objections to the idea itself, but it's causing regression
> and we'd like to revert and then fix it.

If you see this in mainline kernel, then I'm fine with reverting it.
Then I will have to trace why this CPU capacity value can change over
time in mainline kernel (while it shouldn't, because we register the
cpufreq policy and the governor later, after we calculate the capacity
in arch_topology.c). Maybe something has changed in mainline in the
meantime in this CPU capacity setup code, which caused this side effect.

I know that android-mainline has some different setup code for those
custom vendor governors. I just want to eliminate this bit and be on the
same page.

Regards,
Lukasz

> 
> -Saravana
>
  

On Wed, Nov 16, 2022 at 12:43 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
>
>
> On 11/15/22 22:35, Saravana Kannan wrote:
> > On Thu, Nov 10, 2022 at 11:57 AM Sam Wu <wusamuel@google.com> wrote:
> >>
> >> This reverts commit 6d5afdc97ea71958287364a1f1d07e59ef151b11.
> >>
> >> On a Pixel 6 device, it is observed that this commit increases
> >> latency by approximately 50ms, or 20%, in migrating a task
> >> that requires full CPU utilization from a LITTLE CPU to Fmax
> >> on a big CPU. Reverting this change restores the latency back
> >> to its original baseline value.
> >>
> >> Fixes: 6d5afdc97ea7 ("cpufreq: schedutil: Move max CPU capacity to sugov_policy")
> >> Cc: Lukasz Luba <lukasz.luba@arm.com>
> >> Cc: Saravana Kannan <saravanak@google.com>
> >> Cc: Isaac J. Manjarres <isaacmanjarres@google.com>
> >> Signed-off-by: Sam Wu <wusamuel@google.com>
> >
> > Rafael, can we pick this up please?
> >
> > Lukasz, no objections to the idea itself, but it's causing regression
> > and we'd like to revert and then fix it.
>
> If you see this in mainline kernel, then I'm fine with reverting it.

OK, I'll wait for the confirmation of this.

> Then I will have to trace why this CPU capacity value can change over
> time in mainline kernel (while it shouldn't, because we register the
> cpufreq policy and the governor later, after we calculate the capacity
> in arch_topology.c). Maybe something has changed in mainline in the
> meantime in this CPU capacity setup code, which caused this side effect.
>
> I know that android-mainline has some different setup code for those
> custom vendor governors. I just want to eliminate this bit and be on the
> same page.
  

On Wed, Nov 16, 2022 at 3:35 AM Lukasz Luba <lukasz.luba@arm.com> 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.

Best,
Sam
  

[Note: this mail is primarily send for documentation purposes and/or for
regzbot, my Linux kernel regression tracking bot. That's why I removed
most or all folks from the list of recipients, but left any that looked
like a mailing lists. These mails usually contain '#forregzbot' in the
subject, to make them easy to spot and filter out.]

[TLDR: I'm adding this regression report to the list of tracked
regressions; all text from me you find below is based on a few templates
paragraphs you might have encountered already already in similar form.]

Hi, this is your Linux kernel regression tracker.

On 10.11.22 20:57, Sam Wu wrote:
> This reverts commit 6d5afdc97ea71958287364a1f1d07e59ef151b11.
> 
> On a Pixel 6 device, it is observed that this commit increases
> latency by approximately 50ms, or 20%, in migrating a task
> that requires full CPU utilization from a LITTLE CPU to Fmax
> on a big CPU. Reverting this change restores the latency back
> to its original baseline value.
> 
> Fixes: 6d5afdc97ea7 ("cpufreq: schedutil: Move max CPU capacity to sugov_policy")
> Cc: Lukasz Luba <lukasz.luba@arm.com>
> Cc: Saravana Kannan <saravanak@google.com>
> Cc: Isaac J. Manjarres <isaacmanjarres@google.com>
> Signed-off-by: Sam Wu <wusamuel@google.com>

Thanks for the report. To be sure below issue doesn't fall through the
cracks unnoticed, I'm adding it to regzbot, my Linux kernel regression
tracking bot:

#regzbot ^introduced 6d5afdc97ea7
#regzbot title cpufreq: schedutil: improved latency on Pixel 6
#regzbot ignore-activity

This isn't a regression? This issue or a fix for it are already
discussed somewhere else? It was fixed already? You want to clarify when
the regression started to happen? Or point out I got the title or
something else totally wrong? Then just reply -- ideally with also
telling regzbot about it, as explained here:
https://linux-regtracking.leemhuis.info/tracked-regression/

Reminder for developers: When fixing the issue, add 'Link:' tags
pointing to the report (the mail this one replies to), as explained for
in the Linux kernel's documentation; above webpage explains why this is
important for tracked regressions.

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)

P.S.: As the Linux kernel's regression tracker I deal with a lot of
reports and sometimes miss something important when writing mails like
this. If that's the case here, don't hesitate to tell me in a public
reply, it's in everyone's interest to set the public record straight.
  

On Fri, Nov 18, 2022 at 2:00 AM Sam Wu <wusamuel@google.com> wrote:
>
> On Wed, Nov 16, 2022 at 3:35 AM Lukasz Luba <lukasz.luba@arm.com> 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!
  

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@google.com> wrote:
>>
>> On Wed, Nov 16, 2022 at 3:35 AM Lukasz Luba <lukasz.luba@arm.com> 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
  

On 20.11.22 19:07, Thorsten Leemhuis wrote:
> On 10.11.22 20:57, Sam Wu wrote:
>> This reverts commit 6d5afdc97ea71958287364a1f1d07e59ef151b11.
>>
>> On a Pixel 6 device, it is observed that this commit increases
>> latency by approximately 50ms, or 20%, in migrating a task
>> that requires full CPU utilization from a LITTLE CPU to Fmax
>> on a big CPU. Reverting this change restores the latency back
>> to its original baseline value.
>>
>> Fixes: 6d5afdc97ea7 ("cpufreq: schedutil: Move max CPU capacity to sugov_policy")
>> Cc: Lukasz Luba <lukasz.luba@arm.com>
>> Cc: Saravana Kannan <saravanak@google.com>
>> Cc: Isaac J. Manjarres <isaacmanjarres@google.com>
>> Signed-off-by: Sam Wu <wusamuel@google.com>
> 
> Thanks for the report. To be sure below issue doesn't fall through the
> cracks unnoticed, I'm adding it to regzbot, my Linux kernel regression
> tracking bot:
> 
> #regzbot ^introduced 6d5afdc97ea7
> #regzbot title cpufreq: schedutil: improved latency on Pixel 6
> #regzbot ignore-activity

#regzbot fixed-by: cdcc5ef26b3
  

Hi All

Just for the log and because it took me a while to figure out the root
cause of the problem: This patch also creates a regression for
snapdragon845 based systems and probably on any QC chipsets that use a
LUT to update the OPP table at boot. The behavior is the same as
described by Sam with a staled value in sugov_policy.max field.

Regards,
Vincent

On Tue, 22 Nov 2022 at 09:58, Lukasz Luba <lukasz.luba@arm.com> wrote:
>
> 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@google.com> wrote:
> >>
> >> On Wed, Nov 16, 2022 at 3:35 AM Lukasz Luba <lukasz.luba@arm.com> 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
  

Hi Vincent,

On 11/30/22 10:42, Vincent Guittot wrote:
> Hi All
> 
> Just for the log and because it took me a while to figure out the root
> cause of the problem: This patch also creates a regression for
> snapdragon845 based systems and probably on any QC chipsets that use a
> LUT to update the OPP table at boot. The behavior is the same as
> described by Sam with a staled value in sugov_policy.max field.

Thanks for sharing this info and apologies that you spent cycles
on it.

I have checked that whole setup code (capacity + cpufreq policy and
governor). It looks like to have a proper capacity of CPUs, we need
to wait till the last policy is created. It's due to the arch_topology.c
mechanism which is only triggered after all CPUs' got the policy.
Unfortunately, this leads to a chicken & egg situation for this
schedutil setup of max capacity.

I have experimented with this code, which triggers an update in
the schedutil, when all CPUs got the policy and sugov gov
(with trace_printk() to mach the output below)

-------------------------8<-----------------------------------------
diff --git a/kernel/sched/cpufreq_schedutil.c 
b/kernel/sched/cpufreq_schedutil.c
index 9161d1136d01..f1913a857218 100644
--- a/kernel/sched/cpufreq_schedutil.c
+++ b/kernel/sched/cpufreq_schedutil.c
@@ -59,6 +59,7 @@ struct sugov_cpu {
  };

  static DEFINE_PER_CPU(struct sugov_cpu, sugov_cpu);
+static cpumask_var_t cpus_to_visit;

  /************************ Governor internals ***********************/

@@ -783,6 +784,22 @@ static int sugov_start(struct cpufreq_policy *policy)

                 cpufreq_add_update_util_hook(cpu, &sg_cpu->update_util, 
uu);
         }
+
+       cpumask_andnot(cpus_to_visit, cpus_to_visit, policy->related_cpus);
+
+       if (cpumask_empty(cpus_to_visit)) {
+               trace_printk("schedutil the visit cpu mask is empty now\n");
+               for_each_possible_cpu(cpu) {
+                       struct sugov_cpu *sg_cpu = &per_cpu(sugov_cpu, cpu);
+                       struct sugov_policy *sg_policy = sg_cpu->sg_policy;
+
+                       sg_policy->max = arch_scale_cpu_capacity(cpu);
+
+                       trace_printk("SCHEDUTIL: NEW  CPU%u 
cpu_capacity=%lu\n",
+                               cpu, sg_policy->max);
+               }
+       }
+
         return 0;
  }

@@ -800,6 +817,8 @@ static void sugov_stop(struct cpufreq_policy *policy)
                 irq_work_sync(&sg_policy->irq_work);
                 kthread_cancel_work_sync(&sg_policy->work);
         }
+
+       cpumask_or(cpus_to_visit, cpus_to_visit, policy->related_cpus);
  }

  static void sugov_limits(struct cpufreq_policy *policy)
@@ -829,6 +848,11 @@ struct cpufreq_governor schedutil_gov = {
  #ifdef CONFIG_CPU_FREQ_DEFAULT_GOV_SCHEDUTIL
  struct cpufreq_governor *cpufreq_default_governor(void)
  {
+       if (!alloc_cpumask_var(&cpus_to_visit, GFP_KERNEL))
+               return NULL;
+
+       cpumask_copy(cpus_to_visit, cpu_possible_mask);
+
         return &schedutil_gov;
  }
  #endif

---------------------------------->8---------------------------------


That simple approach fixes the issue. I have also tested it with
governor change a few times and setting back the schedutil.

-------------------------------------------
    kworker/u12:1-48      [004] .....     2.208847: sugov_start: 
schedutil the visit cpu mask is empty now
    kworker/u12:1-48      [004] .....     2.208854: sugov_start: 
SCHEDUTIL: NEW  CPU0 cpu_capacity=381
    kworker/u12:1-48      [004] .....     2.208857: sugov_start: 
SCHEDUTIL: NEW  CPU1 cpu_capacity=381
    kworker/u12:1-48      [004] .....     2.208860: sugov_start: 
SCHEDUTIL: NEW  CPU2 cpu_capacity=381
    kworker/u12:1-48      [004] .....     2.208862: sugov_start: 
SCHEDUTIL: NEW  CPU3 cpu_capacity=381
    kworker/u12:1-48      [004] .....     2.208864: sugov_start: 
SCHEDUTIL: NEW  CPU4 cpu_capacity=1024
    kworker/u12:1-48      [004] .....     2.208866: sugov_start: 
SCHEDUTIL: NEW  CPU5 cpu_capacity=1024
             bash-615     [005] .....    35.317113: sugov_start: 
schedutil the visit cpu mask is empty now
             bash-615     [005] .....    35.317120: sugov_start: 
SCHEDUTIL: NEW  CPU0 cpu_capacity=381
             bash-615     [005] .....    35.317123: sugov_start: 
SCHEDUTIL: NEW  CPU1 cpu_capacity=381
             bash-615     [005] .....    35.317125: sugov_start: 
SCHEDUTIL: NEW  CPU2 cpu_capacity=381
             bash-615     [005] .....    35.317127: sugov_start: 
SCHEDUTIL: NEW  CPU3 cpu_capacity=381
             bash-615     [005] .....    35.317129: sugov_start: 
SCHEDUTIL: NEW  CPU4 cpu_capacity=1024
             bash-615     [005] .....    35.317131: sugov_start: 
SCHEDUTIL: NEW  CPU5 cpu_capacity=1024
             bash-623     [003] .....    57.633328: sugov_start: 
schedutil the visit cpu mask is empty now
             bash-623     [003] .....    57.633336: sugov_start: 
SCHEDUTIL: NEW  CPU0 cpu_capacity=381
             bash-623     [003] .....    57.633339: sugov_start: 
SCHEDUTIL: NEW  CPU1 cpu_capacity=381
             bash-623     [003] .....    57.633340: sugov_start: 
SCHEDUTIL: NEW  CPU2 cpu_capacity=381
             bash-623     [003] .....    57.633342: sugov_start: 
SCHEDUTIL: NEW  CPU3 cpu_capacity=381
             bash-623     [003] .....    57.633343: sugov_start: 
SCHEDUTIL: NEW  CPU4 cpu_capacity=1024
             bash-623     [003] .....    57.633344: sugov_start: 
SCHEDUTIL: NEW  CPU5 cpu_capacity=1024
----------------------------------------------------

It should work.

Regards,
Lukasz
  

On Wed, 30 Nov 2022 at 15:04, Lukasz Luba <lukasz.luba@arm.com> wrote:
>
> Hi Vincent,
>
> On 11/30/22 10:42, Vincent Guittot wrote:
> > Hi All
> >
> > Just for the log and because it took me a while to figure out the root
> > cause of the problem: This patch also creates a regression for
> > snapdragon845 based systems and probably on any QC chipsets that use a
> > LUT to update the OPP table at boot. The behavior is the same as
> > described by Sam with a staled value in sugov_policy.max field.
>
> Thanks for sharing this info and apologies that you spent cycles
> on it.
>
> I have checked that whole setup code (capacity + cpufreq policy and
> governor). It looks like to have a proper capacity of CPUs, we need
> to wait till the last policy is created. It's due to the arch_topology.c
> mechanism which is only triggered after all CPUs' got the policy.
> Unfortunately, this leads to a chicken & egg situation for this
> schedutil setup of max capacity.
>
> I have experimented with this code, which triggers an update in
> the schedutil, when all CPUs got the policy and sugov gov
> (with trace_printk() to mach the output below)

Your proposal below looks similar to what is done in arch_topology.c.
arch_topology.c triggers a rebuild of sched_domain and removes its
cpufreq notifier cb once it has visited all CPUs, could it also
trigger an update of CPU's policy with cpufreq_update_policy() ?

At this point you will be sure that the normalization has happened and
the max capacity will not change.

I don't know if it's a global problem or only for systems using arch_topology

>
> -------------------------8<-----------------------------------------
> diff --git a/kernel/sched/cpufreq_schedutil.c
> b/kernel/sched/cpufreq_schedutil.c
> index 9161d1136d01..f1913a857218 100644
> --- a/kernel/sched/cpufreq_schedutil.c
> +++ b/kernel/sched/cpufreq_schedutil.c
> @@ -59,6 +59,7 @@ struct sugov_cpu {
>   };
>
>   static DEFINE_PER_CPU(struct sugov_cpu, sugov_cpu);
> +static cpumask_var_t cpus_to_visit;
>
>   /************************ Governor internals ***********************/
>
> @@ -783,6 +784,22 @@ static int sugov_start(struct cpufreq_policy *policy)
>
>                  cpufreq_add_update_util_hook(cpu, &sg_cpu->update_util,
> uu);
>          }
> +
> +       cpumask_andnot(cpus_to_visit, cpus_to_visit, policy->related_cpus);
> +
> +       if (cpumask_empty(cpus_to_visit)) {
> +               trace_printk("schedutil the visit cpu mask is empty now\n");
> +               for_each_possible_cpu(cpu) {
> +                       struct sugov_cpu *sg_cpu = &per_cpu(sugov_cpu, cpu);
> +                       struct sugov_policy *sg_policy = sg_cpu->sg_policy;
> +
> +                       sg_policy->max = arch_scale_cpu_capacity(cpu);
> +
> +                       trace_printk("SCHEDUTIL: NEW  CPU%u
> cpu_capacity=%lu\n",
> +                               cpu, sg_policy->max);
> +               }
> +       }
> +
>          return 0;
>   }
>
> @@ -800,6 +817,8 @@ static void sugov_stop(struct cpufreq_policy *policy)
>                  irq_work_sync(&sg_policy->irq_work);
>                  kthread_cancel_work_sync(&sg_policy->work);
>          }
> +
> +       cpumask_or(cpus_to_visit, cpus_to_visit, policy->related_cpus);
>   }
>
>   static void sugov_limits(struct cpufreq_policy *policy)
> @@ -829,6 +848,11 @@ struct cpufreq_governor schedutil_gov = {
>   #ifdef CONFIG_CPU_FREQ_DEFAULT_GOV_SCHEDUTIL
>   struct cpufreq_governor *cpufreq_default_governor(void)
>   {
> +       if (!alloc_cpumask_var(&cpus_to_visit, GFP_KERNEL))
> +               return NULL;
> +
> +       cpumask_copy(cpus_to_visit, cpu_possible_mask);
> +
>          return &schedutil_gov;
>   }
>   #endif
>
> ---------------------------------->8---------------------------------
>
>
> That simple approach fixes the issue. I have also tested it with
> governor change a few times and setting back the schedutil.
>
> -------------------------------------------
>     kworker/u12:1-48      [004] .....     2.208847: sugov_start:
> schedutil the visit cpu mask is empty now
>     kworker/u12:1-48      [004] .....     2.208854: sugov_start:
> SCHEDUTIL: NEW  CPU0 cpu_capacity=381
>     kworker/u12:1-48      [004] .....     2.208857: sugov_start:
> SCHEDUTIL: NEW  CPU1 cpu_capacity=381
>     kworker/u12:1-48      [004] .....     2.208860: sugov_start:
> SCHEDUTIL: NEW  CPU2 cpu_capacity=381
>     kworker/u12:1-48      [004] .....     2.208862: sugov_start:
> SCHEDUTIL: NEW  CPU3 cpu_capacity=381
>     kworker/u12:1-48      [004] .....     2.208864: sugov_start:
> SCHEDUTIL: NEW  CPU4 cpu_capacity=1024
>     kworker/u12:1-48      [004] .....     2.208866: sugov_start:
> SCHEDUTIL: NEW  CPU5 cpu_capacity=1024
>              bash-615     [005] .....    35.317113: sugov_start:
> schedutil the visit cpu mask is empty now
>              bash-615     [005] .....    35.317120: sugov_start:
> SCHEDUTIL: NEW  CPU0 cpu_capacity=381
>              bash-615     [005] .....    35.317123: sugov_start:
> SCHEDUTIL: NEW  CPU1 cpu_capacity=381
>              bash-615     [005] .....    35.317125: sugov_start:
> SCHEDUTIL: NEW  CPU2 cpu_capacity=381
>              bash-615     [005] .....    35.317127: sugov_start:
> SCHEDUTIL: NEW  CPU3 cpu_capacity=381
>              bash-615     [005] .....    35.317129: sugov_start:
> SCHEDUTIL: NEW  CPU4 cpu_capacity=1024
>              bash-615     [005] .....    35.317131: sugov_start:
> SCHEDUTIL: NEW  CPU5 cpu_capacity=1024
>              bash-623     [003] .....    57.633328: sugov_start:
> schedutil the visit cpu mask is empty now
>              bash-623     [003] .....    57.633336: sugov_start:
> SCHEDUTIL: NEW  CPU0 cpu_capacity=381
>              bash-623     [003] .....    57.633339: sugov_start:
> SCHEDUTIL: NEW  CPU1 cpu_capacity=381
>              bash-623     [003] .....    57.633340: sugov_start:
> SCHEDUTIL: NEW  CPU2 cpu_capacity=381
>              bash-623     [003] .....    57.633342: sugov_start:
> SCHEDUTIL: NEW  CPU3 cpu_capacity=381
>              bash-623     [003] .....    57.633343: sugov_start:
> SCHEDUTIL: NEW  CPU4 cpu_capacity=1024
>              bash-623     [003] .....    57.633344: sugov_start:
> SCHEDUTIL: NEW  CPU5 cpu_capacity=1024
> ----------------------------------------------------
>
> It should work.
>
> Regards,
> Lukasz
  

On 11/30/22 14:29, Vincent Guittot wrote:
> On Wed, 30 Nov 2022 at 15:04, Lukasz Luba <lukasz.luba@arm.com> wrote:
>>
>> Hi Vincent,
>>
>> On 11/30/22 10:42, Vincent Guittot wrote:
>>> Hi All
>>>
>>> Just for the log and because it took me a while to figure out the root
>>> cause of the problem: This patch also creates a regression for
>>> snapdragon845 based systems and probably on any QC chipsets that use a
>>> LUT to update the OPP table at boot. The behavior is the same as
>>> described by Sam with a staled value in sugov_policy.max field.
>>
>> Thanks for sharing this info and apologies that you spent cycles
>> on it.
>>
>> I have checked that whole setup code (capacity + cpufreq policy and
>> governor). It looks like to have a proper capacity of CPUs, we need
>> to wait till the last policy is created. It's due to the arch_topology.c
>> mechanism which is only triggered after all CPUs' got the policy.
>> Unfortunately, this leads to a chicken & egg situation for this
>> schedutil setup of max capacity.
>>
>> I have experimented with this code, which triggers an update in
>> the schedutil, when all CPUs got the policy and sugov gov
>> (with trace_printk() to mach the output below)
> 
> Your proposal below looks similar to what is done in arch_topology.c.

Yes, even the name 'cpus_to_visit' looks similar ;)

> arch_topology.c triggers a rebuild of sched_domain and removes its
> cpufreq notifier cb once it has visited all CPUs, could it also
> trigger an update of CPU's policy with cpufreq_update_policy() ?
> 
> At this point you will be sure that the normalization has happened and
> the max capacity will not change.

True, they are done under that blocking notification chain, for the
last policy init. This is before the last time we call the
schedutil sugov_start with that last policy. That's why this code
is able to see that properly normalized max capacity under the:
trace_printk("schedutil the visit cpu mask is empty now\n");


> 
> I don't know if it's a global problem or only for systems using arch_topology
> 

It would only be for those with arch_topology, so only our asymmetric
systems AFAICS.
  

Lukasz,

On 10-11-22, 19:57, Sam Wu wrote:
> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> index 9161d1136d01..1207c78f85c1 100644
> --- a/kernel/sched/cpufreq_schedutil.c
> +++ b/kernel/sched/cpufreq_schedutil.c
> @@ -25,9 +25,6 @@ struct sugov_policy {
>  	unsigned int		next_freq;
>  	unsigned int		cached_raw_freq;
>  
> -	/* max CPU capacity, which is equal for all CPUs in freq. domain */
> -	unsigned long		max;
> -
>  	/* The next fields are only needed if fast switch cannot be used: */
>  	struct			irq_work irq_work;
>  	struct			kthread_work work;
> @@ -51,6 +48,7 @@ struct sugov_cpu {
>  
>  	unsigned long		util;
>  	unsigned long		bw_dl;
> +	unsigned long		max;

IIUC, this part, i.e. moving max to sugov_policy, wasn't the problem
here, right ? Can you send a patch for that at least first, since this
is fully reverted now.

Or it doesn't make sense?
  

Hi Viresh,

On 12/5/22 09:18, Viresh Kumar wrote:
> Lukasz,
> 
> On 10-11-22, 19:57, Sam Wu wrote:
>> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
>> index 9161d1136d01..1207c78f85c1 100644
>> --- a/kernel/sched/cpufreq_schedutil.c
>> +++ b/kernel/sched/cpufreq_schedutil.c
>> @@ -25,9 +25,6 @@ struct sugov_policy {
>>   	unsigned int		next_freq;
>>   	unsigned int		cached_raw_freq;
>>   
>> -	/* max CPU capacity, which is equal for all CPUs in freq. domain */
>> -	unsigned long		max;
>> -
>>   	/* The next fields are only needed if fast switch cannot be used: */
>>   	struct			irq_work irq_work;
>>   	struct			kthread_work work;
>> @@ -51,6 +48,7 @@ struct sugov_cpu {
>>   
>>   	unsigned long		util;
>>   	unsigned long		bw_dl;
>> +	unsigned long		max;
> 
> IIUC, this part, i.e. moving max to sugov_policy, wasn't the problem
> here, right ? Can you send a patch for that at least first, since this
> is fully reverted now.
> 
> Or it doesn't make sense?
> 

Yes, that still could make sense. We could still optimize a bit that
code in the sugov_next_freq_shared(). Look at that function. It loops
over all CPUs in the policy and calls sugov_get_util() which calls
this arch_scale_cpu_capacity() N times. Then it does those
multiplications below:

if (j_util * max > j_max * util)

which will be 2*N mul operations...
IMO this is pointless and heavy for LITTLE cores which are 4 or
sometimes 6 in the policy.

As you could see, my code just left that loop with a simple
max() operation.

I might just attack this code differently. Switch to that
sugov_policy::max, fetch the cpu capacity only once, before that loop.
I will rewrite a bit the sugov_get_util() and adjust the
2nd user of it: sugov_update_single_common()

Regards,
Lukasz