Scheduler changes for v6.7

Please pull the latest sched/core git tree from:

   git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git sched-core-2024-01-08

Just a note that I'm currently bisecting into this merge for a horrendous performance regression.

It makes my empty kernel build go from 22 seconds to 44 seconds, and makes a full kernel build enormously slower too.

I haven't finished the bisection, but it's now inside *just* this pull, so I can already tell that I'm going to revert something in here, because this has been making my merge window miserable.

It's one of these two:

f12560779f9d sched/cpufreq: Rework iowait boost
9c0b4bb7f630 sched/cpufreq: Rework schedutil governor performance estimation

one more boot to go, then I'll try to revert whichever causes my machine to perform horribly much worse.

Quoted 8 lines · Linus Torvalds

It's one of these two:

f12560779f9d sched/cpufreq: Rework iowait boost
9c0b4bb7f630 sched/cpufreq: Rework schedutil governor performance estimation

one more boot to go, then I'll try to revert whichever causes my machine to perform horribly much worse.

I guess it should come as no surprise that the result is

9c0b4bb7f6303c9c4e2e34984c46f5a86478f84d is the first bad commit

but to revert cleanly I will have to revert all of

b3edde44e5d4 ("cpufreq/schedutil: Use a fixed reference frequency") f12560779f9d ("sched/cpufreq: Rework iowait boost") 9c0b4bb7f630 ("sched/cpufreq: Rework schedutil governor
performance estimation")

This is on a 32-core (64-thread) AMD Ryzen Threadripper 3970X, fwiw.

I'll keep that revert in my private test-tree for now (so that I have a working machine again), but I'll move it to my main branch soon unless somebody has a quick fix for this problem.

Linus

Could you confirm that cpufreq governor is schedutil and the driver is amd-pstate on your system ?

Also I'm interested by the output of the amd_pstate to confirm that it uses the adjust_perf callback

I suppose that you don't use uclamp feature and amd doesn't use EAS so that let the change of the min parameter of adjust_perf which was probably always 0 unless you use deadline scheduler and which now takes into account irq pressure.

Could you try the patch below which restores the previous min value ?

---
 kernel/sched/cpufreq_schedutil.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

kernel/sched/cpufreq_schedutil.c +3 -3

index 95c3c097083e..3fe8ac6ce9cc 100644
@@ -194,10 +194,11 @@ unsigned long sugov_effective_cpu_perf(int cpu, unsigned long actual,
 static void sugov_get_util(struct sugov_cpu *sg_cpu, unsigned long boost)
 {
        unsigned long min, max, util = cpu_util_cfs_boost(sg_cpu->cpu);
+       struct rq *rq = cpu_rq(sg_cpu->cpu);
 
        util = effective_cpu_util(sg_cpu->cpu, util, &min, &max);
        util = max(util, boost);
-       sg_cpu->bw_min = min;
+       sg_cpu->bw_min = cpu_bw_dl(rq);
        sg_cpu->util = sugov_effective_cpu_perf(sg_cpu->cpu, util, min, max);
 }
 

@@ -442,7 +443,7 @@ static void sugov_update_single_perf(struct update_util_data *hook, u64 time,
            sugov_cpu_is_busy(sg_cpu) && sg_cpu->util < prev_util)
                sg_cpu->util = prev_util;
 
-       cpufreq_driver_adjust_perf(sg_cpu->cpu, sg_cpu->bw_min,
+       cpufreq_driver_adjust_perf(sg_cpu->cpu, map_util_perf(sg_cpu->bw_min),
                                   sg_cpu->util, max_cap);
 
        sg_cpu->sg_policy->last_freq_update_time = time;
--

2.34.1

Quoted 6 lines

I'll keep that revert in my private test-tree for now (so that I have a working machine again), but I'll move it to my main branch soon unless somebody has a quick fix for this problem.

Linus

Quoted 3 lines · Vincent Guittot

Could you confirm that cpufreq governor is schedutil and the driver is amd-pstate on your system ?

schedutil yes, amd-pstate no. I actually just use acpi_cpufreq

Quoted 8 lines · Linus Torvalds

Quoted 3 lines · Vincent Guittot

Could you confirm that cpufreq governor is schedutil and the driver is amd-pstate on your system ?

schedutil yes, amd-pstate no. I actually just use acpi_cpufreq

Bah. Hit 'send' mistakenly too soon, thus the abrupt end and unfinished quoting removal.

And don't ask me why it's acpi_pstate-driven. I have X86_AMD_PSTATE=y, but

/sys/devices/system/cpu/cpufreq/policy0/scaling_driver

clearly says 'acpi-cpufreq'. Maybe I'm looking in the wrong place. My dmesg says

amd_pstate: the _CPC object is not present in SBIOS or ACPI disabled

which is presumably the reason my machine uses acpi-pstate.

I will also test out your other questions, but I need to go back and do more pull requests first.

Linus

Quoted 12 lines · Linus Torvalds

Quoted 8 lines · Linus Torvalds

Quoted 3 lines · Vincent Guittot

Could you confirm that cpufreq governor is schedutil and the driver is amd-pstate on your system ?

schedutil yes, amd-pstate no. I actually just use acpi_cpufreq

Quoted 8 lines

Bah. Hit 'send' mistakenly too soon, thus the abrupt end and unfinished quoting removal.

And don't ask me why it's acpi_pstate-driven. I have X86_AMD_PSTATE=y, but

/sys/devices/system/cpu/cpufreq/policy0/scaling_driver

clearly says 'acpi-cpufreq'. Maybe I'm looking in the wrong place. My dmesg says

That seems to be the right place to look

Quoted 5 lines

amd_pstate: the _CPC object is not present in SBIOS or ACPI disabled

which is presumably the reason my machine uses acpi-pstate.

Quoted 2 lines

I will also test out your other questions, but I need to go back and do more pull requests first.

ok, thanks

I'm going to continue checking what else could trigger such regression having in mind that your system should not have beeb impacted by this changes

Quoted 30 lines · Vincent Guittot

Quoted 12 lines · Linus Torvalds

Quoted 8 lines · Linus Torvalds

Quoted 3 lines · Vincent Guittot

Could you confirm that cpufreq governor is schedutil and the driver is amd-pstate on your system ?

schedutil yes, amd-pstate no. I actually just use acpi_cpufreq

Quoted 8 lines

Bah. Hit 'send' mistakenly too soon, thus the abrupt end and unfinished quoting removal.

And don't ask me why it's acpi_pstate-driven. I have X86_AMD_PSTATE=y, but

/sys/devices/system/cpu/cpufreq/policy0/scaling_driver

clearly says 'acpi-cpufreq'. Maybe I'm looking in the wrong place. My dmesg says

That seems to be the right place to look

Quoted 5 lines

amd_pstate: the _CPC object is not present in SBIOS or ACPI disabled

which is presumably the reason my machine uses acpi-pstate.

Quoted 8 lines

Quoted 2 lines

I will also test out your other questions, but I need to go back and do more pull requests first.

ok, thanks

I'm going to continue checking what else could trigger such regression having in mind that your system should not have beeb impacted by this changes

I can't see the regression on my

20-core (40-thread) Intel Xeon CPU E5-2690 v2

with 'schedutil' and 'acpi-cpufreq'.

f12560779f9d - sched/cpufreq: Rework iowait boost <- (w/ patches)
9c0b4bb7f630 - sched/cpufreq: Rework schedutil governor performance estimation
50181c0cff31 - sched/pelt: Avoid underestimation of task utilization <- (base)
...

# cpufreq-info -c 0 -e
...
analyzing CPU 0:
  driver: acpi-cpufreq
  CPUs which run at the same hardware frequency: 0
  CPUs which need to have their frequency coordinated by software: 0
  maximum transition latency: 10.0 us.
  hardware limits: 1.20 GHz - 3.00 GHz
  available frequency steps: 3.00 GHz, 3.00 GHz, 2.90 GHz, 2.70 GHz, 2.60 GHz, 2.50 GHz, 2.40 GHz, 2.20 GHz,
                             2.10 GHz, 2.00 GHz, 1.80 GHz, 1.70 GHz, 1.60 GHz, 1.50 GHz, 1.30 GHz, 1.20 GHz
  available cpufreq governors: conservative, ondemand, userspace, powersave, performance, schedutil
  current policy: frequency should be within 1.20 GHz and 3.00 GHz.
                  The governor "schedutil" may decide which speed to use
                  within this range.
  current CPU frequency is 1.20 GHz (asserted by call to hardware).

cpufreq is still fast-switching, so no schedutil 'sugov' DL threads.

Quoted 30 lines · Vincent Guittot

Quoted 12 lines · Linus Torvalds

Quoted 8 lines · Linus Torvalds

Quoted 3 lines · Vincent Guittot

Could you confirm that cpufreq governor is schedutil and the driver is amd-pstate on your system ?

schedutil yes, amd-pstate no. I actually just use acpi_cpufreq

Quoted 8 lines

Bah. Hit 'send' mistakenly too soon, thus the abrupt end and unfinished quoting removal.

And don't ask me why it's acpi_pstate-driven. I have X86_AMD_PSTATE=y, but

/sys/devices/system/cpu/cpufreq/policy0/scaling_driver

clearly says 'acpi-cpufreq'. Maybe I'm looking in the wrong place. My dmesg says

That seems to be the right place to look

Quoted 5 lines

amd_pstate: the _CPC object is not present in SBIOS or ACPI disabled

which is presumably the reason my machine uses acpi-pstate.

Quoted 8 lines

Quoted 2 lines

I will also test out your other questions, but I need to go back and do more pull requests first.

ok, thanks

I'm going to continue checking what else could trigger such regression having in mind that your system should not have beeb impacted by this changes

I can't see the regression on my

20-core (40-thread) Intel Xeon CPU E5-2690 v2

with 'schedutil' and 'acpi-cpufreq'.

Quoted 40 lines · Dietmar Eggemann

Quoted 38 lines · Vincent Guittot

Quoted 12 lines · Linus Torvalds

Quoted 8 lines · Linus Torvalds

Quoted 3 lines · Vincent Guittot

Could you confirm that cpufreq governor is schedutil and the driver is amd-pstate on your system ?

schedutil yes, amd-pstate no. I actually just use acpi_cpufreq

Quoted 8 lines

Bah. Hit 'send' mistakenly too soon, thus the abrupt end and unfinished quoting removal.

And don't ask me why it's acpi_pstate-driven. I have X86_AMD_PSTATE=y, but

/sys/devices/system/cpu/cpufreq/policy0/scaling_driver

clearly says 'acpi-cpufreq'. Maybe I'm looking in the wrong place. My dmesg says

That seems to be the right place to look

Quoted 7 lines

amd_pstate: the _CPC object is not present in SBIOS or ACPI disabled

which is presumably the reason my machine uses acpi-pstate.

I will also test out your other questions, but I need to go back and do more pull requests first.

ok, thanks

I'm going to continue checking what else could trigger such regression having in mind that your system should not have beeb impacted by this changes

Quoted 5 lines

I can't see the regression on my

20-core (40-thread) Intel Xeon CPU E5-2690 v2

with 'schedutil' and 'acpi-cpufreq'.

I tried to reproduce on AMD 3900X 12-Core system. I don't see any difference in compiling defconfig with and without the two patches reverted. ~1m26s.

using schedutil and acpi-cpufreq driver too.

I disabled CONFIG_UCLAMP_TASK and that didn't make a difference.

I would have expected the iowait boost to be the more troublesome being the more subtle one tbh.

I will test Vincent's test-patch next.

All cores stay at 2.2GHz (ok, so there's noise, but we're talking "within a couple of MHz of 2.2GHz").

Quoted 3 lines · Linus Torvalds

All cores stay at 2.2GHz (ok, so there's noise, but we're talking "within a couple of MHz of 2.2GHz").

Note: that is true also when every CPU is fully loaded and I do a real full build.

So the "empty make" is just my quick test that happens to be single-threaded and should take just 20s. All my real builds slow down too, because all CPUs stay at the minimum frequency.

And I just verified that Ingo's revert that only reverts two commits (commit 60ee1706bd11 in the tip tree), makes things work correctly for me.

Not surprising, since the bisection clearly pointed at just commit 9c0b4bb7f6303c being the one that caused the issue, but I decided to just double-check anyway.

So with that revert, for the single-threaded case I see 4GHz+ numbers (they spread from a single CPU to multiple CPUs once you run the benchmark a few times).

And then when I run a full parallel build (rather than the single-threaded empty one), the frequencies drop to ~3.85GHz for the all-cpu case.

Linus

That is odd. I can't see how the patch can cause this yet, could you try with a different compiler if possible?

I usually use perfetto but it should be easy to see frequency updates from power/cpu_frequency trace event.

echo 1 | sudo tee /sys/kernel/tracing/tracing_on
echo 1 | sudo tee /sys/kernel/tracing/events/power/cpu_frequency/enable sudo cat /sys/kernel/tracing/trace

With a *working* kernel, I get events, setting the frequency to either 2.2GHz (idle) or 3.8GHz (work).

Quoted 3 lines · Linus Torvalds

With a *working* kernel, I get events, setting the frequency to either 2.2GHz (idle) or 3.8GHz (work).

Just to fix that - not 3.8Ghz, but in addition to 2.2 I see 2.8 or 3.7:

...
<idle>-0 [034] d.s.. 208.340412: cpu_frequency:
state=2200000 cpu_id=34

     cc1-101686  [034] d.h..   208.342402: cpu_frequency:
state=2800000 cpu_id=34
     cc1-101686  [034] d.h..   208.343401: cpu_frequency:
state=3700000 cpu_id=34
      sh-108794  [029] d.h..   216.401014: cpu_frequency:
state=2200000 cpu_id=29
      sh-108794  [029] d....   216.402670: cpu_frequency:

state=2800000 cpu_id=29
genksyms-108565 [029] d.h.. 216.404005: cpu_frequency:
state=3700000 cpu_id=29
...

etc.

Linus

Quoted 6 lines · Linus Torvalds

Quoted 3 lines · Linus Torvalds

With a *working* kernel, I get events, setting the frequency to either 2.2GHz (idle) or 3.8GHz (work).

Quoted 1 line

Just to fix that - not 3.8Ghz, but in addition to 2.2 I see 2.8 or 3.7:

IIUC, with the commit you stay at lowest frequency all time which is clearly weird. One change that could create such behavior is in sugov_effective_cpu_perf() where we take the min between the actual utilization and the max allowed. If max is set to min capacity for whatever the reason, then you stay stuck to lowest capacity/frequency

What is the output of
/sys/devices/system/cpu/cpu0/cpu_capacity

It should be 1024 or close

Could you try the below ? it skips this step and ensures to use the actual utilization to select the frequency

---
 kernel/sched/cpufreq_schedutil.c | 1 -
 1 file changed, 1 deletion(-)

kernel/sched/cpufreq_schedutil.c +0 -1

index 95c3c097083e..e420e2ee1a10 100644
@@ -181,7 +181,6 @@ unsigned long sugov_effective_cpu_perf(int cpu, unsigned long actual,
 	/* Add dvfs headroom to actual utilization */
 	actual = map_util_perf(actual);
 	/* Actually we don't need to target the max performance */
-	if (actual < max)
 		max = actual;
 
 	/*

Signature · 1 line

2.34.1

Quoted 3 lines · Qais Yousef

That is odd. I can't see how the patch can cause this yet, could you try with a different compiler if possible?

I use two different compilers - I do my allmodconfig builds with gcc, and the kernels I boot with clang, so my problems have been with a kernel built with

   clang version 17.0.6

but to check that it's not a compiler issue I just did another try with my current public tip of tree (ie *without* any reverts for this issue) and gcc:

    gcc version 13.2.1

and the behavior is exactly the same: all cores are stuck at 2.2GHz.

So no, it's not compiler-dependent.

Quoted 6 lines

I usually use perfetto but it should be easy to see frequency updates from power/cpu_frequency trace event.

echo 1 | sudo tee /sys/kernel/tracing/tracing_on
echo 1 | sudo tee /sys/kernel/tracing/events/power/cpu_frequency/enable sudo cat /sys/kernel/tracing/trace

Shows absolutely nothing. Or rather, it shows the header with

# entries-in-buffer/entries-written: 0/0 #P:64

and that's it.

With a *working* kernel, I get events, setting the frequency to either 2.2GHz (idle) or 3.8GHz (work).

IOW, the tracing output is 100% consistent with "that commit breaks everything".

Quoted 8 lines · Linus Torvalds

It's one of these two:

f12560779f9d sched/cpufreq: Rework iowait boost
9c0b4bb7f630 sched/cpufreq: Rework schedutil governor performance estimation

one more boot to go, then I'll try to revert whichever causes my machine to perform horribly much worse.

I guess it should come as no surprise that the result is

9c0b4bb7f6303c9c4e2e34984c46f5a86478f84d is the first bad commit

but to revert cleanly I will have to revert all of

b3edde44e5d4 ("cpufreq/schedutil: Use a fixed reference frequency") f12560779f9d ("sched/cpufreq: Rework iowait boost") 9c0b4bb7f630 ("sched/cpufreq: Rework schedutil governor
performance estimation")

This is on a 32-core (64-thread) AMD Ryzen Threadripper 3970X, fwiw.

I'll keep that revert in my private test-tree for now (so that I have a working machine again), but I'll move it to my main branch soon unless somebody has a quick fix for this problem.

Quoted 22 lines · Linus Torvalds

Quoted 8 lines · Linus Torvalds

It's one of these two:

f12560779f9d sched/cpufreq: Rework iowait boost
9c0b4bb7f630 sched/cpufreq: Rework schedutil governor performance estimation

one more boot to go, then I'll try to revert whichever causes my machine to perform horribly much worse.

I guess it should come as no surprise that the result is

9c0b4bb7f6303c9c4e2e34984c46f5a86478f84d is the first bad commit

but to revert cleanly I will have to revert all of

b3edde44e5d4 ("cpufreq/schedutil: Use a fixed reference frequency") f12560779f9d ("sched/cpufreq: Rework iowait boost") 9c0b4bb7f630 ("sched/cpufreq: Rework schedutil governor
performance estimation")

Quoted 5 lines

This is on a 32-core (64-thread) AMD Ryzen Threadripper 3970X, fwiw.

I'll keep that revert in my private test-tree for now (so that I have a working machine again), but I'll move it to my main branch soon unless somebody has a quick fix for this problem.

Thanks a lot for bisecting this, and ack on the revert in any case, these are relatively fresh changes that clearly didn't get enough testing - sorry!

I also made the revert in sched/urgent & added a changelog, which you can pull from:

git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git sched-urgent-2024-01-11

# HEAD: 250ce3c1169743f3575cc5937fccd72380052795 Revert "sched/cpufreq: Rework schedutil governor performance estimation" and dependent commits

Revert recent changes to the sched_util logic, to address a bad performance regression that increased kernel build time on Linus's 64-CPU desktop system substantially.

Lightly build and boot tested.

Thanks,

Ingo

------------------>
Ingo Molnar (1):
Revert "sched/cpufreq: Rework schedutil governor performance estimation" and dependent commits

 include/linux/energy_model.h     |  1 +
 kernel/sched/core.c              | 90 +++++++++++++++++++++++-----------------
 kernel/sched/cpufreq_schedutil.c | 90 ++++++++++++----------------------------
 kernel/sched/fair.c              | 22 ++--------
 kernel/sched/sched.h             | 84 +++++++++++++++++++++++++++++++++----
 5 files changed, 160 insertions(+), 127 deletions(-)

Quoted 32 lines · Ingo Molnar

Quoted 27 lines · Linus Torvalds

Quoted 8 lines · Linus Torvalds

It's one of these two:

f12560779f9d sched/cpufreq: Rework iowait boost
9c0b4bb7f630 sched/cpufreq: Rework schedutil governor performance estimation

one more boot to go, then I'll try to revert whichever causes my machine to perform horribly much worse.

I guess it should come as no surprise that the result is

9c0b4bb7f6303c9c4e2e34984c46f5a86478f84d is the first bad commit

but to revert cleanly I will have to revert all of

b3edde44e5d4 ("cpufreq/schedutil: Use a fixed reference frequency") f12560779f9d ("sched/cpufreq: Rework iowait boost") 9c0b4bb7f630 ("sched/cpufreq: Rework schedutil governor
performance estimation")

This is on a 32-core (64-thread) AMD Ryzen Threadripper 3970X, fwiw.

I'll keep that revert in my private test-tree for now (so that I have a working machine again), but I'll move it to my main branch soon unless somebody has a quick fix for this problem.

Quoted 2 lines

Thanks a lot for bisecting this, and ack on the revert in any case, these are relatively fresh changes that clearly didn't get enough testing - sorry!

b3edde44e5d4 ("cpufreq/schedutil: Use a fixed reference frequency") is linked with other patches.

I can provide a clean revert of only :
f12560779f9d ("sched/cpufreq: Rework iowait boost")
9c0b4bb7f630 ("sched/cpufreq: Rework schedutil governor performance estimation")

if the fix that i proposed doesn't work:
https://lore.kernel.org/all/ZZ+ixagkxRPYyTCE@vingu-book/

Quoted 33 lines · Vincent Guittot

Quoted 32 lines · Ingo Molnar

Quoted 27 lines · Linus Torvalds

Quoted 8 lines · Linus Torvalds

It's one of these two:

f12560779f9d sched/cpufreq: Rework iowait boost
9c0b4bb7f630 sched/cpufreq: Rework schedutil governor performance estimation

one more boot to go, then I'll try to revert whichever causes my machine to perform horribly much worse.

I guess it should come as no surprise that the result is

9c0b4bb7f6303c9c4e2e34984c46f5a86478f84d is the first bad commit

but to revert cleanly I will have to revert all of

b3edde44e5d4 ("cpufreq/schedutil: Use a fixed reference frequency") f12560779f9d ("sched/cpufreq: Rework iowait boost") 9c0b4bb7f630 ("sched/cpufreq: Rework schedutil governor
performance estimation")

This is on a 32-core (64-thread) AMD Ryzen Threadripper 3970X, fwiw.

I'll keep that revert in my private test-tree for now (so that I have a working machine again), but I'll move it to my main branch soon unless somebody has a quick fix for this problem.

Quoted 5 lines

Quoted 2 lines

Thanks a lot for bisecting this, and ack on the revert in any case, these are relatively fresh changes that clearly didn't get enough testing - sorry!

b3edde44e5d4 ("cpufreq/schedutil: Use a fixed reference frequency") is linked with other patches.

Indeed.

Quoted 3 lines

I can provide a clean revert of only :
f12560779f9d ("sched/cpufreq: Rework iowait boost")
9c0b4bb7f630 ("sched/cpufreq: Rework schedutil governor performance estimation")

I've done this too, see this new commit in sched/urgent:

60ee1706bd11 ("Revert "sched/cpufreq: Rework schedutil governor performance estimation" and dependent commit")

Also attached below.

Quoted 2 lines

if the fix that i proposed doesn't work:
https://lore.kernel.org/all/ZZ+ixagkxRPYyTCE@vingu-book/

Yeah - although of course Linus is free to just pull the revert as well.

I'll try to reproduce the regression locally as well.

Thanks,

Ingo

===============================>
From: Ingo Molnar <[email protected]>
Date: Thu, 11 Jan 2024 11:45:17 +0100
Subject: [PATCH] Revert "sched/cpufreq: Rework schedutil governor performance estimation" and dependent commit

This reverts the following commits:

f12560779f9d73 ("sched/cpufreq: Rework iowait boost")
9c0b4bb7f6303c ("sched/cpufreq: Rework schedutil governor performance estimation")

Because Linus reported a bad performance regression with the sched_util governor, that increased the time his empty
kernel build took from 22 to 44 seconds (and can be similarly measured in full builds as well) - and bisected it back to 9c0b4bb7f6303c.

Until we have a proper fix, revert the broken commit and its dependent commit.

Reported-by Linus Torvalds <[email protected]>

Bisected-by Linus Torvalds <[email protected]>

Signed-off-by Ingo Molnar <[email protected]>

Cc Vincent Guittot <[email protected]>

Link https://lore.kernel.org/r/CAHk-=wgWcYX2oXKtgvNN2LLDXP7kXkbo-xTfumEjmPbjSer2RQ@mail.gmail.com open here

---
 include/linux/energy_model.h     |  1 +
 kernel/sched/core.c              | 90 +++++++++++++++++++++++-----------------
 kernel/sched/cpufreq_schedutil.c | 64 +++++++++++-----------------
 kernel/sched/fair.c              | 22 ++--------
 kernel/sched/sched.h             | 84 +++++++++++++++++++++++++++++++++----
 5 files changed, 158 insertions(+), 103 deletions(-)

include/linux/energy_model.h +1 -0

index 88d91e087471..c19e7effe764 100644
@@ -243,6 +243,7 @@ static inline unsigned long em_cpu_energy(struct em_perf_domain *pd,
        scale_cpu = arch_scale_cpu_capacity(cpu);
        ref_freq = arch_scale_freq_ref(cpu);
 
+       max_util = map_util_perf(max_util);
        max_util = min(max_util, allowed_cpu_cap);
        freq = map_util_freq(max_util, ref_freq, scale_cpu);
 

kernel/sched/core.c +52 -38

index 9116bcc90346..038eeaf76d2d 100644
@@ -7468,13 +7468,18 @@ int sched_core_idle_cpu(int cpu)
  * required to meet deadlines.
  */
 unsigned long effective_cpu_util(int cpu, unsigned long util_cfs,
-                                unsigned long *min,
-                                unsigned long *max)
+                                enum cpu_util_type type,
+                                struct task_struct *p)
 {
-       unsigned long util, irq, scale;
+       unsigned long dl_util, util, irq, max;
        struct rq *rq = cpu_rq(cpu);
 
-       scale = arch_scale_cpu_capacity(cpu);
+       max = arch_scale_cpu_capacity(cpu);
+
+       if (!uclamp_is_used() &&
+           type == FREQUENCY_UTIL && rt_rq_is_runnable(&rq->rt)) {
+               return max;
+       }
 
        /*
         * Early check to see if IRQ/steal time saturates the CPU, can be

@@ -7482,49 +7487,45 @@ unsigned long effective_cpu_util(int cpu, unsigned long util_cfs,
         * update_irq_load_avg().
         */
        irq = cpu_util_irq(rq);
-       if (unlikely(irq >= scale)) {
-               if (min)
-                       *min = scale;
-               if (max)
-                       *max = scale;
-               return scale;
-       }
-
-       if (min) {
-               /*
-                * The minimum utilization returns the highest level between:
-                * - the computed DL bandwidth needed with the IRQ pressure which
-                *   steals time to the deadline task.
-                * - The minimum performance requirement for CFS and/or RT.
-                */
-               *min = max(irq + cpu_bw_dl(rq), uclamp_rq_get(rq, UCLAMP_MIN));
-
-               /*
-                * When an RT task is runnable and uclamp is not used, we must
-                * ensure that the task will run at maximum compute capacity.
-                */
-               if (!uclamp_is_used() && rt_rq_is_runnable(&rq->rt))
-                       *min = max(*min, scale);
-       }
+       if (unlikely(irq >= max))
+               return max;
 
        /*
         * Because the time spend on RT/DL tasks is visible as 'lost' time to
         * CFS tasks and we use the same metric to track the effective
         * utilization (PELT windows are synchronized) we can directly add them
         * to obtain the CPU's actual utilization.
+        *
+        * CFS and RT utilization can be boosted or capped, depending on
+        * utilization clamp constraints requested by currently RUNNABLE
+        * tasks.
+        * When there are no CFS RUNNABLE tasks, clamps are released and
+        * frequency will be gracefully reduced with the utilization decay.
         */
        util = util_cfs + cpu_util_rt(rq);
-       util += cpu_util_dl(rq);
+       if (type == FREQUENCY_UTIL)
+               util = uclamp_rq_util_with(rq, util, p);
+
+       dl_util = cpu_util_dl(rq);
 
        /*
-        * The maximum hint is a soft bandwidth requirement, which can be lower
-        * than the actual utilization because of uclamp_max requirements.
+        * For frequency selection we do not make cpu_util_dl() a permanent part
+        * of this sum because we want to use cpu_bw_dl() later on, but we need
+        * to check if the CFS+RT+DL sum is saturated (ie. no idle time) such
+        * that we select f_max when there is no idle time.
+        *
+        * NOTE: numerical errors or stop class might cause us to not quite hit
+        * saturation when we should -- something for later.
         */
-       if (max)
-               *max = min(scale, uclamp_rq_get(rq, UCLAMP_MAX));
+       if (util + dl_util >= max)
+               return max;
 
-       if (util >= scale)
-               return scale;
+       /*
+        * OTOH, for energy computation we need the estimated running time, so
+        * include util_dl and ignore dl_bw.
+        */
+       if (type == ENERGY_UTIL)
+               util += dl_util;
 
        /*
         * There is still idle time; further improve the number by using the

@@ -7535,15 +7536,28 @@ unsigned long effective_cpu_util(int cpu, unsigned long util_cfs,
         *   U' = irq + --------- * U
         *                 max
         */
-       util = scale_irq_capacity(util, irq, scale);
+       util = scale_irq_capacity(util, irq, max);
        util += irq;
 
-       return min(scale, util);
+       /*
+        * Bandwidth required by DEADLINE must always be granted while, for
+        * FAIR and RT, we use blocked utilization of IDLE CPUs as a mechanism
+        * to gracefully reduce the frequency when no tasks show up for longer
+        * periods of time.
+        *
+        * Ideally we would like to set bw_dl as min/guaranteed freq and util +
+        * bw_dl as requested freq. However, cpufreq is not yet ready for such
+        * an interface. So, we only do the latter for now.
+        */
+       if (type == FREQUENCY_UTIL)
+               util += cpu_bw_dl(rq);
+
+       return min(max, util);
 }
 
 unsigned long sched_cpu_util(int cpu)
 {
-       return effective_cpu_util(cpu, cpu_util_cfs(cpu), NULL, NULL);
+       return effective_cpu_util(cpu, cpu_util_cfs(cpu), ENERGY_UTIL, NULL);
 }
 #endif /* CONFIG_SMP */
 

kernel/sched/cpufreq_schedutil.c +25 -39

index 95c3c097083e..5f8729c41d0a 100644
@@ -47,7 +47,7 @@ struct sugov_cpu {
        u64                     last_update;
 
        unsigned long           util;
-       unsigned long           bw_min;
+       unsigned long           bw_dl;
 
        /* The field below is for single-CPU policies only: */
 #ifdef CONFIG_NO_HZ_COMMON

@@ -164,6 +164,7 @@ static unsigned int get_next_freq(struct sugov_policy *sg_policy,
        struct cpufreq_policy *policy = sg_policy->policy;
        unsigned int freq;
 
+       util = map_util_perf(util);
        freq = get_capacity_ref_freq(policy);
        freq = map_util_freq(util, freq, max);
 

@@ -174,31 +175,14 @@ static unsigned int get_next_freq(struct sugov_policy *sg_policy,
        return cpufreq_driver_resolve_freq(policy, freq);
 }
 
-unsigned long sugov_effective_cpu_perf(int cpu, unsigned long actual,
-                                unsigned long min,
-                                unsigned long max)
+static void sugov_get_util(struct sugov_cpu *sg_cpu)
 {
-       /* Add dvfs headroom to actual utilization */
-       actual = map_util_perf(actual);
-       /* Actually we don't need to target the max performance */
-       if (actual < max)
-               max = actual;
+       unsigned long util = cpu_util_cfs_boost(sg_cpu->cpu);
+       struct rq *rq = cpu_rq(sg_cpu->cpu);
 
-       /*
-        * Ensure at least minimum performance while providing more compute
-        * capacity when possible.
-        */
-       return max(min, max);
-}
-
-static void sugov_get_util(struct sugov_cpu *sg_cpu, unsigned long boost)
-{
-       unsigned long min, max, util = cpu_util_cfs_boost(sg_cpu->cpu);
-
-       util = effective_cpu_util(sg_cpu->cpu, util, &min, &max);
-       util = max(util, boost);
-       sg_cpu->bw_min = min;
-       sg_cpu->util = sugov_effective_cpu_perf(sg_cpu->cpu, util, min, max);
+       sg_cpu->bw_dl = cpu_bw_dl(rq);
+       sg_cpu->util = effective_cpu_util(sg_cpu->cpu, util,
+                                         FREQUENCY_UTIL, NULL);
 }
 
 /**

@@ -289,16 +273,18 @@ static void sugov_iowait_boost(struct sugov_cpu *sg_cpu, u64 time,
  * This mechanism is designed to boost high frequently IO waiting tasks, while
  * being more conservative on tasks which does sporadic IO operations.
  */
-static unsigned long sugov_iowait_apply(struct sugov_cpu *sg_cpu, u64 time,
+static void sugov_iowait_apply(struct sugov_cpu *sg_cpu, u64 time,
                               unsigned long max_cap)
 {
+       unsigned long boost;
+
        /* No boost currently required */
        if (!sg_cpu->iowait_boost)
-               return 0;
+               return;
 
        /* Reset boost if the CPU appears to have been idle enough */
        if (sugov_iowait_reset(sg_cpu, time, false))
-               return 0;
+               return;
 
        if (!sg_cpu->iowait_boost_pending) {
                /*

@@ -307,7 +293,7 @@ static unsigned long sugov_iowait_apply(struct sugov_cpu *sg_cpu, u64 time,
                sg_cpu->iowait_boost >>= 1;
                if (sg_cpu->iowait_boost < IOWAIT_BOOST_MIN) {
                        sg_cpu->iowait_boost = 0;
-                       return 0;
+                       return;
                }
        }
 

@@ -317,7 +303,10 @@ static unsigned long 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.
         */
-       return (sg_cpu->iowait_boost * max_cap) >> SCHED_CAPACITY_SHIFT;
+       boost = (sg_cpu->iowait_boost * max_cap) >> SCHED_CAPACITY_SHIFT;
+       boost = uclamp_rq_util_with(cpu_rq(sg_cpu->cpu), boost, NULL);
+       if (sg_cpu->util < boost)
+               sg_cpu->util = boost;
 }
 
 #ifdef CONFIG_NO_HZ_COMMON

@@ -339,7 +328,7 @@ static inline bool sugov_cpu_is_busy(struct sugov_cpu *sg_cpu) { return false; }
  */
 static inline void ignore_dl_rate_limit(struct sugov_cpu *sg_cpu)
 {
-       if (cpu_bw_dl(cpu_rq(sg_cpu->cpu)) > sg_cpu->bw_min)
+       if (cpu_bw_dl(cpu_rq(sg_cpu->cpu)) > sg_cpu->bw_dl)
                sg_cpu->sg_policy->limits_changed = true;
 }
 

@@ -347,8 +336,6 @@ static inline bool sugov_update_single_common(struct sugov_cpu *sg_cpu,
                                              u64 time, unsigned long max_cap,
                                              unsigned int flags)
 {
-       unsigned long boost;
-
        sugov_iowait_boost(sg_cpu, time, flags);
        sg_cpu->last_update = time;
 

@@ -357,8 +344,8 @@ static inline bool sugov_update_single_common(struct sugov_cpu *sg_cpu,
        if (!sugov_should_update_freq(sg_cpu->sg_policy, time))
                return false;
 
-       boost = sugov_iowait_apply(sg_cpu, time, max_cap);
-       sugov_get_util(sg_cpu, boost);
+       sugov_get_util(sg_cpu);
+       sugov_iowait_apply(sg_cpu, time, max_cap);
 
        return true;
 }

@@ -442,8 +429,8 @@ static void sugov_update_single_perf(struct update_util_data *hook, u64 time,
            sugov_cpu_is_busy(sg_cpu) && sg_cpu->util < prev_util)
                sg_cpu->util = prev_util;
 
-       cpufreq_driver_adjust_perf(sg_cpu->cpu, sg_cpu->bw_min,
-                                  sg_cpu->util, max_cap);
+       cpufreq_driver_adjust_perf(sg_cpu->cpu, map_util_perf(sg_cpu->bw_dl),
+                                  map_util_perf(sg_cpu->util), max_cap);
 
        sg_cpu->sg_policy->last_freq_update_time = time;
 }

@@ -459,10 +446,9 @@ static unsigned int sugov_next_freq_shared(struct sugov_cpu *sg_cpu, u64 time)
 
        for_each_cpu(j, policy->cpus) {
                struct sugov_cpu *j_sg_cpu = &per_cpu(sugov_cpu, j);
-               unsigned long boost;
 
-               boost = sugov_iowait_apply(j_sg_cpu, time, max_cap);
-               sugov_get_util(j_sg_cpu, boost);
+               sugov_get_util(j_sg_cpu);
+               sugov_iowait_apply(j_sg_cpu, time, max_cap);
 
                util = max(j_sg_cpu->util, util);
        }

kernel/sched/fair.c +3 -19

index 533547e3c90a..f2bb83675e4a 100644
@@ -7820,7 +7820,7 @@ static inline void eenv_pd_busy_time(struct energy_env *eenv,
        for_each_cpu(cpu, pd_cpus) {
                unsigned long util = cpu_util(cpu, p, -1, 0);
 
-               busy_time += effective_cpu_util(cpu, util, NULL, NULL);
+               busy_time += effective_cpu_util(cpu, util, ENERGY_UTIL, NULL);
        }
 
        eenv->pd_busy_time = min(eenv->pd_cap, busy_time);

@@ -7843,7 +7843,7 @@ eenv_pd_max_util(struct energy_env *eenv, struct cpumask *pd_cpus,
        for_each_cpu(cpu, pd_cpus) {
                struct task_struct *tsk = (cpu == dst_cpu) ? p : NULL;
                unsigned long util = cpu_util(cpu, p, dst_cpu, 1);
-               unsigned long eff_util, min, max;
+               unsigned long eff_util;
 
                /*
                 * Performance domain frequency: utilization clamping

@@ -7852,23 +7852,7 @@ eenv_pd_max_util(struct energy_env *eenv, struct cpumask *pd_cpus,
                 * NOTE: in case RT tasks are running, by default the
                 * FREQUENCY_UTIL's utilization can be max OPP.
                 */
-               eff_util = effective_cpu_util(cpu, util, &min, &max);
-
-               /* Task's uclamp can modify min and max value */
-               if (tsk && uclamp_is_used()) {
-                       min = max(min, uclamp_eff_value(p, UCLAMP_MIN));
-
-                       /*
-                        * If there is no active max uclamp constraint,
-                        * directly use task's one, otherwise keep max.
-                        */
-                       if (uclamp_rq_is_idle(cpu_rq(cpu)))
-                               max = uclamp_eff_value(p, UCLAMP_MAX);
-                       else
-                               max = max(max, uclamp_eff_value(p, UCLAMP_MAX));
-               }
-
-               eff_util = sugov_effective_cpu_perf(cpu, eff_util, min, max);
+               eff_util = effective_cpu_util(cpu, util, FREQUENCY_UTIL, tsk);
                max_util = max(max_util, eff_util);
        }
 

kernel/sched/sched.h +77 -7

index 001fe047bd5d..eb7e07a1abcc 100644
@@ -3005,14 +3005,24 @@ static inline void cpufreq_update_util(struct rq *rq, unsigned int flags) {}
 #endif
 
 #ifdef CONFIG_SMP
+/**
+ * enum cpu_util_type - CPU utilization type
+ * @FREQUENCY_UTIL:    Utilization used to select frequency
+ * @ENERGY_UTIL:       Utilization used during energy calculation
+ *
+ * The utilization signals of all scheduling classes (CFS/RT/DL) and IRQ time
+ * need to be aggregated differently depending on the usage made of them. This
+ * enum is used within effective_cpu_util() to differentiate the types of
+ * utilization expected by the callers, and adjust the aggregation accordingly.
+ */
+enum cpu_util_type {
+       FREQUENCY_UTIL,
+       ENERGY_UTIL,
+};
+
 unsigned long effective_cpu_util(int cpu, unsigned long util_cfs,
-                                unsigned long *min,
-                                unsigned long *max);
-
-unsigned long sugov_effective_cpu_perf(int cpu, unsigned long actual,
-                                unsigned long min,
-                                unsigned long max);
-
+                                enum cpu_util_type type,
+                                struct task_struct *p);
 
 /*
  * Verify the fitness of task @p to run on @cpu taking into account the

@@ -3069,6 +3079,59 @@ static inline bool uclamp_rq_is_idle(struct rq *rq)
        return rq->uclamp_flags & UCLAMP_FLAG_IDLE;
 }
 
+/**
+ * uclamp_rq_util_with - clamp @util with @rq and @p effective uclamp values.
+ * @rq:                The rq to clamp against. Must not be NULL.
+ * @util:      The util value to clamp.
+ * @p:         The task to clamp against. Can be NULL if you want to clamp
+ *             against @rq only.
+ *
+ * Clamps the passed @util to the max(@rq, @p) effective uclamp values.
+ *
+ * If sched_uclamp_used static key is disabled, then just return the util
+ * without any clamping since uclamp aggregation at the rq level in the fast
+ * path is disabled, rendering this operation a NOP.
+ *
+ * Use uclamp_eff_value() if you don't care about uclamp values at rq level. It
+ * will return the correct effective uclamp value of the task even if the
+ * static key is disabled.
+ */
+static __always_inline
+unsigned long uclamp_rq_util_with(struct rq *rq, unsigned long util,
+                                 struct task_struct *p)
+{
+       unsigned long min_util = 0;
+       unsigned long max_util = 0;
+
+       if (!static_branch_likely(&sched_uclamp_used))
+               return util;
+
+       if (p) {
+               min_util = uclamp_eff_value(p, UCLAMP_MIN);
+               max_util = uclamp_eff_value(p, UCLAMP_MAX);
+
+               /*
+                * Ignore last runnable task's max clamp, as this task will
+                * reset it. Similarly, no need to read the rq's min clamp.
+                */
+               if (uclamp_rq_is_idle(rq))
+                       goto out;
+       }
+
+       min_util = max_t(unsigned long, min_util, uclamp_rq_get(rq, UCLAMP_MIN));
+       max_util = max_t(unsigned long, max_util, uclamp_rq_get(rq, UCLAMP_MAX));
+out:
+       /*
+        * Since CPU's {min,max}_util clamps are MAX aggregated considering
+        * RUNNABLE tasks with _different_ clamps, we can end up with an
+        * inversion. Fix it now when the clamps are applied.
+        */
+       if (unlikely(min_util >= max_util))
+               return min_util;
+
+       return clamp(util, min_util, max_util);
+}
+
 /* Is the rq being capped/throttled by uclamp_max? */
 static inline bool uclamp_rq_is_capped(struct rq *rq)
 {

@@ -3106,6 +3169,13 @@ static inline unsigned long uclamp_eff_value(struct task_struct *p,
        return SCHED_CAPACITY_SCALE;
 }
 
+static inline
+unsigned long uclamp_rq_util_with(struct rq *rq, unsigned long util,
+                                 struct task_struct *p)
+{
+       return util;
+}
+
 static inline bool uclamp_rq_is_capped(struct rq *rq) { return false; }
 
 static inline bool uclamp_is_used(void)

Quoted 3 lines

I can provide a clean revert of only :
f12560779f9d ("sched/cpufreq: Rework iowait boost")
9c0b4bb7f630 ("sched/cpufreq: Rework schedutil governor performance estimation")

I've done this too, see this new commit in sched/urgent:

60ee1706bd11 ("Revert "sched/cpufreq: Rework schedutil governor performance estimation" and dependent commit")

Also attached below.

Quoted 2 lines

if the fix that i proposed doesn't work:
https://lore.kernel.org/all/ZZ+ixagkxRPYyTCE@vingu-book/

Yeah - although of course Linus is free to just pull the revert as well. I'll try to reproduce the regression locally as well.

This is on a 32-core (64-thread) AMD Ryzen Threadripper 3970X, fwiw.

Quoted 8 lines · Linus Torvalds

It's one of these two:

f12560779f9d sched/cpufreq: Rework iowait boost
9c0b4bb7f630 sched/cpufreq: Rework schedutil governor performance estimation

one more boot to go, then I'll try to revert whichever causes my machine to perform horribly much worse.

I guess it should come as no surprise that the result is

9c0b4bb7f6303c9c4e2e34984c46f5a86478f84d is the first bad commit

but to revert cleanly I will have to revert all of

b3edde44e5d4 ("cpufreq/schedutil: Use a fixed reference frequency") f12560779f9d ("sched/cpufreq: Rework iowait boost") 9c0b4bb7f630 ("sched/cpufreq: Rework schedutil governor
performance estimation")

This is on a 32-core (64-thread) AMD Ryzen Threadripper 3970X, fwiw.

I'll keep that revert in my private test-tree for now (so that I have a working machine again), but I'll move it to my main branch soon unless somebody has a quick fix for this problem.

Quoted 22 lines · Linus Torvalds

Quoted 8 lines · Linus Torvalds

It's one of these two:

f12560779f9d sched/cpufreq: Rework iowait boost
9c0b4bb7f630 sched/cpufreq: Rework schedutil governor performance estimation

one more boot to go, then I'll try to revert whichever causes my machine to perform horribly much worse.

I guess it should come as no surprise that the result is

9c0b4bb7f6303c9c4e2e34984c46f5a86478f84d is the first bad commit

but to revert cleanly I will have to revert all of

b3edde44e5d4 ("cpufreq/schedutil: Use a fixed reference frequency") f12560779f9d ("sched/cpufreq: Rework iowait boost") 9c0b4bb7f630 ("sched/cpufreq: Rework schedutil governor
performance estimation")

Quoted 5 lines

This is on a 32-core (64-thread) AMD Ryzen Threadripper 3970X, fwiw.

I'll keep that revert in my private test-tree for now (so that I have a working machine again), but I'll move it to my main branch soon unless somebody has a quick fix for this problem.

Hi Linus,

I'm able to reproduce this issue with my AMD Ryzen 5600G system. But only if I disable CPPC in BIOS and boot with acpi-cpufreq + schedutil. (I believe for your case also CPPC is diabled as log "_CPC object is not present" came). Enabling CPPC in BIOS issue not seen in my system. For AMD acpi-cpufreq also uses _CPC object to determine the boost ratio. When CPPC is disabled in BIOS something is going wrong and max capacity is becoming zero.

Hi Vincent, Qais,

I have collected some data with bpftracing:

sudo bpftrace -e 'kretprobe:effective_cpu_util /cpu == 1/ { @eff_util = lhist(retval, 0, 1200, 50);} kprobe:get_next_freq /cpu == 1/ { @sugov_eff_util = lhist(arg1, 0, 1200, 50); @sugov_max_cap = lhist(arg2, 0, 1000, 2);} kretprobe:get_next_freq /cpu == 1/ { @sugov_freq = lhist(retval, 1000000, 5000000, 100000);}'

with running: taskset -c 1 make

issue case:

Attaching 3 probes...
@eff_util:

[0, 50)             1263 |@                                                   |
[50, 100)            517 |                                                    |
[100, 150)           233 |                                                    |
[150, 200)           297 |                                                    |
[200, 250)           162 |                                                    |
[250, 300)            98 |                                                    |
[300, 350)            75 |                                                    |
[350, 400)           205 |                                                    |
[400, 450)           210 |                                                    |
[450, 500)            16 |                                                    |
[500, 550)          1532 |@                                                   |
[550, 600)          1026 |                                                    |
[600, 650)           761 |                                                    |

[650, 700)           876 |                                                    |
[700, 750)          1085 |                                                    |
[750, 800)           891 |                                                    |
[800, 850)           816 |                                                    |
[850, 900)           983 |                                                    |
[900, 950)           661 |                                                    |
[950, 1000)          759 |                                                    |
[1000, 1050)       57433 |@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@|

@sugov_eff_util:

[0, 50)             1074 |                                                    |
[50, 100)            571 |                                                    |
[100, 150)           259 |                                                    |
[150, 200)           169 |                                                    |
[200, 250)           237 |                                                    |
[250, 300)           156 |                                                    |
[300, 350)            91 |                                                    |
[350, 400)            46 |                                                    |
[400, 450)            52 |                                                    |
[450, 500)           195 |                                                    |
[500, 550)           175 |                                                    |
[550, 600)            46 |                                                    |
[600, 650)           493 |                                                    |
[650, 700)          1424 |@                                                   |
[700, 750)           646 |                                                    |
[750, 800)           628 |                                                    |
[800, 850)           612 |                                                    |
[850, 900)           840 |                                                    |
[900, 950)           893 |                                                    |
[950, 1000)          640 |                                                    |
[1000, 1050)       60679 |@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@|

@sugov_freq:
[1400000, 1500000) 69911 |@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@|

@sugov_max_cap:
[0, 2) 69926 |@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@|

good case:

Attaching 3 probes...
@eff_util:

[0, 50)              246 |@                                                   |
[50, 100)            150 |@                                                   |
[100, 150)           191 |@                                                   |
[150, 200)           239 |@                                                   |
[200, 250)           117 |                                                    |
[250, 300)          2101 |@@@@@@@@@@@@@@@                                     |
[300, 350)          2284 |@@@@@@@@@@@@@@@@                                    |
[350, 400)           713 |@@@@@                                               |
[400, 450)           151 |@                                                   |
[450, 500)           154 |@                                                   |
[500, 550)          1121 |@@@@@@@@                                            |
[550, 600)          1901 |@@@@@@@@@@@@@                                       |
[600, 650)          1208 |@@@@@@@@                                            |
[650, 700)           606 |@@@@                                                |
[700, 750)           557 |@@@                                                 |
[750, 800)           872 |@@@@@@                                              |
[800, 850)          1092 |@@@@@@@                                             |
[850, 900)          1416 |@@@@@@@@@@                                          |
[900, 950)          1107 |@@@@@@@                                             |
[950, 1000)         1051 |@@@@@@@                                             |
[1000, 1050)        7260 |@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@|

@sugov_eff_util:

[0, 50)              241 |                                                    |
[50, 100)            149 |                                                    |
[100, 150)            72 |                                                    |
[150, 200)            95 |                                                    |
[200, 250)            43 |                                                    |
[250, 300)            49 |                                                    |
[300, 350)            19 |                                                    |
[350, 400)            56 |                                                    |
[400, 450)            22 |                                                    |
[450, 500)            29 |                                                    |
[500, 550)          1840 |@@@@@@                                              |
[550, 600)          1476 |@@@@@                                               |
[600, 650)          1027 |@@@                                                 |
[650, 700)           473 |@                                                   |
[700, 750)           366 |@                                                   |
[750, 800)           627 |@@                                                  |
[800, 850)           930 |@@@                                                 |
[850, 900)          1285 |@@@@                                                |
[900, 950)           971 |@@@                                                 |
[950, 1000)          946 |@@@                                                 |
[1000, 1050)       13839 |@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@|

@sugov_freq:

[1400000, 1500000)     648 |@                                                   |
[1500000, 1600000)       0 |                                                    |
[1600000, 1700000)       0 |                                                    |
[1700000, 1800000)      25 |                                                    |
[1800000, 1900000)       0 |                                                    |
[1900000, 2000000)       0 |                                                    |
[2000000, 2100000)       0 |                                                    |
[2100000, 2200000)       0 |                                                    |
[2200000, 2300000)       0 |                                                    |
[2300000, 2400000)       0 |                                                    |
[2400000, 2500000)       0 |                                                    |
[2500000, 2600000)       0 |                                                    |
[2600000, 2700000)       0 |                                                    |
[2700000, 2800000)       0 |                                                    |
[2800000, 2900000)       0 |                                                    |
[2900000, 3000000)       0 |                                                    |
[3000000, 3100000)       0 |                                                    |
[3100000, 3125K)       0 |                                                    |
[3125K, 3300000)       0 |                                                    |
[3300000, 3400000)       0 |                                                    |
[3400000, 3500000)       0 |                                                    |
[3500000, 3600000)       0 |                                                    |
[3600000, 3700000)       0 |                                                    |
[3700000, 3800000)       0 |                                                    |
[3800000, 3900000)       0 |                                                    |

[3900000, 4000000) 23879 |@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@|

@sugov_max_cap:
[0, 2) 24555 |@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@|

In both case max_cap is zero but selected freq is incorrect in bad case.

Thanks,
Wyes

Hi Wyes,

Quoted 39 lines · Wyes Karny

Quoted 22 lines · Linus Torvalds

Quoted 8 lines · Linus Torvalds

It's one of these two:

f12560779f9d sched/cpufreq: Rework iowait boost
9c0b4bb7f630 sched/cpufreq: Rework schedutil governor performance estimation

one more boot to go, then I'll try to revert whichever causes my machine to perform horribly much worse.

I guess it should come as no surprise that the result is

9c0b4bb7f6303c9c4e2e34984c46f5a86478f84d is the first bad commit

but to revert cleanly I will have to revert all of

b3edde44e5d4 ("cpufreq/schedutil: Use a fixed reference frequency") f12560779f9d ("sched/cpufreq: Rework iowait boost") 9c0b4bb7f630 ("sched/cpufreq: Rework schedutil governor
performance estimation")

Quoted 5 lines

This is on a 32-core (64-thread) AMD Ryzen Threadripper 3970X, fwiw.

I'll keep that revert in my private test-tree for now (so that I have a working machine again), but I'll move it to my main branch soon unless somebody has a quick fix for this problem.

Hi Linus,

I'm able to reproduce this issue with my AMD Ryzen 5600G system. But only if I disable CPPC in BIOS and boot with acpi-cpufreq + schedutil. (I believe for your case also CPPC is diabled as log "_CPC object is not present" came). Enabling CPPC in BIOS issue not seen in my system. For AMD acpi-cpufreq also uses _CPC object to determine the boost ratio. When CPPC is disabled in BIOS something is going wrong and max capacity is becoming zero.

Quoted 3 lines

Hi Vincent, Qais,

I have collected some data with bpftracing:

Thanks for your tests results

Quoted 22 lines

sudo bpftrace -e 'kretprobe:effective_cpu_util /cpu == 1/ { @eff_util = lhist(retval, 0, 1200, 50);} kprobe:get_next_freq /cpu == 1/ { @sugov_eff_util = lhist(arg1, 0, 1200, 50); @sugov_max_cap = lhist(arg2, 0, 1000, 2);} kretprobe:get_next_freq /cpu == 1/ { @sugov_freq = lhist(retval, 1000000, 5000000, 100000);}'

with running: taskset -c 1 make

issue case:

Attaching 3 probes...
@eff_util:

[0, 50)             1263 |@                                                   |
[50, 100)            517 |                                                    |
[100, 150)           233 |                                                    |
[150, 200)           297 |                                                    |
[200, 250)           162 |                                                    |
[250, 300)            98 |                                                    |
[300, 350)            75 |                                                    |
[350, 400)           205 |                                                    |
[400, 450)           210 |                                                    |
[450, 500)            16 |                                                    |
[500, 550)          1532 |@                                                   |
[550, 600)          1026 |                                                    |
[600, 650)           761 |                                                    |

Quoted 9 lines

[650, 700)           876 |                                                    |
[700, 750)          1085 |                                                    |
[750, 800)           891 |                                                    |
[800, 850)           816 |                                                    |
[850, 900)           983 |                                                    |
[900, 950)           661 |                                                    |
[950, 1000)          759 |                                                    |
[1000, 1050)       57433 |@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@|

ok so the output of effective_cpu_util() seems correct or at least to maw utilization value. In order to be correct, it means that arch_scale_cpu_capacity(cpu) is not zero because of :

unsigned long effective_cpu_util(int cpu, unsigned long util_cfs,
unsigned long *min, unsigned long *max)

{
	unsigned long util, irq, scale;
	struct rq *rq = cpu_rq(cpu);

	scale = arch_scale_cpu_capacity(cpu);

	/*
	 * Early check to see if IRQ/steal time saturates the CPU, can be
	 * because of inaccuracies in how we track these -- see
	 * update_irq_load_avg().
	 */
	irq = cpu_util_irq(rq);
	if (unlikely(irq >= scale)) {
		if (min)
			*min = scale;
		if (max)
			*max = scale;
		return scale;
	}

...
}

If arch_scale_cpu_capacity(cpu) returns 0 then effective_cpu_util() should returns 0 too.

Now see below

Quoted 23 lines

@sugov_eff_util:

[0, 50)             1074 |                                                    |
[50, 100)            571 |                                                    |
[100, 150)           259 |                                                    |
[150, 200)           169 |                                                    |
[200, 250)           237 |                                                    |
[250, 300)           156 |                                                    |
[300, 350)            91 |                                                    |
[350, 400)            46 |                                                    |
[400, 450)            52 |                                                    |
[450, 500)           195 |                                                    |
[500, 550)           175 |                                                    |
[550, 600)            46 |                                                    |
[600, 650)           493 |                                                    |
[650, 700)          1424 |@                                                   |
[700, 750)           646 |                                                    |
[750, 800)           628 |                                                    |
[800, 850)           612 |                                                    |
[850, 900)           840 |                                                    |
[900, 950)           893 |                                                    |
[950, 1000)          640 |                                                    |
[1000, 1050)       60679 |@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@|

Quoted 5 lines

@sugov_freq:
[1400000, 1500000) 69911 |@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@|

@sugov_max_cap:
[0, 2) 69926 |@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@|

In get_next_freq(struct sugov_policy *sg_policy, unsigned long util, unsigned long max)

max is 0 and we comes from this path:

static void sugov_update_single_freq(struct update_util_data *hook, u64 time,
unsigned int flags)
{

...

	max_cap = arch_scale_cpu_capacity(sg_cpu->cpu);

	if (!sugov_update_single_common(sg_cpu, time, max_cap, flags))
		return;

	next_f = get_next_freq(sg_policy, sg_cpu->util, max_cap);

...

so here arch_scale_cpu_capacity(sg_cpu->cpu) returns 0 ...

AFAICT, AMD platform uses the default
static __always_inline
unsigned long arch_scale_cpu_capacity(int cpu)

{
	return SCHED_CAPACITY_SCALE;
}

I'm missing something here

Quoted 77 lines

good case:

Attaching 3 probes...
@eff_util:

[0, 50)              246 |@                                                   |
[50, 100)            150 |@                                                   |
[100, 150)           191 |@                                                   |
[150, 200)           239 |@                                                   |
[200, 250)           117 |                                                    |
[250, 300)          2101 |@@@@@@@@@@@@@@@                                     |
[300, 350)          2284 |@@@@@@@@@@@@@@@@                                    |
[350, 400)           713 |@@@@@                                               |
[400, 450)           151 |@                                                   |
[450, 500)           154 |@                                                   |
[500, 550)          1121 |@@@@@@@@                                            |
[550, 600)          1901 |@@@@@@@@@@@@@                                       |
[600, 650)          1208 |@@@@@@@@                                            |
[650, 700)           606 |@@@@                                                |
[700, 750)           557 |@@@                                                 |
[750, 800)           872 |@@@@@@                                              |
[800, 850)          1092 |@@@@@@@                                             |
[850, 900)          1416 |@@@@@@@@@@                                          |
[900, 950)          1107 |@@@@@@@                                             |
[950, 1000)         1051 |@@@@@@@                                             |
[1000, 1050)        7260 |@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@|

@sugov_eff_util:

[0, 50)              241 |                                                    |
[50, 100)            149 |                                                    |
[100, 150)            72 |                                                    |
[150, 200)            95 |                                                    |
[200, 250)            43 |                                                    |
[250, 300)            49 |                                                    |
[300, 350)            19 |                                                    |
[350, 400)            56 |                                                    |
[400, 450)            22 |                                                    |
[450, 500)            29 |                                                    |
[500, 550)          1840 |@@@@@@                                              |
[550, 600)          1476 |@@@@@                                               |
[600, 650)          1027 |@@@                                                 |
[650, 700)           473 |@                                                   |
[700, 750)           366 |@                                                   |
[750, 800)           627 |@@                                                  |
[800, 850)           930 |@@@                                                 |
[850, 900)          1285 |@@@@                                                |
[900, 950)           971 |@@@                                                 |
[950, 1000)          946 |@@@                                                 |
[1000, 1050)       13839 |@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@|

@sugov_freq:

[1400000, 1500000)     648 |@                                                   |
[1500000, 1600000)       0 |                                                    |
[1600000, 1700000)       0 |                                                    |
[1700000, 1800000)      25 |                                                    |
[1800000, 1900000)       0 |                                                    |
[1900000, 2000000)       0 |                                                    |
[2000000, 2100000)       0 |                                                    |
[2100000, 2200000)       0 |                                                    |
[2200000, 2300000)       0 |                                                    |
[2300000, 2400000)       0 |                                                    |
[2400000, 2500000)       0 |                                                    |
[2500000, 2600000)       0 |                                                    |
[2600000, 2700000)       0 |                                                    |
[2700000, 2800000)       0 |                                                    |
[2800000, 2900000)       0 |                                                    |
[2900000, 3000000)       0 |                                                    |
[3000000, 3100000)       0 |                                                    |
[3100000, 3125K)       0 |                                                    |
[3125K, 3300000)       0 |                                                    |
[3300000, 3400000)       0 |                                                    |
[3400000, 3500000)       0 |                                                    |
[3500000, 3600000)       0 |                                                    |
[3600000, 3700000)       0 |                                                    |
[3700000, 3800000)       0 |                                                    |
[3800000, 3900000)       0 |                                                    |

Quoted 6 lines

[3900000, 4000000) 23879 |@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@|

@sugov_max_cap:
[0, 2) 24555 |@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@|

In both case max_cap is zero but selected freq is incorrect in bad case.

Also we have in get_next_freq():

	freq = map_util_freq(util, freq, max);
	       --> util * freq /max

If max was 0, we should have been an error ?

There is something strange that I don't understand

Could you trace on the return of sugov_get_util()
the value of sg_cpu->util ?

Thanks for you help
Vincent

Quoted 4 lines

Thanks,
Wyes

Quoted 6 lines · Vincent Guittot

Hi Wyes,

Le dimanche 14 janv. 2024 � 14:42:40 (+0530), Wyes Karny a �crit :

Quoted 3 lines · Linus Torvalds

Quoted 2 lines

On Wed, 10 Jan 2024 at 14:41, Linus Torvalds
<[email protected]> wrote:

Yeah, correct something was wrong in the bpftrace readings, max_cap is not zero in traces.

             git-5511    [001] d.h1.   427.159763: get_next_freq.constprop.0: [DEBUG] : freq 1400000, util 1024, max 1024
             git-5511    [001] d.h1.   427.163733: sugov_get_util: [DEBUG] : util 1024, sg_cpu->util 1024
             git-5511    [001] d.h1.   427.163735: get_next_freq.constprop.0: [DEBUG] : freq 1400000, util 1024, max 1024
             git-5511    [001] d.h1.   427.167706: sugov_get_util: [DEBUG] : util 1024, sg_cpu->util 1024
             git-5511    [001] d.h1.   427.167708: get_next_freq.constprop.0: [DEBUG] : freq 1400000, util 1024, max 1024
             git-5511    [001] d.h1.   427.171678: sugov_get_util: [DEBUG] : util 1024, sg_cpu->util 1024
             git-5511    [001] d.h1.   427.171679: get_next_freq.constprop.0: [DEBUG] : freq 1400000, util 1024, max 1024
             git-5511    [001] d.h1.   427.175653: sugov_get_util: [DEBUG] : util 1024, sg_cpu->util 1024
             git-5511    [001] d.h1.   427.175655: get_next_freq.constprop.0: [DEBUG] : freq 1400000, util 1024, max 1024
             git-5511    [001] d.s1.   427.175665: sugov_get_util: [DEBUG] : util 1024, sg_cpu->util 1024
             git-5511    [001] d.s1.   427.175665: get_next_freq.constprop.0: [DEBUG] : freq 1400000, util 1024, max 1024

Debug patch applied:

kernel/sched/cpufreq_schedutil.c +2 -0

index 95c3c097083e..5c9b3e1de7a0 100644
@@ -166,6 +166,7 @@ static unsigned int get_next_freq(struct sugov_policy *sg_policy,
 
        freq = get_capacity_ref_freq(policy);
        freq = map_util_freq(util, freq, max);
+       trace_printk("[DEBUG] : freq %llu, util %llu, max %llu\n", freq, util, max);
 
        if (freq == sg_policy->cached_raw_freq && !sg_policy->need_freq_update)
                return sg_policy->next_freq;

@@ -199,6 +200,7 @@ static void sugov_get_util(struct sugov_cpu *sg_cpu, unsigned long boost)
        util = max(util, boost);
        sg_cpu->bw_min = min;
        sg_cpu->util = sugov_effective_cpu_perf(sg_cpu->cpu, util, min, max);
+       trace_printk("[DEBUG] : util %llu, sg_cpu->util %llu\n", util, sg_cpu->util);
 }
 
 /**

So, I guess map_util_freq going wrong somewhere.

Quoted 7 lines · Wyes Karny

Quoted 6 lines · Vincent Guittot

Hi Wyes,

Le dimanche 14 janv. 2024 � 14:42:40 (+0530), Wyes Karny a �crit :

Quoted 3 lines · Linus Torvalds

Quoted 2 lines

On Wed, 10 Jan 2024 at 14:41, Linus Torvalds
<[email protected]> wrote:

[...]

Quoted 39 lines

Yeah, correct something was wrong in the bpftrace readings, max_cap is not zero in traces.

             git-5511    [001] d.h1.   427.159763: get_next_freq.constprop.0: [DEBUG] : freq 1400000, util 1024, max 1024
             git-5511    [001] d.h1.   427.163733: sugov_get_util: [DEBUG] : util 1024, sg_cpu->util 1024
             git-5511    [001] d.h1.   427.163735: get_next_freq.constprop.0: [DEBUG] : freq 1400000, util 1024, max 1024
             git-5511    [001] d.h1.   427.167706: sugov_get_util: [DEBUG] : util 1024, sg_cpu->util 1024
             git-5511    [001] d.h1.   427.167708: get_next_freq.constprop.0: [DEBUG] : freq 1400000, util 1024, max 1024
             git-5511    [001] d.h1.   427.171678: sugov_get_util: [DEBUG] : util 1024, sg_cpu->util 1024
             git-5511    [001] d.h1.   427.171679: get_next_freq.constprop.0: [DEBUG] : freq 1400000, util 1024, max 1024
             git-5511    [001] d.h1.   427.175653: sugov_get_util: [DEBUG] : util 1024, sg_cpu->util 1024
             git-5511    [001] d.h1.   427.175655: get_next_freq.constprop.0: [DEBUG] : freq 1400000, util 1024, max 1024
             git-5511    [001] d.s1.   427.175665: sugov_get_util: [DEBUG] : util 1024, sg_cpu->util 1024
             git-5511    [001] d.s1.   427.175665: get_next_freq.constprop.0: [DEBUG] : freq 1400000, util 1024, max 1024

Debug patch applied:

kernel/sched/cpufreq_schedutil.c +2 -0

index 95c3c097083e..5c9b3e1de7a0 100644
@@ -166,6 +166,7 @@ static unsigned int get_next_freq(struct sugov_policy *sg_policy,
 
        freq = get_capacity_ref_freq(policy);
        freq = map_util_freq(util, freq, max);
+       trace_printk("[DEBUG] : freq %llu, util %llu, max %llu\n", freq, util, max);
 
        if (freq == sg_policy->cached_raw_freq && !sg_policy->need_freq_update)
                return sg_policy->next_freq;

@@ -199,6 +200,7 @@ static void sugov_get_util(struct sugov_cpu *sg_cpu, unsigned long boost)
        util = max(util, boost);
        sg_cpu->bw_min = min;
        sg_cpu->util = sugov_effective_cpu_perf(sg_cpu->cpu, util, min, max);
+       trace_printk("[DEBUG] : util %llu, sg_cpu->util %llu\n", util, sg_cpu->util);
 }
 
 /**

Quoted 1 line

So, I guess map_util_freq going wrong somewhere.

sugov_update_single_freq() -> get_next_freq() -> get_capacity_ref_freq()

Is arch_scale_freq_invariant() true in both cases, so that get_capacity_ref_freq() returns 'policy->cpuinfo.max_freq' and not just 'policy->cur'?

Quoted 44 lines · Vincent Guittot

Hi Wyes,

Quoted 41 lines · Wyes Karny

Quoted 22 lines · Linus Torvalds

Quoted 8 lines · Linus Torvalds

It's one of these two:

f12560779f9d sched/cpufreq: Rework iowait boost
9c0b4bb7f630 sched/cpufreq: Rework schedutil governor performance estimation

one more boot to go, then I'll try to revert whichever causes my machine to perform horribly much worse.

I guess it should come as no surprise that the result is

9c0b4bb7f6303c9c4e2e34984c46f5a86478f84d is the first bad commit

but to revert cleanly I will have to revert all of

b3edde44e5d4 ("cpufreq/schedutil: Use a fixed reference frequency") f12560779f9d ("sched/cpufreq: Rework iowait boost") 9c0b4bb7f630 ("sched/cpufreq: Rework schedutil governor
performance estimation")

Quoted 5 lines

This is on a 32-core (64-thread) AMD Ryzen Threadripper 3970X, fwiw.

I'll keep that revert in my private test-tree for now (so that I have a working machine again), but I'll move it to my main branch soon unless somebody has a quick fix for this problem.

Hi Linus,

I'm able to reproduce this issue with my AMD Ryzen 5600G system. But only if I disable CPPC in BIOS and boot with acpi-cpufreq + schedutil. (I believe for your case also CPPC is diabled as log "_CPC object is not present" came). Enabling CPPC in BIOS issue not seen in my system. For AMD acpi-cpufreq also uses _CPC object to determine the boost ratio. When CPPC is disabled in BIOS something is going wrong and max capacity is becoming zero.

Hi Vincent, Qais,

Quoted 5 lines

There is something strange that I don't understand

Could you trace on the return of sugov_get_util()
the value of sg_cpu->util ?

Yeah, correct something was wrong in the bpftrace readings, max_cap is not zero in traces.

             git-5511    [001] d.h1.   427.159763: get_next_freq.constprop.0: [DEBUG] : freq 1400000, util 1024, max 1024
             git-5511    [001] d.h1.   427.163733: sugov_get_util: [DEBUG] : util 1024, sg_cpu->util 1024
             git-5511    [001] d.h1.   427.163735: get_next_freq.constprop.0: [DEBUG] : freq 1400000, util 1024, max 1024
             git-5511    [001] d.h1.   427.167706: sugov_get_util: [DEBUG] : util 1024, sg_cpu->util 1024
             git-5511    [001] d.h1.   427.167708: get_next_freq.constprop.0: [DEBUG] : freq 1400000, util 1024, max 1024
             git-5511    [001] d.h1.   427.171678: sugov_get_util: [DEBUG] : util 1024, sg_cpu->util 1024
             git-5511    [001] d.h1.   427.171679: get_next_freq.constprop.0: [DEBUG] : freq 1400000, util 1024, max 1024
             git-5511    [001] d.h1.   427.175653: sugov_get_util: [DEBUG] : util 1024, sg_cpu->util 1024
             git-5511    [001] d.h1.   427.175655: get_next_freq.constprop.0: [DEBUG] : freq 1400000, util 1024, max 1024
             git-5511    [001] d.s1.   427.175665: sugov_get_util: [DEBUG] : util 1024, sg_cpu->util 1024
             git-5511    [001] d.s1.   427.175665: get_next_freq.constprop.0: [DEBUG] : freq 1400000, util 1024, max 1024

Debug patch applied:

kernel/sched/cpufreq_schedutil.c +2 -0

index 95c3c097083e..5c9b3e1de7a0 100644
@@ -166,6 +166,7 @@ static unsigned int get_next_freq(struct sugov_policy *sg_policy,
 
        freq = get_capacity_ref_freq(policy);
        freq = map_util_freq(util, freq, max);
+       trace_printk("[DEBUG] : freq %llu, util %llu, max %llu\n", freq, util, max);
 
        if (freq == sg_policy->cached_raw_freq && !sg_policy->need_freq_update)
                return sg_policy->next_freq;

@@ -199,6 +200,7 @@ static void sugov_get_util(struct sugov_cpu *sg_cpu, unsigned long boost)
        util = max(util, boost);
        sg_cpu->bw_min = min;
        sg_cpu->util = sugov_effective_cpu_perf(sg_cpu->cpu, util, min, max);
+       trace_printk("[DEBUG] : util %llu, sg_cpu->util %llu\n", util, sg_cpu->util);
 }
 
 /**

So, I guess map_util_freq going wrong somewhere.

Thanks,
Wyes

Quoted 8 lines

Thanks for you help
Vincent

Quoted 4 lines

Thanks,
Wyes

Thanks for the trace. It was really helpful and I think that I got the root cause.

The problem comes from get_capacity_ref_freq() which returns current freq when arch_scale_freq_invariant() is not enable, and the fact that we apply map_util_perf() earlier in the path now which is then capped by max capacity.

Could you try the below ?

kernel/sched/cpufreq_schedutil.c +1 -1

index e420e2ee1a10..611c621543f4 100644
@@ -133,7 +133,7 @@ unsigned long get_capacity_ref_freq(struct
 cpufreq_policy *policy)
        if (arch_scale_freq_invariant())
                return policy->cpuinfo.max_freq;
 
-       return policy->cur;
+       return policy->cur + policy->cur >> 2;
 }
 

/**

Quoted 19 lines · Vincent Guittot

Thanks for the trace. It was really helpful and I think that I got the root cause.

The problem comes from get_capacity_ref_freq() which returns current freq when arch_scale_freq_invariant() is not enable, and the fact that we apply map_util_perf() earlier in the path now which is then capped by max capacity.

Could you try the below ?

kernel/sched/cpufreq_schedutil.c +0 -0

index e420e2ee1a10..611c621543f4 100644
@@ -133,7 +133,7 @@ unsigned long get_capacity_ref_freq(struct
 cpufreq_policy *policy)
        if (arch_scale_freq_invariant())
                return policy->cpuinfo.max_freq;
 

Quoted 5 lines

kernel/sched/cpufreq_schedutil.c +1 -1

-       return policy->cur;
+       return policy->cur + policy->cur >> 2;
 }
 

/**

Is this a test patch or a proper fix? I can't see it being the latter. It seems

the current logic fails when util is already 1024, and I think we're trying to fix the invariance issue too late.

Is the problem that we can't read policy->cur in the scheduler to fix the util while it's being updated that's why it's done here in this case?

If this is the problem, shouldn't the logic be if util is max then always go to max frequency? I don't think we have enough info to correct the invariance here IIUC. All we can see the system is saturated at this frequency and whether a small jump or a big jump is required is hard to tell.

Something like this

kernel/sched/cpufreq_schedutil.c +0 -0

index 95c3c097083e..473d0352030b 100644
@@ -164,8 +164,12 @@ static unsigned int get_next_freq(struct sugov_policy *sg_policy,
        struct cpufreq_policy *policy = sg_policy->policy;

kernel/sched/cpufreq_schedutil.c +0 -0

        if (freq == sg_policy->cached_raw_freq && !sg_policy->need_freq_update)
                return sg_policy->next_freq;

Quoted 22 lines · Qais Yousef

Quoted 19 lines · Vincent Guittot

Thanks for the trace. It was really helpful and I think that I got the root cause.

The problem comes from get_capacity_ref_freq() which returns current freq when arch_scale_freq_invariant() is not enable, and the fact that we apply map_util_perf() earlier in the path now which is then capped by max capacity.

Could you try the below ?

kernel/sched/cpufreq_schedutil.c +0 -0

index e420e2ee1a10..611c621543f4 100644
@@ -133,7 +133,7 @@ unsigned long get_capacity_ref_freq(struct
 cpufreq_policy *policy)
        if (arch_scale_freq_invariant())
                return policy->cpuinfo.max_freq;
 

Quoted 7 lines

Quoted 5 lines

kernel/sched/cpufreq_schedutil.c +1 -1

-       return policy->cur;
+       return policy->cur + policy->cur >> 2;
 }
 

/**

Is this a test patch or a proper fix? I can't see it being the latter. It seems

It's a proper fix. It's the same mechanism that is used already :
- Either you add margin on the utilization to go above current freq
before it is fully used. This si what was done previously
- or you add margin on the freq range to select a higher freq than
current one before it become fully used

Quoted 22 lines

the current logic fails when util is already 1024, and I think we're trying to fix the invariance issue too late.

Is the problem that we can't read policy->cur in the scheduler to fix the util while it's being updated that's why it's done here in this case?

If this is the problem, shouldn't the logic be if util is max then always go to max frequency? I don't think we have enough info to correct the invariance here IIUC. All we can see the system is saturated at this frequency and whether a small jump or a big jump is required is hard to tell.

Something like this

kernel/sched/cpufreq_schedutil.c +0 -0

index 95c3c097083e..473d0352030b 100644
@@ -164,8 +164,12 @@ static unsigned int get_next_freq(struct sugov_policy *sg_policy,
        struct cpufreq_policy *policy = sg_policy->policy;
        unsigned int freq;
 

kernel/sched/cpufreq_schedutil.c static unsigned int get_next_freq(struct sugov_policy *sg_policy, · line 172

Quoted 8 lines

kernel/sched/cpufreq_schedutil.c +6 -2

-       freq = get_capacity_ref_freq(policy);
-       freq = map_util_freq(util, freq, max);
+       if (util != max) {
+               freq = get_capacity_ref_freq(policy);
+               freq = map_util_freq(util, freq, max);
+       } else {
+               freq = policy->cpuinfo.max_freq;
+       }

This is not correct because you will have to wait to reach full utilization at the current OPP possibly the lowest OPP before moving directly to max OPP

Quoted 4 lines

kernel/sched/cpufreq_schedutil.c +0 -0

        if (freq == sg_policy->cached_raw_freq && !sg_policy->need_freq_update)
                return sg_policy->next_freq;

Quoted 3 lines

This is not correct because you will have to wait to reach full utilization at the current OPP possibly the lowest OPP before moving directly to max OPP

Isn't this already the case? The ratio (util+headroom/max) will be less than 1 until util is 80% (with 25% headroom). And for all values <= 80% * max, we will request a frequency smaller than/equal policy->cur, no?

ie:

	util = 600
	max = 1024

	freq = 1.25 * 600 * policy->cur / 1024 = 0.73 * policy->cur

(util+headroom/max) must be greater than 1 for us to start going above policy->cur - which seems to have been working by accident IIUC.

So yes my proposal is incorrect, but it seems the conversion is not right to me now.

I could reproduce the problem now (thanks Wyes!). I have 3 freqs on my system

2.2GHz, 2.8GHz and 3.8GHz

which (I believe) translates into capacities

~592, ~754, 1024

which means we should pick 2.8GHz as soon as util * 1.25 > 592; which translates into util = ~473.

But what I see is that we go to 2.8GHz when we jump from 650 to 680 (see attached picture), which is what you'd expect since we apply two headrooms now, which means the ratio (util+headroom/max) will be greater than 1 after go above this value

	1024 * 0.8 * 0.8 = ~655

So I think the math makes sense logically, but we're missing some other correction factor.

When I re-enable CPPC I see for the same test that we go into 3.8GHz straight away. My test is simple busyloop via

cat /dev/zero > /dev/null

I see the CPU util_avg is at 523 at fork. I expected us to run to 2.8GHz here to be honest, but I am not sure if util_cfs_boost() and util_est() are maybe causing us to be slightly above 523 and that's why we start with max freq.

Or I've done the math wrong :-) But the two don't behave the same for the same kernel with and without CPPC.

Quoted 51 lines · Qais Yousef

Quoted 3 lines

This is not correct because you will have to wait to reach full utilization at the current OPP possibly the lowest OPP before moving directly to max OPP

Isn't this already the case? The ratio (util+headroom/max) will be less than 1 until util is 80% (with 25% headroom). And for all values <= 80% * max, we will request a frequency smaller than/equal policy->cur, no?

ie:

      util = 600
      max = 1024

      freq = 1.25 * 600 * policy->cur / 1024 = 0.73 * policy->cur

(util+headroom/max) must be greater than 1 for us to start going above policy->cur - which seems to have been working by accident IIUC.

So yes my proposal is incorrect, but it seems the conversion is not right to me now.

I could reproduce the problem now (thanks Wyes!). I have 3 freqs on my system

2.2GHz, 2.8GHz and 3.8GHz

which (I believe) translates into capacities

~592, ~754, 1024

which means we should pick 2.8GHz as soon as util * 1.25 > 592; which translates into util = ~473.

But what I see is that we go to 2.8GHz when we jump from 650 to 680 (see attached picture), which is what you'd expect since we apply two headrooms now, which means the ratio (util+headroom/max) will be greater than 1 after go above this value

      1024 * 0.8 * 0.8 = ~655

So I think the math makes sense logically, but we're missing some other correction factor.

When I re-enable CPPC I see for the same test that we go into 3.8GHz straight away. My test is simple busyloop via

cat /dev/zero > /dev/null

I see the CPU util_avg is at 523 at fork. I expected us to run to 2.8GHz here to be honest, but I am not sure if util_cfs_boost() and util_est() are maybe causing us to be slightly above 523 and that's why we start with max freq.

Quoted 2 lines

Or I've done the math wrong :-) But the two don't behave the same for the same kernel with and without CPPC.

I think the relationship should be:

freq = util * f_curr / cap_curr

(patch below)

with that I see (almost) the expected behavior (picture attached). We go to 2.8GHz when we are above 500. But the move to 3.8GHz is a bit earlier at 581 (instead of 754 * 0.8 = 603). Not sure why. With 25% headroom 581 is 726. So it's a tad too early.

kernel/sched/cpufreq_schedutil.c +2 -1

index 95c3c097083e..155f96a44fa0 100644
@@ -123,7 +123,8 @@ static void sugov_deferred_update(struct sugov_policy *sg_policy)
  * Return: the reference CPU frequency to compute a capacity.
  */
 static __always_inline
-unsigned long get_capacity_ref_freq(struct cpufreq_policy *policy)
+unsigned long get_capacity_ref_freq(struct cpufreq_policy *policy,
+                                   unsigned long *max)
 {
        unsigned int freq = arch_scale_freq_ref(policy->cpu);
 

kernel/sched/cpufreq_schedutil.c +2 -1

+
        return policy->cur;
 }
 

@@ -164,7 +168,7 @@ static unsigned int get_next_freq(struct sugov_policy *sg_policy,
        struct cpufreq_policy *policy = sg_policy->policy;
        unsigned int freq;
 
-       freq = get_capacity_ref_freq(policy);
+       freq = get_capacity_ref_freq(policy, &max);
        freq = map_util_freq(util, freq, max);
 
        if (freq == sg_policy->cached_raw_freq && !sg_policy->need_freq_update)

Quoted 14 lines

kernel/sched/cpufreq_schedutil.c +2 -1

index 95c3c097083e..155f96a44fa0 100644
@@ -123,7 +123,8 @@ static void sugov_deferred_update(struct sugov_policy *sg_policy)
  * Return: the reference CPU frequency to compute a capacity.
  */
 static __always_inline
-unsigned long get_capacity_ref_freq(struct cpufreq_policy *policy)
+unsigned long get_capacity_ref_freq(struct cpufreq_policy *policy,
+                                   unsigned long *max)
 {
        unsigned int freq = arch_scale_freq_ref(policy->cpu);
 

kernel/sched/cpufreq_schedutil.c unsigned long get_capacity_ref_freq(struct cpufreq_policy *policy) · line 138

Quoted 6 lines

kernel/sched/cpufreq_schedutil.c +2 -0

@@ -133,6 +134,9 @@ unsigned long get_capacity_ref_freq(struct cpufreq_policy *policy)
        if (arch_scale_freq_invariant())
                return policy->cpuinfo.max_freq;
 
+       if (max)
+               *max = policy->cur * (*max) / policy->cpuinfo.max_freq;

But when freq_invaiant is disabled we don't have policy->cpuinfo.max_freq.

Quoted 31 lines · Vincent Guittot

Quoted 29 lines · Qais Yousef

Quoted 23 lines · Vincent Guittot

Thanks for the trace. It was really helpful and I think that I got the root cause.

The problem comes from get_capacity_ref_freq() which returns current freq when arch_scale_freq_invariant() is not enable, and the fact that we apply map_util_perf() earlier in the path now which is then capped by max capacity.

Could you try the below ?

kernel/sched/cpufreq_schedutil.c +1 -1

index e420e2ee1a10..611c621543f4 100644
@@ -133,7 +133,7 @@ unsigned long get_capacity_ref_freq(struct
 cpufreq_policy *policy)
        if (arch_scale_freq_invariant())
                return policy->cpuinfo.max_freq;
 
-       return policy->cur;
+       return policy->cur + policy->cur >> 2;
 }
 

Quoted 1 line

/**

Is this a test patch or a proper fix? I can't see it being the latter. It seems

Quoted 5 lines

It's a proper fix. It's the same mechanism that is used already :
- Either you add margin on the utilization to go above current freq
before it is fully used. This si what was done previously
- or you add margin on the freq range to select a higher freq than
current one before it become fully used

Aren't we applying the 25% headroom twice then?

Quoted 23 lines

Quoted 23 lines

the current logic fails when util is already 1024, and I think we're trying to fix the invariance issue too late.

Is the problem that we can't read policy->cur in the scheduler to fix the util while it's being updated that's why it's done here in this case?

If this is the problem, shouldn't the logic be if util is max then always go to max frequency? I don't think we have enough info to correct the invariance here IIUC. All we can see the system is saturated at this frequency and whether a small jump or a big jump is required is hard to tell.

Something like this

kernel/sched/cpufreq_schedutil.c +0 -0

index 95c3c097083e..473d0352030b 100644
@@ -164,8 +164,12 @@ static unsigned int get_next_freq(struct sugov_policy *sg_policy,
        struct cpufreq_policy *policy = sg_policy->policy;
        unsigned int freq;
 

Quoted 12 lines

kernel/sched/cpufreq_schedutil.c static unsigned int get_next_freq(struct sugov_policy *sg_policy, · line 172

Quoted 8 lines

kernel/sched/cpufreq_schedutil.c +6 -2

-       freq = get_capacity_ref_freq(policy);
-       freq = map_util_freq(util, freq, max);
+       if (util != max) {
+               freq = get_capacity_ref_freq(policy);
+               freq = map_util_freq(util, freq, max);
+       } else {
+               freq = policy->cpuinfo.max_freq;
+       }

This is not correct because you will have to wait to reach full utilization at the current OPP possibly the lowest OPP before moving directly to max OPP

Isn't this already the case? The ratio (util+headroom/max) will be less than 1 until util is 80% (with 25% headroom). And for all values <= 80% * max, we will request a frequency smaller than/equal policy->cur, no?

ie:

        util = 600
        max = 1024

        freq = 1.25 * 600 * policy->cur / 1024 = 0.73 * policy->cur

(util+headroom/max) must be greater than 1 for us to start going above policy->cur - which seems to have been working by accident IIUC.

So yes my proposal is incorrect, but it seems the conversion is not right to me now.

I could reproduce the problem now (thanks Wyes!). I have 3 freqs on my system

2.2GHz, 2.8GHz and 3.8GHz

which (I believe) translates into capacities

~592, ~754, 1024

which means we should pick 2.8GHz as soon as util * 1.25 > 592; which translates into util = ~473.

But what I see is that we go to 2.8GHz when we jump from 650 to 680 (see attached picture), which is what you'd expect since we apply two headrooms now, which means the ratio (util+headroom/max) will be greater than 1 after go above this value

        1024 * 0.8 * 0.8 = ~655

So I think the math makes sense logically, but we're missing some other correction factor.

When I re-enable CPPC I see for the same test that we go into 3.8GHz straight away. My test is simple busyloop via

cat /dev/zero > /dev/null

I see the CPU util_avg is at 523 at fork. I expected us to run to 2.8GHz here to be honest, but I am not sure if util_cfs_boost() and util_est() are maybe causing us to be slightly above 523 and that's why we start with max freq.

Or I've done the math wrong :-) But the two don't behave the same for the same kernel with and without CPPC.

Quoted 5 lines

Quoted 5 lines

kernel/sched/cpufreq_schedutil.c +0 -0

        if (freq == sg_policy->cached_raw_freq && !sg_policy->need_freq_update)
                return sg_policy->next_freq;

Quoted 2 lines

Or I've done the math wrong :-) But the two don't behave the same for the same kernel with and without CPPC.

They will never behave the same because they can't
- with invariance, the utilization is the utilization at max capacity so we can easily jump several OPP to go directly to the right one
- without invariance, the utilization is the utilization at current OPP so we can only jump to a limited number of OPP

Quoted 8 lines · Vincent Guittot

Quoted 2 lines

Or I've done the math wrong :-) But the two don't behave the same for the same kernel with and without CPPC.

They will never behave the same because they can't
- with invariance, the utilization is the utilization at max capacity so we can easily jump several OPP to go directly to the right one
- without invariance, the utilization is the utilization at current OPP so we can only jump to a limited number of OPP

I am probably missing some subtlty, but the behavior looks more sensible to me when we divide by current capacity instead of max one.

It seems what you're saying is that the capacity range for each OPP is 0-1024.

And that's when we know that we saturated the current capacity level we decide to move on.

As I am trying to remove the hardcoded headroom values I am wary of another one. But it seems this is bandaid scenario anyway; so maybe I shouldn't worry too much about it.

Cheers

--
Qais Yousef

Quoted 15 lines · Qais Yousef

Quoted 8 lines · Vincent Guittot

Quoted 2 lines

Or I've done the math wrong :-) But the two don't behave the same for the same kernel with and without CPPC.

They will never behave the same because they can't
- with invariance, the utilization is the utilization at max capacity so we can easily jump several OPP to go directly to the right one
- without invariance, the utilization is the utilization at current OPP so we can only jump to a limited number of OPP

I am probably missing some subtlty, but the behavior looks more sensible to me when we divide by current capacity instead of max one.

Quoted 1 line

It seems what you're saying is that the capacity range for each OPP is 0-1024.

Yes that's the case when you don't have frequency invariance

Quoted 2 lines

And that's when we know that we saturated the current capacity level we decide to move on.

yes

Quoted 4 lines

As I am trying to remove the hardcoded headroom values I am wary of another one. But it seems this is bandaid scenario anyway; so maybe I shouldn't worry too much about it.

Quoted 16 lines · Vincent Guittot

Quoted 15 lines · Qais Yousef

Quoted 8 lines · Vincent Guittot

Quoted 2 lines

Or I've done the math wrong :-) But the two don't behave the same for the same kernel with and without CPPC.

They will never behave the same because they can't
- with invariance, the utilization is the utilization at max capacity so we can easily jump several OPP to go directly to the right one
- without invariance, the utilization is the utilization at current OPP so we can only jump to a limited number of OPP

I am probably missing some subtlty, but the behavior looks more sensible to me when we divide by current capacity instead of max one.

Quoted 13 lines

Quoted 1 line

It seems what you're saying is that the capacity range for each OPP is 0-1024.

Yes that's the case when you don't have frequency invariance

Quoted 2 lines

And that's when we know that we saturated the current capacity level we decide to move on.

yes

Quoted 4 lines

As I am trying to remove the hardcoded headroom values I am wary of another one. But it seems this is bandaid scenario anyway; so maybe I shouldn't worry too much about it.

I still don't fully understand this fix.

We had:

sugov_update_single_freq()

sugov_update_single_common()

next_f = get_next_freq()

freq = arch_scale_freq_invariant() ?

          policy->cpuinfo.max_freq : policy->cur (**) <- (2) !freq_inv

util = map_util_perf(util); <- (1) util *= 1.25

freq = map_util_freq(util, freq, max); <- (3)
}

And now there is:

sugov_update_single_freq()

sugov_update_single_common()

    sugov_get_util()

      sg_cpu->util = sugov_effective_cpu_perf()

        /* Add dvfs headroom to actual utilization */
        actual = map_util_perf(actual)            <- (1) util *= 1.25

next_f = get_next_freq()

freq = get_capacity_ref_freq()

return policy->cur (*) <- (2) !freq_inv

freq = map_util_freq(util, freq, max) <- (3)

Still not clear to me why we need this extra 'policy->cur *= 1.25' here (*) and not here (**)

Quoted 5 lines · Dietmar Eggemann

Quoted 3 lines · Vincent Guittot

Quoted 2 lines · Qais Yousef

On 01/15/24 09:21, Vincent Guittot wrote:

Now, util can't be higher than max to handle clamping use cases whereas it could be the case before. The jump to next OPP was previously done with util being higher than max and it's now done with freq being higher than policy->cur

My test is simple busyloop via

cat /dev/zero > /dev/null

Quoted 46 lines · Wyes Karny

Quoted 44 lines · Vincent Guittot

Hi Wyes,

Quoted 41 lines · Wyes Karny

Quoted 22 lines · Linus Torvalds

Quoted 8 lines · Linus Torvalds

It's one of these two:

f12560779f9d sched/cpufreq: Rework iowait boost
9c0b4bb7f630 sched/cpufreq: Rework schedutil governor performance estimation

one more boot to go, then I'll try to revert whichever causes my machine to perform horribly much worse.

I guess it should come as no surprise that the result is

9c0b4bb7f6303c9c4e2e34984c46f5a86478f84d is the first bad commit

but to revert cleanly I will have to revert all of

b3edde44e5d4 ("cpufreq/schedutil: Use a fixed reference frequency") f12560779f9d ("sched/cpufreq: Rework iowait boost") 9c0b4bb7f630 ("sched/cpufreq: Rework schedutil governor
performance estimation")

Quoted 5 lines

This is on a 32-core (64-thread) AMD Ryzen Threadripper 3970X, fwiw.

I'll keep that revert in my private test-tree for now (so that I have a working machine again), but I'll move it to my main branch soon unless somebody has a quick fix for this problem.

Hi Linus,

I'm able to reproduce this issue with my AMD Ryzen 5600G system. But only if I disable CPPC in BIOS and boot with acpi-cpufreq + schedutil. (I believe for your case also CPPC is diabled as log "_CPC object is not present" came). Enabling CPPC in BIOS issue not seen in my system. For AMD acpi-cpufreq also uses _CPC object to determine the boost ratio. When CPPC is disabled in BIOS something is going wrong and max capacity is becoming zero.

Hi Vincent, Qais,

...

Quoted 45 lines

Quoted 5 lines

There is something strange that I don't understand

Could you trace on the return of sugov_get_util()
the value of sg_cpu->util ?

Yeah, correct something was wrong in the bpftrace readings, max_cap is not zero in traces.

             git-5511    [001] d.h1.   427.159763: get_next_freq.constprop.0: [DEBUG] : freq 1400000, util 1024, max 1024
             git-5511    [001] d.h1.   427.163733: sugov_get_util: [DEBUG] : util 1024, sg_cpu->util 1024
             git-5511    [001] d.h1.   427.163735: get_next_freq.constprop.0: [DEBUG] : freq 1400000, util 1024, max 1024
             git-5511    [001] d.h1.   427.167706: sugov_get_util: [DEBUG] : util 1024, sg_cpu->util 1024
             git-5511    [001] d.h1.   427.167708: get_next_freq.constprop.0: [DEBUG] : freq 1400000, util 1024, max 1024
             git-5511    [001] d.h1.   427.171678: sugov_get_util: [DEBUG] : util 1024, sg_cpu->util 1024
             git-5511    [001] d.h1.   427.171679: get_next_freq.constprop.0: [DEBUG] : freq 1400000, util 1024, max 1024
             git-5511    [001] d.h1.   427.175653: sugov_get_util: [DEBUG] : util 1024, sg_cpu->util 1024
             git-5511    [001] d.h1.   427.175655: get_next_freq.constprop.0: [DEBUG] : freq 1400000, util 1024, max 1024
             git-5511    [001] d.s1.   427.175665: sugov_get_util: [DEBUG] : util 1024, sg_cpu->util 1024
             git-5511    [001] d.s1.   427.175665: get_next_freq.constprop.0: [DEBUG] : freq 1400000, util 1024, max 1024

Debug patch applied:

kernel/sched/cpufreq_schedutil.c +2 -0

index 95c3c097083e..5c9b3e1de7a0 100644
@@ -166,6 +166,7 @@ static unsigned int get_next_freq(struct sugov_policy *sg_policy,
 
        freq = get_capacity_ref_freq(policy);
        freq = map_util_freq(util, freq, max);
+       trace_printk("[DEBUG] : freq %llu, util %llu, max %llu\n", freq, util, max);
 
        if (freq == sg_policy->cached_raw_freq && !sg_policy->need_freq_update)
                return sg_policy->next_freq;

@@ -199,6 +200,7 @@ static void sugov_get_util(struct sugov_cpu *sg_cpu, unsigned long boost)
        util = max(util, boost);
        sg_cpu->bw_min = min;
        sg_cpu->util = sugov_effective_cpu_perf(sg_cpu->cpu, util, min, max);
+       trace_printk("[DEBUG] : util %llu, sg_cpu->util %llu\n", util, sg_cpu->util);
 }
 
 /**

Quoted 1 line

So, I guess map_util_freq going wrong somewhere.

Thanks for the trace. It was really helpful and I think that I got the root cause.

The problem comes from get_capacity_ref_freq() which returns current freq when arch_scale_freq_invariant() is not enable, and the fact that we apply map_util_perf() earlier in the path now which is then capped by max capacity.

Could you try the below ?

kernel/sched/cpufreq_schedutil.c +1 -1

index e420e2ee1a10..611c621543f4 100644
@@ -133,7 +133,7 @@ unsigned long get_capacity_ref_freq(struct
 cpufreq_policy *policy)
        if (arch_scale_freq_invariant())
                return policy->cpuinfo.max_freq;
 
-       return policy->cur;
+       return policy->cur + policy->cur >> 2;
 }
 

/**

Quoted 116 lines · Vincent Guittot

Quoted 46 lines · Wyes Karny

Quoted 44 lines · Vincent Guittot

Hi Wyes,

Quoted 41 lines · Wyes Karny

Quoted 22 lines · Linus Torvalds

Quoted 8 lines · Linus Torvalds

It's one of these two:

f12560779f9d sched/cpufreq: Rework iowait boost
9c0b4bb7f630 sched/cpufreq: Rework schedutil governor performance estimation

one more boot to go, then I'll try to revert whichever causes my machine to perform horribly much worse.

I guess it should come as no surprise that the result is

9c0b4bb7f6303c9c4e2e34984c46f5a86478f84d is the first bad commit

but to revert cleanly I will have to revert all of

b3edde44e5d4 ("cpufreq/schedutil: Use a fixed reference frequency") f12560779f9d ("sched/cpufreq: Rework iowait boost") 9c0b4bb7f630 ("sched/cpufreq: Rework schedutil governor
performance estimation")

Quoted 5 lines

This is on a 32-core (64-thread) AMD Ryzen Threadripper 3970X, fwiw.

I'll keep that revert in my private test-tree for now (so that I have a working machine again), but I'll move it to my main branch soon unless somebody has a quick fix for this problem.

Hi Linus,

I'm able to reproduce this issue with my AMD Ryzen 5600G system. But only if I disable CPPC in BIOS and boot with acpi-cpufreq + schedutil. (I believe for your case also CPPC is diabled as log "_CPC object is not present" came). Enabling CPPC in BIOS issue not seen in my system. For AMD acpi-cpufreq also uses _CPC object to determine the boost ratio. When CPPC is disabled in BIOS something is going wrong and max capacity is becoming zero.

Hi Vincent, Qais,

...

Quoted 45 lines

Quoted 5 lines

There is something strange that I don't understand

Could you trace on the return of sugov_get_util()
the value of sg_cpu->util ?

Yeah, correct something was wrong in the bpftrace readings, max_cap is not zero in traces.

             git-5511    [001] d.h1.   427.159763: get_next_freq.constprop.0: [DEBUG] : freq 1400000, util 1024, max 1024
             git-5511    [001] d.h1.   427.163733: sugov_get_util: [DEBUG] : util 1024, sg_cpu->util 1024
             git-5511    [001] d.h1.   427.163735: get_next_freq.constprop.0: [DEBUG] : freq 1400000, util 1024, max 1024
             git-5511    [001] d.h1.   427.167706: sugov_get_util: [DEBUG] : util 1024, sg_cpu->util 1024
             git-5511    [001] d.h1.   427.167708: get_next_freq.constprop.0: [DEBUG] : freq 1400000, util 1024, max 1024
             git-5511    [001] d.h1.   427.171678: sugov_get_util: [DEBUG] : util 1024, sg_cpu->util 1024
             git-5511    [001] d.h1.   427.171679: get_next_freq.constprop.0: [DEBUG] : freq 1400000, util 1024, max 1024
             git-5511    [001] d.h1.   427.175653: sugov_get_util: [DEBUG] : util 1024, sg_cpu->util 1024
             git-5511    [001] d.h1.   427.175655: get_next_freq.constprop.0: [DEBUG] : freq 1400000, util 1024, max 1024
             git-5511    [001] d.s1.   427.175665: sugov_get_util: [DEBUG] : util 1024, sg_cpu->util 1024
             git-5511    [001] d.s1.   427.175665: get_next_freq.constprop.0: [DEBUG] : freq 1400000, util 1024, max 1024

Debug patch applied:

kernel/sched/cpufreq_schedutil.c +2 -0

index 95c3c097083e..5c9b3e1de7a0 100644
@@ -166,6 +166,7 @@ static unsigned int get_next_freq(struct sugov_policy *sg_policy,
 
        freq = get_capacity_ref_freq(policy);
        freq = map_util_freq(util, freq, max);
+       trace_printk("[DEBUG] : freq %llu, util %llu, max %llu\n", freq, util, max);
 
        if (freq == sg_policy->cached_raw_freq && !sg_policy->need_freq_update)
                return sg_policy->next_freq;

@@ -199,6 +200,7 @@ static void sugov_get_util(struct sugov_cpu *sg_cpu, unsigned long boost)
        util = max(util, boost);
        sg_cpu->bw_min = min;
        sg_cpu->util = sugov_effective_cpu_perf(sg_cpu->cpu, util, min, max);
+       trace_printk("[DEBUG] : util %llu, sg_cpu->util %llu\n", util, sg_cpu->util);
 }
 
 /**

Quoted 1 line

So, I guess map_util_freq going wrong somewhere.

Thanks for the trace. It was really helpful and I think that I got the root cause.

The problem comes from get_capacity_ref_freq() which returns current freq when arch_scale_freq_invariant() is not enable, and the fact that we apply map_util_perf() earlier in the path now which is then capped by max capacity.

Could you try the below ?

kernel/sched/cpufreq_schedutil.c +0 -0

index e420e2ee1a10..611c621543f4 100644
@@ -133,7 +133,7 @@ unsigned long get_capacity_ref_freq(struct
 cpufreq_policy *policy)
        if (arch_scale_freq_invariant())
                return policy->cpuinfo.max_freq;
 

Quoted 5 lines

kernel/sched/cpufreq_schedutil.c +1 -1

-       return policy->cur;
+       return policy->cur + policy->cur >> 2;
 }
 

/**

Issue seems to be fixed with this (but bit modified by me for arithmetic precedence):

(quoted diff) +0 -0

 }
 
 /**

trace:

            make-7912    [001] d..2.   182.070005: sugov_get_util: [DEBUG] : util 595, sg_cpu->util 743
            make-7912    [001] d..2.   182.070006: get_next_freq.constprop.0: [DEBUG] : freq 3537231, util 743, max 1024
              sh-7956    [001] d..2.   182.070494: sugov_get_util: [DEBUG] : util 835, sg_cpu->util 1024
              sh-7956    [001] d..2.   182.070495: get_next_freq.constprop.0: [DEBUG] : freq 4875000, util 1024, max 1024
              sh-7956    [001] d..2.   182.070576: sugov_get_util: [DEBUG] : util 955, sg_cpu->util 1024
              sh-7956    [001] d..2.   182.070576: get_next_freq.constprop.0: [DEBUG] : freq 4875000, util 1024, max 1024
              sh-7957    [001] d.h1.   182.072120: sugov_get_util: [DEBUG] : util 990, sg_cpu->util 1024
              sh-7957    [001] d.h1.   182.072121: get_next_freq.constprop.0: [DEBUG] : freq 4875000, util 1024, max 1024
              nm-7957    [001] dNh1.   182.076088: sugov_get_util: [DEBUG] : util 1024, sg_cpu->util 1024
              nm-7957    [001] dNh1.   182.076089: get_next_freq.constprop.0: [DEBUG] : freq 4875000, util 1024, max 1024
            grep-7958    [001] d..2.   182.076833: sugov_get_util: [DEBUG] : util 1024, sg_cpu->util 1024

Thanks,
Wyes

Quoted 4 lines · Ingo Molnar

Please pull the latest sched/core git tree from:

   git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git sched-core-2024-01-08

Just a note that I'm currently bisecting into this merge for a horrendous performance regression.

It makes my empty kernel build go from 22 seconds to 44 seconds, and makes a full kernel build enormously slower too.

I haven't finished the bisection, but it's now inside *just* this pull, so I can already tell that I'm going to revert something in here, because this has been making my merge window miserable.