diff mbox series

[1/2] cpufreq: schedutil: Fix superfluous updates caused by need_freq_update

Message ID 20241212015734.41241-2-sultan@kerneltoast.com (mailing list archive)
State Queued
Delegated to: Rafael Wysocki
Headers show
Series [1/2] cpufreq: schedutil: Fix superfluous updates caused by need_freq_update | expand

Commit Message

Sultan Alsawaf (unemployed) Dec. 12, 2024, 1:57 a.m. UTC 
From: "Sultan Alsawaf (unemployed)" <sultan@kerneltoast.com>

A redundant frequency update is only truly needed when there is a policy
limits change with a driver that specifies CPUFREQ_NEED_UPDATE_LIMITS.

In spite of that, drivers specifying CPUFREQ_NEED_UPDATE_LIMITS receive a
frequency update _all the time_, not just for a policy limits change,
because need_freq_update is never cleared.

Furthermore, ignore_dl_rate_limit()'s usage of need_freq_update also leads
to a redundant frequency update, regardless of whether or not the driver
specifies CPUFREQ_NEED_UPDATE_LIMITS, when the next chosen frequency is the
same as the current one.

Fix the superfluous updates by only honoring CPUFREQ_NEED_UPDATE_LIMITS
when there's a policy limits change, and clearing need_freq_update when a
requisite redundant update occurs.

This is neatly achieved by moving up the CPUFREQ_NEED_UPDATE_LIMITS test
and instead setting need_freq_update to false in sugov_update_next_freq().

Signed-off-by: Sultan Alsawaf (unemployed) <sultan@kerneltoast.com>
---
 kernel/sched/cpufreq_schedutil.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Christian Loehle Dec. 12, 2024, 1:24 p.m. UTC | #1 
On 12/12/24 01:57, Sultan Alsawaf wrote:
> From: "Sultan Alsawaf (unemployed)" <sultan@kerneltoast.com>
> 
> A redundant frequency update is only truly needed when there is a policy
> limits change with a driver that specifies CPUFREQ_NEED_UPDATE_LIMITS.
> 
> In spite of that, drivers specifying CPUFREQ_NEED_UPDATE_LIMITS receive a
> frequency update _all the time_, not just for a policy limits change,
> because need_freq_update is never cleared.
> 
> Furthermore, ignore_dl_rate_limit()'s usage of need_freq_update also leads
> to a redundant frequency update, regardless of whether or not the driver
> specifies CPUFREQ_NEED_UPDATE_LIMITS, when the next chosen frequency is the
> same as the current one.
> 
> Fix the superfluous updates by only honoring CPUFREQ_NEED_UPDATE_LIMITS
> when there's a policy limits change, and clearing need_freq_update when a
> requisite redundant update occurs.
> 
> This is neatly achieved by moving up the CPUFREQ_NEED_UPDATE_LIMITS test
> and instead setting need_freq_update to false in sugov_update_next_freq().
>

Good catch!
Fixes:
600f5badb78c ("cpufreq: schedutil: Don't skip freq update when limits change")


> Signed-off-by: Sultan Alsawaf (unemployed) <sultan@kerneltoast.com>

Reviewed-by: Christian Loehle <christian.loehle@arm.com>

> ---
>  kernel/sched/cpufreq_schedutil.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> index 28c77904ea74..e51d5ce730be 100644
> --- a/kernel/sched/cpufreq_schedutil.c
> +++ b/kernel/sched/cpufreq_schedutil.c
> @@ -83,7 +83,7 @@ static bool sugov_should_update_freq(struct sugov_policy *sg_policy, u64 time)
>  
>  	if (unlikely(sg_policy->limits_changed)) {
>  		sg_policy->limits_changed = false;
> -		sg_policy->need_freq_update = true;
> +		sg_policy->need_freq_update = cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS);>  		return true;
>  	}
>  
> @@ -96,7 +96,7 @@ static bool sugov_update_next_freq(struct sugov_policy *sg_policy, u64 time,
>  				   unsigned int next_freq)
>  {
>  	if (sg_policy->need_freq_update)
> -		sg_policy->need_freq_update = cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS);
> +		sg_policy->need_freq_update = false;
>  	else if (sg_policy->next_freq == next_freq)
>  		return false;

I guess you could rewrite this into just one if like

---

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

	sg_policy->need_freq_update = false
	sg_policy->next_freq = next_freq;
	sg_policy->last_freq_update_time = time;
Sultan Alsawaf (unemployed) Dec. 14, 2024, 2:35 a.m. UTC | #2 
On Thu, Dec 12, 2024 at 01:24:41PM +0000, Christian Loehle wrote:
> On 12/12/24 01:57, Sultan Alsawaf wrote:
> > From: "Sultan Alsawaf (unemployed)" <sultan@kerneltoast.com>
> > 
> > A redundant frequency update is only truly needed when there is a policy
> > limits change with a driver that specifies CPUFREQ_NEED_UPDATE_LIMITS.
> > 
> > In spite of that, drivers specifying CPUFREQ_NEED_UPDATE_LIMITS receive a
> > frequency update _all the time_, not just for a policy limits change,
> > because need_freq_update is never cleared.
> > 
> > Furthermore, ignore_dl_rate_limit()'s usage of need_freq_update also leads
> > to a redundant frequency update, regardless of whether or not the driver
> > specifies CPUFREQ_NEED_UPDATE_LIMITS, when the next chosen frequency is the
> > same as the current one.
> > 
> > Fix the superfluous updates by only honoring CPUFREQ_NEED_UPDATE_LIMITS
> > when there's a policy limits change, and clearing need_freq_update when a
> > requisite redundant update occurs.
> > 
> > This is neatly achieved by moving up the CPUFREQ_NEED_UPDATE_LIMITS test
> > and instead setting need_freq_update to false in sugov_update_next_freq().
> >
> 
> Good catch!
> Fixes:
> 600f5badb78c ("cpufreq: schedutil: Don't skip freq update when limits change")
> 
> 
> > Signed-off-by: Sultan Alsawaf (unemployed) <sultan@kerneltoast.com>
> 
> Reviewed-by: Christian Loehle <christian.loehle@arm.com>

Thanks for the review and digging up the bug provenance!

> > ---
> >  kernel/sched/cpufreq_schedutil.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> > index 28c77904ea74..e51d5ce730be 100644
> > --- a/kernel/sched/cpufreq_schedutil.c
> > +++ b/kernel/sched/cpufreq_schedutil.c
> > @@ -83,7 +83,7 @@ static bool sugov_should_update_freq(struct sugov_policy *sg_policy, u64 time)
> >  
> >  	if (unlikely(sg_policy->limits_changed)) {
> >  		sg_policy->limits_changed = false;
> > -		sg_policy->need_freq_update = true;
> > +		sg_policy->need_freq_update = cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS);>  		return true;
> >  	}
> >  
> > @@ -96,7 +96,7 @@ static bool sugov_update_next_freq(struct sugov_policy *sg_policy, u64 time,
> >  				   unsigned int next_freq)
> >  {
> >  	if (sg_policy->need_freq_update)
> > -		sg_policy->need_freq_update = cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS);
> > +		sg_policy->need_freq_update = false;
> >  	else if (sg_policy->next_freq == next_freq)
> >  		return false;
> 
> I guess you could rewrite this into just one if like
> 
> ---
> 
> 	if (sg_policy->next_freq == next_freq && !sg_policy->need_freq_update))
> 		return false;
> 
> 	sg_policy->need_freq_update = false
> 	sg_policy->next_freq = next_freq;
> 	sg_policy->last_freq_update_time = time;

Hmm, asm seems worse since it adds an extra store to one of the branch targets:

Before:
-------
        00100020 e3 03 00 aa     mov        x3,x0
        00100024 00 a8 43 39     ldrb       w0,[x0, #0xea]
        00100028 3f 23 03 d5     paciasp
        0010002c c0 00 00 36     tbz        w0,#0x0,LAB_00100044
        00100030 7f a8 03 39     strb       wzr,[x3, #0xea]
                             LAB_00100034
        00100034 20 00 80 52     mov        w0,#0x1
        00100038 61 14 00 f9     str        x1,[x3, #0x28]
        0010003c 62 38 00 b9     str        w2,[x3, #0x38]
        00100040 ff 0b 5f d6     retaa
                             LAB_00100044
        00100044 64 38 40 b9     ldr        w4,[x3, #0x38]
        00100048 9f 00 02 6b     cmp        w4,w2
        0010004c 41 ff ff 54     b.ne       LAB_00100034
        00100050 ff 0b 5f d6     retaa

After:
------
        00100020 e3 03 00 aa     mov        x3,x0
        00100024 00 38 40 b9     ldr        w0,[x0, #0x38]
        00100028 3f 23 03 d5     paciasp
        0010002c 1f 00 02 6b     cmp        w0,w2
        00100030 c0 00 00 54     b.eq       LAB_00100048
                             LAB_00100034
        00100034 20 00 80 52     mov        w0,#0x1
        00100038 61 14 00 f9     str        x1,[x3, #0x28]
        0010003c 62 38 00 b9     str        w2,[x3, #0x38]
        00100040 7f a8 03 39     strb       wzr,[x3, #0xea]
        00100044 ff 0b 5f d6     retaa
                             LAB_00100048
        00100048 60 a8 43 39     ldrb       w0,[x3, #0xea]
        0010004c 40 ff 07 37     tbnz       w0,#0x0,LAB_00100034
        00100050 ff 0b 5f d6     retaa

Sultan
Rafael J. Wysocki Dec. 18, 2024, 3:10 p.m. UTC | #3 
On Thu, Dec 12, 2024 at 2:24 PM Christian Loehle
<christian.loehle@arm.com> wrote:
>
> On 12/12/24 01:57, Sultan Alsawaf wrote:
> > From: "Sultan Alsawaf (unemployed)" <sultan@kerneltoast.com>
> >
> > A redundant frequency update is only truly needed when there is a policy
> > limits change with a driver that specifies CPUFREQ_NEED_UPDATE_LIMITS.
> >
> > In spite of that, drivers specifying CPUFREQ_NEED_UPDATE_LIMITS receive a
> > frequency update _all the time_, not just for a policy limits change,
> > because need_freq_update is never cleared.
> >
> > Furthermore, ignore_dl_rate_limit()'s usage of need_freq_update also leads
> > to a redundant frequency update, regardless of whether or not the driver
> > specifies CPUFREQ_NEED_UPDATE_LIMITS, when the next chosen frequency is the
> > same as the current one.
> >
> > Fix the superfluous updates by only honoring CPUFREQ_NEED_UPDATE_LIMITS
> > when there's a policy limits change, and clearing need_freq_update when a
> > requisite redundant update occurs.
> >
> > This is neatly achieved by moving up the CPUFREQ_NEED_UPDATE_LIMITS test
> > and instead setting need_freq_update to false in sugov_update_next_freq().
> >
>
> Good catch!
> Fixes:
> 600f5badb78c ("cpufreq: schedutil: Don't skip freq update when limits change")
>
>
> > Signed-off-by: Sultan Alsawaf (unemployed) <sultan@kerneltoast.com>
>
> Reviewed-by: Christian Loehle <christian.loehle@arm.com>

Applied with the above Fixes tag added as 6.14 material, thanks!
diff mbox series

Patch

diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
index 28c77904ea74..e51d5ce730be 100644
--- a/kernel/sched/cpufreq_schedutil.c
+++ b/kernel/sched/cpufreq_schedutil.c
@@ -83,7 +83,7 @@  static bool sugov_should_update_freq(struct sugov_policy *sg_policy, u64 time)
 
 	if (unlikely(sg_policy->limits_changed)) {
 		sg_policy->limits_changed = false;
-		sg_policy->need_freq_update = true;
+		sg_policy->need_freq_update = cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS);
 		return true;
 	}
 
@@ -96,7 +96,7 @@  static bool sugov_update_next_freq(struct sugov_policy *sg_policy, u64 time,
 				   unsigned int next_freq)
 {
 	if (sg_policy->need_freq_update)
-		sg_policy->need_freq_update = cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS);
+		sg_policy->need_freq_update = false;
 	else if (sg_policy->next_freq == next_freq)
 		return false;