cpufreq: schedutil: Don't ignore limit changes when util is unchanged

From: Sultan Alsawaf <[email protected]>

When utilization is unchanged, a policy limits update is ignored unless CPUFREQ_NEED_UPDATE_LIMITS is set. This occurs because limits_changed depends on the old broken behavior of need_freq_update to trigger a call into cpufreq_driver_resolve_freq() to evaluate the changed policy limits.

After fixing need_freq_update, limit changes are ignored without CPUFREQ_NEED_UPDATE_LIMITS, at least until utilization changes enough to make map_util_freq() return something different.

Fix the ignored limit changes by preserving the value of limits_changed until get_next_freq() is called, so limits_changed can trigger a call to cpufreq_driver_resolve_freq().

Reported-and-tested-by Stephan Gerhold <[email protected]>

Link https://lore.kernel.org/lkml/[email protected] open here

Fixes 8e461a1cb43d6 ("cpufreq: schedutil: Fix superfluous updates caused by need_freq_update")

Signed-off-by Sultan Alsawaf <[email protected]>

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

kernel/sched/cpufreq_schedutil.c +0 -1

index 1a19d69b91ed3..f37b999854d52 100644
@@ -82,7 +82,6 @@ static bool sugov_should_update_freq(struct sugov_policy *sg_policy, u64 time)
                return false;
 
        if (unlikely(sg_policy->limits_changed)) {
-               sg_policy->limits_changed = false;
                sg_policy->need_freq_update = cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS);
                return true;
        }

kernel/sched/cpufreq_schedutil.c +1 -1

                return sg_policy->next_freq;
 
+       sg_policy->limits_changed = false;
        sg_policy->cached_raw_freq = freq;
        return cpufreq_driver_resolve_freq(policy, freq);
 }
--

2.49.0

From: Sultan Alsawaf <[email protected]>

When utilization is unchanged, a policy limits update is ignored unless CPUFREQ_NEED_UPDATE_LIMITS is set. This occurs because limits_changed depends on the old broken behavior of need_freq_update to trigger a call into cpufreq_driver_resolve_freq() to evaluate the changed policy limits.

After fixing need_freq_update, limit changes are ignored without CPUFREQ_NEED_UPDATE_LIMITS, at least until utilization changes enough to make map_util_freq() return something different.

Fix the ignored limit changes by preserving the value of limits_changed until get_next_freq() is called, so limits_changed can trigger a call to cpufreq_driver_resolve_freq().

Reported-and-tested-by Stephan Gerhold <[email protected]>

Link https://lore.kernel.org/lkml/[email protected] open here

Fixes 8e461a1cb43d6 ("cpufreq: schedutil: Fix superfluous updates caused by need_freq_update")

Signed-off-by Sultan Alsawaf <[email protected]>

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

kernel/sched/cpufreq_schedutil.c +0 -1

index 1a19d69b91ed3..f37b999854d52 100644
@@ -82,7 +82,6 @@ static bool sugov_should_update_freq(struct sugov_policy *sg_policy, u64 time)
                return false;
 
        if (unlikely(sg_policy->limits_changed)) {
-               sg_policy->limits_changed = false;
                sg_policy->need_freq_update = cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS);
                return true;
        }

kernel/sched/cpufreq_schedutil.c +0 -1

        sg_policy->cached_raw_freq = freq;
        return cpufreq_driver_resolve_freq(policy, freq);
 }
--

2.49.0

Quoted 45 lines · Sultan Alsawaf

From: Sultan Alsawaf <[email protected]>

When utilization is unchanged, a policy limits update is ignored unless CPUFREQ_NEED_UPDATE_LIMITS is set. This occurs because limits_changed depends on the old broken behavior of need_freq_update to trigger a call into cpufreq_driver_resolve_freq() to evaluate the changed policy limits.

After fixing need_freq_update, limit changes are ignored without CPUFREQ_NEED_UPDATE_LIMITS, at least until utilization changes enough to make map_util_freq() return something different.

Fix the ignored limit changes by preserving the value of limits_changed until get_next_freq() is called, so limits_changed can trigger a call to cpufreq_driver_resolve_freq().

Reported-and-tested-by Stephan Gerhold <[email protected]>

Link https://lore.kernel.org/lkml/[email protected] open here

Fixes 8e461a1cb43d6 ("cpufreq: schedutil: Fix superfluous updates caused by need_freq_update")

Signed-off-by Sultan Alsawaf <[email protected]>

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

kernel/sched/cpufreq_schedutil.c +2 -2

index 1a19d69b91ed3..f37b999854d52 100644
@@ -82,7 +82,6 @@ static bool sugov_should_update_freq(struct sugov_policy *sg_policy, u64 time)
                return false;
 
        if (unlikely(sg_policy->limits_changed)) {
-               sg_policy->limits_changed = false;
                sg_policy->need_freq_update = cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS);
                return true;
        }

@@ -171,9 +170,11 @@ static unsigned int get_next_freq(struct sugov_policy *sg_policy,
        freq = get_capacity_ref_freq(policy);
        freq = map_util_freq(util, freq, max);
 
-       if (freq == sg_policy->cached_raw_freq && !sg_policy->need_freq_update)
+       if (freq == sg_policy->cached_raw_freq && !sg_policy->limits_changed &&
+           !sg_policy->need_freq_update)
                return sg_policy->next_freq;
 

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

Quoted 1 line

kernel/sched/cpufreq_schedutil.c +1 -0

+       sg_policy->limits_changed = false;

AFAICS, after this code modification, a limit change may be missed due to a possible race with sugov_limits() which cannot happen if sg_policy->limits_changed is only cleared when it is set before updating sg_policy->need_freq_update.

Quoted 46 lines · Rafael J. Wysocki

Quoted 45 lines · Sultan Alsawaf

From: Sultan Alsawaf <[email protected]>

When utilization is unchanged, a policy limits update is ignored unless CPUFREQ_NEED_UPDATE_LIMITS is set. This occurs because limits_changed depends on the old broken behavior of need_freq_update to trigger a call into cpufreq_driver_resolve_freq() to evaluate the changed policy limits.

After fixing need_freq_update, limit changes are ignored without CPUFREQ_NEED_UPDATE_LIMITS, at least until utilization changes enough to make map_util_freq() return something different.

Fix the ignored limit changes by preserving the value of limits_changed until get_next_freq() is called, so limits_changed can trigger a call to cpufreq_driver_resolve_freq().

Reported-and-tested-by Stephan Gerhold <[email protected]>

Link https://lore.kernel.org/lkml/[email protected] open here

Fixes 8e461a1cb43d6 ("cpufreq: schedutil: Fix superfluous updates caused by need_freq_update")

Signed-off-by Sultan Alsawaf <[email protected]>

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

kernel/sched/cpufreq_schedutil.c +2 -2

index 1a19d69b91ed3..f37b999854d52 100644
@@ -82,7 +82,6 @@ static bool sugov_should_update_freq(struct sugov_policy *sg_policy, u64 time)
                return false;
 
        if (unlikely(sg_policy->limits_changed)) {
-               sg_policy->limits_changed = false;
                sg_policy->need_freq_update = cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS);
                return true;
        }

@@ -171,9 +170,11 @@ static unsigned int get_next_freq(struct sugov_policy *sg_policy,
        freq = get_capacity_ref_freq(policy);
        freq = map_util_freq(util, freq, max);
 
-       if (freq == sg_policy->cached_raw_freq && !sg_policy->need_freq_update)
+       if (freq == sg_policy->cached_raw_freq && !sg_policy->limits_changed &&
+           !sg_policy->need_freq_update)
                return sg_policy->next_freq;
 

Quoted 6 lines

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

Quoted 1 line

kernel/sched/cpufreq_schedutil.c +1 -0

+       sg_policy->limits_changed = false;

AFAICS, after this code modification, a limit change may be missed due to a possible race with sugov_limits() which cannot happen if sg_policy->limits_changed is only cleared when it is set before updating sg_policy->need_freq_update.

I don't think that's the case because sg_policy->limits_changed is cleared before the new policy limits are evaluated in cpufreq_driver_resolve_freq().

Granted, if we wanted to be really certain of this, we'd need release semantics.

Looking closer at cpufreq.c actually, isn't there already a race on the updated policy limits (policy->min and policy->max) since they can be updated again while schedutil reads them via cpufreq_driver_resolve_freq()?

Quoted 55 lines · Sultan Alsawaf

Quoted 46 lines · Rafael J. Wysocki

Quoted 45 lines · Sultan Alsawaf

From: Sultan Alsawaf <[email protected]>

When utilization is unchanged, a policy limits update is ignored unless CPUFREQ_NEED_UPDATE_LIMITS is set. This occurs because limits_changed depends on the old broken behavior of need_freq_update to trigger a call into cpufreq_driver_resolve_freq() to evaluate the changed policy limits.

After fixing need_freq_update, limit changes are ignored without CPUFREQ_NEED_UPDATE_LIMITS, at least until utilization changes enough to make map_util_freq() return something different.

Fix the ignored limit changes by preserving the value of limits_changed until get_next_freq() is called, so limits_changed can trigger a call to cpufreq_driver_resolve_freq().

Reported-and-tested-by Stephan Gerhold <[email protected]>

Link https://lore.kernel.org/lkml/[email protected] open here

Fixes 8e461a1cb43d6 ("cpufreq: schedutil: Fix superfluous updates caused by need_freq_update")

Signed-off-by Sultan Alsawaf <[email protected]>

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

kernel/sched/cpufreq_schedutil.c +2 -2

index 1a19d69b91ed3..f37b999854d52 100644
@@ -82,7 +82,6 @@ static bool sugov_should_update_freq(struct sugov_policy *sg_policy, u64 time)
                return false;
 
        if (unlikely(sg_policy->limits_changed)) {
-               sg_policy->limits_changed = false;
                sg_policy->need_freq_update = cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS);
                return true;
        }

@@ -171,9 +170,11 @@ static unsigned int get_next_freq(struct sugov_policy *sg_policy,
        freq = get_capacity_ref_freq(policy);
        freq = map_util_freq(util, freq, max);
 
-       if (freq == sg_policy->cached_raw_freq && !sg_policy->need_freq_update)
+       if (freq == sg_policy->cached_raw_freq && !sg_policy->limits_changed &&
+           !sg_policy->need_freq_update)
                return sg_policy->next_freq;
 

Quoted 6 lines

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

Quoted 1 line

kernel/sched/cpufreq_schedutil.c +1 -0

+       sg_policy->limits_changed = false;

AFAICS, after this code modification, a limit change may be missed due to a possible race with sugov_limits() which cannot happen if sg_policy->limits_changed is only cleared when it is set before updating sg_policy->need_freq_update.

Quoted 2 lines

I don't think that's the case because sg_policy->limits_changed is cleared before the new policy limits are evaluated in cpufreq_driver_resolve_freq().

sugov_limits() may be triggered by a scaling_max_freq update, for instance, so it is asynchronous with respect to the usual governor flow. It updates sg_policy->limits_changed and assumes that next time the governor runs, it will call into the driver, for example via cpufreq_driver_fast_switch(), so the new limits take effect. This is not about cpufreq_driver_resolve_freq().

sugov_limits() runs after the driver's ->verify() callback has returned and it is conditional on that callback's return value, so the driver already knows the new limits when sugov_limits() runs, but it may still need to tell the hardware what the new limits are and that's why cpufreq_driver_fast_switch() may need to run.

Now, if sugov_should_update_freq() sees sg_policy->limits_changed set, it will set sg_policy->need_freq_update which (for drivers with CPUFREQ_NEED_UPDATE_LIMITS set) guarantees that the driver will be invoked and so sg_policy->limits_changed can be cleared.

If a new instance of sugov_limits() runs at this point, there are two possibilities. Either it completes before the sg_policy->limits_changed update in sugov_should_update_freq(), in which case the driver already knows the new limits as per the above and so the subsequent invocation of cpufreq_driver_fast_switch() will pick them up, or it sets sg_policy->limits_changed again and the governor will see it set next time it runs. In both cases the new limits will be picked up unless they are changed again in the meantime.

After the above change, sg_policy->limits_changed may be cleared even if it has not been set before and that's problematic. Namely, say it is unset when sugov_should_update_freq() runs, after being called by sugov_update_single_freq() via sugov_update_single_common(), and returns 'true' without setting sg_policy->need_freq_update. Next, sugov_update_single_common() returns 'true' and get_next_freq() is called. It sees that freq != sg_policy->cached_raw_freq, so it clears sg_policy->limits_changed. If sugov_limits() runs on a different CPU between the check and the sg_policy->limits_changed update in get_next_freq(), it may be missed and it is still not guaranteed that cpufreq_driver_fast_switch() will run because sg_policy->need_freq_update is unset and sugov_hold_freq() may return 'true'.

For this to work, sg_policy->limits_changed needs to be cleared only when it is set and sg_policy->need_freq_update needs to be updated when sg_policy->limits_changed is cleared.

It looks like you really want to set sg_policy->need_freq_update to 'true' in sugov_should_update_freq() when sg_policy->limits_changed is set, but that would render CPUFREQ_NEED_UPDATE_LIMITS unnecessary.

Quoted 55 lines · Sultan Alsawaf

Quoted 46 lines · Rafael J. Wysocki

Quoted 45 lines · Sultan Alsawaf

From: Sultan Alsawaf <[email protected]>

When utilization is unchanged, a policy limits update is ignored unless CPUFREQ_NEED_UPDATE_LIMITS is set. This occurs because limits_changed depends on the old broken behavior of need_freq_update to trigger a call into cpufreq_driver_resolve_freq() to evaluate the changed policy limits.

After fixing need_freq_update, limit changes are ignored without CPUFREQ_NEED_UPDATE_LIMITS, at least until utilization changes enough to make map_util_freq() return something different.

Fix the ignored limit changes by preserving the value of limits_changed until get_next_freq() is called, so limits_changed can trigger a call to cpufreq_driver_resolve_freq().

Reported-and-tested-by Stephan Gerhold <[email protected]>

Link https://lore.kernel.org/lkml/[email protected] open here

Fixes 8e461a1cb43d6 ("cpufreq: schedutil: Fix superfluous updates caused by need_freq_update")

Signed-off-by Sultan Alsawaf <[email protected]>

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

kernel/sched/cpufreq_schedutil.c +2 -2

index 1a19d69b91ed3..f37b999854d52 100644
@@ -82,7 +82,6 @@ static bool sugov_should_update_freq(struct sugov_policy *sg_policy, u64 time)
                return false;
 
        if (unlikely(sg_policy->limits_changed)) {
-               sg_policy->limits_changed = false;
                sg_policy->need_freq_update = cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS);
                return true;
        }

@@ -171,9 +170,11 @@ static unsigned int get_next_freq(struct sugov_policy *sg_policy,
        freq = get_capacity_ref_freq(policy);
        freq = map_util_freq(util, freq, max);
 
-       if (freq == sg_policy->cached_raw_freq && !sg_policy->need_freq_update)
+       if (freq == sg_policy->cached_raw_freq && !sg_policy->limits_changed &&
+           !sg_policy->need_freq_update)
                return sg_policy->next_freq;
 

Quoted 6 lines

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

Quoted 1 line

kernel/sched/cpufreq_schedutil.c +1 -0

+       sg_policy->limits_changed = false;

AFAICS, after this code modification, a limit change may be missed due to a possible race with sugov_limits() which cannot happen if sg_policy->limits_changed is only cleared when it is set before updating sg_policy->need_freq_update.

Quoted 2 lines

I don't think that's the case because sg_policy->limits_changed is cleared before the new policy limits are evaluated in cpufreq_driver_resolve_freq().

sugov_limits() may be triggered by a scaling_max_freq update, for instance, so it is asynchronous with respect to the usual governor flow. It updates sg_policy->limits_changed and assumes that next time the governor runs, it will call into the driver, for example via cpufreq_driver_fast_switch(), so the new limits take effect. This is not about cpufreq_driver_resolve_freq().

sugov_limits() runs after the driver's ->verify() callback has returned and it is conditional on that callback's return value, so the driver already knows the new limits when sugov_limits() runs, but it may still need to tell the hardware what the new limits are and that's why cpufreq_driver_fast_switch() may need to run.

Now, if sugov_should_update_freq() sees sg_policy->limits_changed set, it will set sg_policy->need_freq_update which (for drivers with CPUFREQ_NEED_UPDATE_LIMITS set) guarantees that the driver will be invoked and so sg_policy->limits_changed can be cleared.

If a new instance of sugov_limits() runs at this point, there are two possibilities. Either it completes before the sg_policy->limits_changed update in sugov_should_update_freq(), in which case the driver already knows the new limits as per the above and so the subsequent invocation of cpufreq_driver_fast_switch() will pick them up, or it sets sg_policy->limits_changed again and the governor will see it set next time it runs. In both cases the new limits will be picked up unless they are changed again in the meantime.

After the above change, sg_policy->limits_changed may be cleared even if it has not been set before and that's problematic. Namely, say it is unset when sugov_should_update_freq() runs, after being called by sugov_update_single_freq() via sugov_update_single_common(), and returns 'true' without setting sg_policy->need_freq_update. Next, sugov_update_single_common() returns 'true' and get_next_freq() is called. It sees that freq != sg_policy->cached_raw_freq, so it clears sg_policy->limits_changed. If sugov_limits() runs on a different CPU between the check and the sg_policy->limits_changed update in get_next_freq(), it may be missed and it is still not guaranteed that cpufreq_driver_fast_switch() will run because sg_policy->need_freq_update is unset and sugov_hold_freq() may return 'true'.

For this to work, sg_policy->limits_changed needs to be cleared only when it is set and sg_policy->need_freq_update needs to be updated when sg_policy->limits_changed is cleared.

It looks like you really want to set sg_policy->need_freq_update to 'true' in sugov_should_update_freq() when sg_policy->limits_changed is set, but that would render CPUFREQ_NEED_UPDATE_LIMITS unnecessary.

Quoted 1 line

Granted, if we wanted to be really certain of this, we'd need release semantics.

I don't think so, but feel free to prove me wrong.

Quoted 59 lines · Rafael J. Wysocki

Quoted 55 lines · Sultan Alsawaf

Quoted 46 lines · Rafael J. Wysocki

Quoted 45 lines · Sultan Alsawaf

From: Sultan Alsawaf <[email protected]>

When utilization is unchanged, a policy limits update is ignored unless CPUFREQ_NEED_UPDATE_LIMITS is set. This occurs because limits_changed depends on the old broken behavior of need_freq_update to trigger a call into cpufreq_driver_resolve_freq() to evaluate the changed policy limits.

After fixing need_freq_update, limit changes are ignored without CPUFREQ_NEED_UPDATE_LIMITS, at least until utilization changes enough to make map_util_freq() return something different.

Fix the ignored limit changes by preserving the value of limits_changed until get_next_freq() is called, so limits_changed can trigger a call to cpufreq_driver_resolve_freq().

Reported-and-tested-by Stephan Gerhold <[email protected]>

Link https://lore.kernel.org/lkml/[email protected] open here

Fixes 8e461a1cb43d6 ("cpufreq: schedutil: Fix superfluous updates caused by need_freq_update")

Signed-off-by Sultan Alsawaf <[email protected]>

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

kernel/sched/cpufreq_schedutil.c +2 -2

index 1a19d69b91ed3..f37b999854d52 100644
@@ -82,7 +82,6 @@ static bool sugov_should_update_freq(struct sugov_policy *sg_policy, u64 time)
                return false;
 
        if (unlikely(sg_policy->limits_changed)) {
-               sg_policy->limits_changed = false;
                sg_policy->need_freq_update = cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS);
                return true;
        }

@@ -171,9 +170,11 @@ static unsigned int get_next_freq(struct sugov_policy *sg_policy,
        freq = get_capacity_ref_freq(policy);
        freq = map_util_freq(util, freq, max);
 
-       if (freq == sg_policy->cached_raw_freq && !sg_policy->need_freq_update)
+       if (freq == sg_policy->cached_raw_freq && !sg_policy->limits_changed &&
+           !sg_policy->need_freq_update)
                return sg_policy->next_freq;
 

Quoted 6 lines

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

Quoted 1 line

kernel/sched/cpufreq_schedutil.c +1 -0

+       sg_policy->limits_changed = false;

AFAICS, after this code modification, a limit change may be missed due to a possible race with sugov_limits() which cannot happen if sg_policy->limits_changed is only cleared when it is set before updating sg_policy->need_freq_update.

Quoted 2 lines

I don't think that's the case because sg_policy->limits_changed is cleared before the new policy limits are evaluated in cpufreq_driver_resolve_freq().

Quoted 6 lines

sugov_limits() may be triggered by a scaling_max_freq update, for instance, so it is asynchronous with respect to the usual governor flow. It updates sg_policy->limits_changed and assumes that next time the governor runs, it will call into the driver, for example via cpufreq_driver_fast_switch(), so the new limits take effect. This is not about cpufreq_driver_resolve_freq().

OK, though I think there's still a race in cpufreq_driver_resolve_freq().

Quoted 5 lines

sugov_limits() runs after the driver's ->verify() callback has returned and it is conditional on that callback's return value, so the driver already knows the new limits when sugov_limits() runs, but it may still need to tell the hardware what the new limits are and that's why cpufreq_driver_fast_switch() may need to run.

Which is why CPUFREQ_NEED_UPDATE_LIMITS exists, right.

Quoted 29 lines

Now, if sugov_should_update_freq() sees sg_policy->limits_changed set, it will set sg_policy->need_freq_update which (for drivers with CPUFREQ_NEED_UPDATE_LIMITS set) guarantees that the driver will be invoked and so sg_policy->limits_changed can be cleared.

If a new instance of sugov_limits() runs at this point, there are two possibilities. Either it completes before the sg_policy->limits_changed update in sugov_should_update_freq(), in which case the driver already knows the new limits as per the above and so the subsequent invocation of cpufreq_driver_fast_switch() will pick them up, or it sets sg_policy->limits_changed again and the governor will see it set next time it runs. In both cases the new limits will be picked up unless they are changed again in the meantime.

After the above change, sg_policy->limits_changed may be cleared even if it has not been set before and that's problematic. Namely, say it is unset when sugov_should_update_freq() runs, after being called by sugov_update_single_freq() via sugov_update_single_common(), and returns 'true' without setting sg_policy->need_freq_update. Next, sugov_update_single_common() returns 'true' and get_next_freq() is called. It sees that freq != sg_policy->cached_raw_freq, so it clears sg_policy->limits_changed. If sugov_limits() runs on a different CPU between the check and the sg_policy->limits_changed update in get_next_freq(), it may be missed and it is still not guaranteed that cpufreq_driver_fast_switch() will run because sg_policy->need_freq_update is unset and sugov_hold_freq() may return 'true'.

Quoted 3 lines

For this to work, sg_policy->limits_changed needs to be cleared only when it is set and sg_policy->need_freq_update needs to be updated when sg_policy->limits_changed is cleared.

Ah I see, thank you for the detailed explanation. So if I am understanding it
correctly: the problem with my patch is that CPUFREQ_NEED_UPDATE_LIMITS might not be honored after a limits change, because CPUFREQ_NEED_UPDATE_LIMITS is only honored when sg_policy->limits_changed is set. So there is the following race:

CPU-A CPU-B

  sugov_should_update_freq()                    // sg_policy->limits_changed == false, sg_policy->need_freq_update == false
                                sugov_limits()  // sg_policy->limits_changed == true,  sg_policy->need_freq_update == false
  get_next_freq()                               // sg_policy->limits_changed == false, sg_policy->need_freq_update == false

// cpufreq driver won't be invoked for the limits change if:
// next_f == sg_policy->next_freq || (sugov_hold_freq() == true && next_f < sg_policy->next_freq)

Does that look right?

Quoted 4 lines

It looks like you really want to set sg_policy->need_freq_update to 'true' in sugov_should_update_freq() when sg_policy->limits_changed is set, but that would render CPUFREQ_NEED_UPDATE_LIMITS unnecessary.

Quoted 3 lines

Quoted 1 line

Granted, if we wanted to be really certain of this, we'd need release semantics.

I don't think so, but feel free to prove me wrong.

Well, it appears that there really is synchronization missing between cpufreq_set_policy() and schedutil, since cpufreq_set_policy() changes the live policy->min and policy->max, and schedutil may observe either the old values in there or garbage values due to load/store tearing.

Quoted 117 lines · Sultan Alsawaf

Quoted 59 lines · Rafael J. Wysocki

Quoted 55 lines · Sultan Alsawaf

Quoted 46 lines · Rafael J. Wysocki

Quoted 45 lines · Sultan Alsawaf

From: Sultan Alsawaf <[email protected]>

When utilization is unchanged, a policy limits update is ignored unless CPUFREQ_NEED_UPDATE_LIMITS is set. This occurs because limits_changed depends on the old broken behavior of need_freq_update to trigger a call into cpufreq_driver_resolve_freq() to evaluate the changed policy limits.

After fixing need_freq_update, limit changes are ignored without CPUFREQ_NEED_UPDATE_LIMITS, at least until utilization changes enough to make map_util_freq() return something different.

Fix the ignored limit changes by preserving the value of limits_changed until get_next_freq() is called, so limits_changed can trigger a call to cpufreq_driver_resolve_freq().

Reported-and-tested-by Stephan Gerhold <[email protected]>

Link https://lore.kernel.org/lkml/[email protected] open here

Fixes 8e461a1cb43d6 ("cpufreq: schedutil: Fix superfluous updates caused by need_freq_update")

Signed-off-by Sultan Alsawaf <[email protected]>

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

kernel/sched/cpufreq_schedutil.c +2 -2

index 1a19d69b91ed3..f37b999854d52 100644
@@ -82,7 +82,6 @@ static bool sugov_should_update_freq(struct sugov_policy *sg_policy, u64 time)
                return false;
 
        if (unlikely(sg_policy->limits_changed)) {
-               sg_policy->limits_changed = false;
                sg_policy->need_freq_update = cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS);
                return true;
        }

@@ -171,9 +170,11 @@ static unsigned int get_next_freq(struct sugov_policy *sg_policy,
        freq = get_capacity_ref_freq(policy);
        freq = map_util_freq(util, freq, max);
 
-       if (freq == sg_policy->cached_raw_freq && !sg_policy->need_freq_update)
+       if (freq == sg_policy->cached_raw_freq && !sg_policy->limits_changed &&
+           !sg_policy->need_freq_update)
                return sg_policy->next_freq;
 

Quoted 6 lines

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

Quoted 1 line

kernel/sched/cpufreq_schedutil.c +1 -0

+       sg_policy->limits_changed = false;

AFAICS, after this code modification, a limit change may be missed due to a possible race with sugov_limits() which cannot happen if sg_policy->limits_changed is only cleared when it is set before updating sg_policy->need_freq_update.

Quoted 2 lines

I don't think that's the case because sg_policy->limits_changed is cleared before the new policy limits are evaluated in cpufreq_driver_resolve_freq().

Quoted 6 lines

sugov_limits() may be triggered by a scaling_max_freq update, for instance, so it is asynchronous with respect to the usual governor flow. It updates sg_policy->limits_changed and assumes that next time the governor runs, it will call into the driver, for example via cpufreq_driver_fast_switch(), so the new limits take effect. This is not about cpufreq_driver_resolve_freq().

OK, though I think there's still a race in cpufreq_driver_resolve_freq().

Quoted 5 lines

sugov_limits() runs after the driver's ->verify() callback has returned and it is conditional on that callback's return value, so the driver already knows the new limits when sugov_limits() runs, but it may still need to tell the hardware what the new limits are and that's why cpufreq_driver_fast_switch() may need to run.

Which is why CPUFREQ_NEED_UPDATE_LIMITS exists, right.

Quoted 29 lines

Now, if sugov_should_update_freq() sees sg_policy->limits_changed set, it will set sg_policy->need_freq_update which (for drivers with CPUFREQ_NEED_UPDATE_LIMITS set) guarantees that the driver will be invoked and so sg_policy->limits_changed can be cleared.

If a new instance of sugov_limits() runs at this point, there are two possibilities. Either it completes before the sg_policy->limits_changed update in sugov_should_update_freq(), in which case the driver already knows the new limits as per the above and so the subsequent invocation of cpufreq_driver_fast_switch() will pick them up, or it sets sg_policy->limits_changed again and the governor will see it set next time it runs. In both cases the new limits will be picked up unless they are changed again in the meantime.

After the above change, sg_policy->limits_changed may be cleared even if it has not been set before and that's problematic. Namely, say it is unset when sugov_should_update_freq() runs, after being called by sugov_update_single_freq() via sugov_update_single_common(), and returns 'true' without setting sg_policy->need_freq_update. Next, sugov_update_single_common() returns 'true' and get_next_freq() is called. It sees that freq != sg_policy->cached_raw_freq, so it clears sg_policy->limits_changed. If sugov_limits() runs on a different CPU between the check and the sg_policy->limits_changed update in get_next_freq(), it may be missed and it is still not guaranteed that cpufreq_driver_fast_switch() will run because sg_policy->need_freq_update is unset and sugov_hold_freq() may return 'true'.

Quoted 3 lines

For this to work, sg_policy->limits_changed needs to be cleared only when it is set and sg_policy->need_freq_update needs to be updated when sg_policy->limits_changed is cleared.

Ah I see, thank you for the detailed explanation. So if I am understanding it
correctly: the problem with my patch is that CPUFREQ_NEED_UPDATE_LIMITS might not be honored after a limits change, because CPUFREQ_NEED_UPDATE_LIMITS is only honored when sg_policy->limits_changed is set. So there is the following race:

CPU-A CPU-B

Quoted 8 lines

  sugov_should_update_freq()                    // sg_policy->limits_changed == false, sg_policy->need_freq_update == false
                                sugov_limits()  // sg_policy->limits_changed == true,  sg_policy->need_freq_update == false
  get_next_freq()                               // sg_policy->limits_changed == false, sg_policy->need_freq_update == false

// cpufreq driver won't be invoked for the limits change if:
// next_f == sg_policy->next_freq || (sugov_hold_freq() == true && next_f < sg_policy->next_freq)

Does that look right?

Yes, it does.

Quoted 4 lines

Quoted 4 lines

It looks like you really want to set sg_policy->need_freq_update to 'true' in sugov_should_update_freq() when sg_policy->limits_changed is set, but that would render CPUFREQ_NEED_UPDATE_LIMITS unnecessary.

Quoted 8 lines

Quoted 3 lines

Quoted 1 line

Granted, if we wanted to be really certain of this, we'd need release semantics.

I don't think so, but feel free to prove me wrong.

Well, it appears that there really is synchronization missing between cpufreq_set_policy() and schedutil, since cpufreq_set_policy() changes the live policy->min and policy->max, and schedutil may observe either the old values in there or garbage values due to load/store tearing.

schedutil itself doesn't really read policy->min and policy->max. cpufreq_driver_resolve_freq() does this and drivers do, but since policy->min and policy->max are integers, no garbage values will be observed AFAICS.

AFAICS, after this code modification, a limit change may be missed due to a possible race with sugov_limits() which cannot happen if sg_policy->limits_changed is only cleared when it is set before updating sg_policy->need_freq_update.

...

Quoted 6 lines

AFAICS, after this code modification, a limit change may be missed due to a possible race with sugov_limits() which cannot happen if sg_policy->limits_changed is only cleared when it is set before updating sg_policy->need_freq_update.

could the following patch prevent the race?

https://lore.kernel.org/all/CAB8ipk_Ayqmh=Ch2aH2c+i-q+qdiQ317VBH1kOHYN=R9dt6LOw@mail.gmail.com/