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

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

[Sultan Alsawaf]

From: Sultan Alsawaf <sultan@kerneltoast.com>

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 <stephan.gerhold@linaro.org>
Link: https://lore.kernel.org/lkml/Z_Tlc6Qs-tYpxWYb@linaro.org
Fixes: 8e461a1cb43d6 ("cpufreq: schedutil: Fix superfluous updates caused by need_freq_update")
Signed-off-by: Sultan Alsawaf <sultan@kerneltoast.com>
---
 kernel/sched/cpufreq_schedutil.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
index 1a19d69b91ed3..f37b999854d52 100644
--- a/kernel/sched/cpufreq_schedutil.c
+++ b/kernel/sched/cpufreq_schedutil.c
@@ -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;
 
+	sg_policy->limits_changed = false;
 	sg_policy->cached_raw_freq = freq;
 	return cpufreq_driver_resolve_freq(policy, freq);
 }
-- 
2.49.0

Re: [PATCH] cpufreq: schedutil: Don't ignore limit changes when util is unchanged

[Xuewen Yan]

Hi Sultan,

On Thu, Apr 10, 2025 at 10:46 AM Sultan Alsawaf <sultan@kerneltoast.com> wrote:
>
> From: Sultan Alsawaf <sultan@kerneltoast.com>
>
> 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 <stephan.gerhold@linaro.org>
> Link: https://lore.kernel.org/lkml/Z_Tlc6Qs-tYpxWYb@linaro.org
> Fixes: 8e461a1cb43d6 ("cpufreq: schedutil: Fix superfluous updates caused by need_freq_update")
> Signed-off-by: Sultan Alsawaf <sultan@kerneltoast.com>
> ---
>  kernel/sched/cpufreq_schedutil.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> index 1a19d69b91ed3..f37b999854d52 100644
> --- a/kernel/sched/cpufreq_schedutil.c
> +++ b/kernel/sched/cpufreq_schedutil.c
> @@ -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)

As said in:https://lore.kernel.org/all/CAB8ipk8ARRdR8UPgLqJ3EcAzuE4KNEO=cmLNJLk6thTxdBSHWw@mail.gmail.com/

We also should add the limits_changed in the sugov_update_single_freq().

Thanks！

>                 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
>
>

Re: [PATCH] cpufreq: schedutil: Don't ignore limit changes when util is unchanged

[Rafael J. Wysocki]

On Thu, Apr 10, 2025 at 4:45 AM Sultan Alsawaf <sultan@kerneltoast.com> wrote:
>
> From: Sultan Alsawaf <sultan@kerneltoast.com>
>
> 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 <stephan.gerhold@linaro.org>
> Link: https://lore.kernel.org/lkml/Z_Tlc6Qs-tYpxWYb@linaro.org
> Fixes: 8e461a1cb43d6 ("cpufreq: schedutil: Fix superfluous updates caused by need_freq_update")
> Signed-off-by: Sultan Alsawaf <sultan@kerneltoast.com>
> ---
>  kernel/sched/cpufreq_schedutil.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> index 1a19d69b91ed3..f37b999854d52 100644
> --- a/kernel/sched/cpufreq_schedutil.c
> +++ b/kernel/sched/cpufreq_schedutil.c
> @@ -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;
>
> +       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.

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

Re: [PATCH] cpufreq: schedutil: Don't ignore limit changes when util is unchanged

[Sultan Alsawaf]

On Thu, Apr 10, 2025 at 05:34:39PM +0200, Rafael J. Wysocki wrote:
> On Thu, Apr 10, 2025 at 4:45 AM Sultan Alsawaf <sultan@kerneltoast.com> wrote:
> >
> > From: Sultan Alsawaf <sultan@kerneltoast.com>
> >
> > 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 <stephan.gerhold@linaro.org>
> > Link: https://lore.kernel.org/lkml/Z_Tlc6Qs-tYpxWYb@linaro.org
> > Fixes: 8e461a1cb43d6 ("cpufreq: schedutil: Fix superfluous updates caused by need_freq_update")
> > Signed-off-by: Sultan Alsawaf <sultan@kerneltoast.com>
> > ---
> >  kernel/sched/cpufreq_schedutil.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> > index 1a19d69b91ed3..f37b999854d52 100644
> > --- a/kernel/sched/cpufreq_schedutil.c
> > +++ b/kernel/sched/cpufreq_schedutil.c
> > @@ -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;
> >
> > +       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()?

Sultan

Re: [PATCH] cpufreq: schedutil: Don't ignore limit changes when util is unchanged

[Rafael J. Wysocki]

On Thu, Apr 10, 2025 at 6:03 PM Sultan Alsawaf <sultan@kerneltoast.com> wrote:
>
> On Thu, Apr 10, 2025 at 05:34:39PM +0200, Rafael J. Wysocki wrote:
> > On Thu, Apr 10, 2025 at 4:45 AM Sultan Alsawaf <sultan@kerneltoast.com> wrote:
> > >
> > > From: Sultan Alsawaf <sultan@kerneltoast.com>
> > >
> > > 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 <stephan.gerhold@linaro.org>
> > > Link: https://lore.kernel.org/lkml/Z_Tlc6Qs-tYpxWYb@linaro.org
> > > Fixes: 8e461a1cb43d6 ("cpufreq: schedutil: Fix superfluous updates caused by need_freq_update")
> > > Signed-off-by: Sultan Alsawaf <sultan@kerneltoast.com>
> > > ---
> > >  kernel/sched/cpufreq_schedutil.c | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> > > index 1a19d69b91ed3..f37b999854d52 100644
> > > --- a/kernel/sched/cpufreq_schedutil.c
> > > +++ b/kernel/sched/cpufreq_schedutil.c
> > > @@ -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;
> > >
> > > +       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().

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.

> 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.

> 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()?

That can happen, but it is a different thing.

Re: [PATCH] cpufreq: schedutil: Don't ignore limit changes when util is unchanged

[Rafael J. Wysocki]

On Thu, Apr 10, 2025 at 8:47 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Thu, Apr 10, 2025 at 6:03 PM Sultan Alsawaf <sultan@kerneltoast.com> wrote:
> >
> > On Thu, Apr 10, 2025 at 05:34:39PM +0200, Rafael J. Wysocki wrote:
> > > On Thu, Apr 10, 2025 at 4:45 AM Sultan Alsawaf <sultan@kerneltoast.com> wrote:
> > > >
> > > > From: Sultan Alsawaf <sultan@kerneltoast.com>
> > > >
> > > > 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 <stephan.gerhold@linaro.org>
> > > > Link: https://lore.kernel.org/lkml/Z_Tlc6Qs-tYpxWYb@linaro.org
> > > > Fixes: 8e461a1cb43d6 ("cpufreq: schedutil: Fix superfluous updates caused by need_freq_update")
> > > > Signed-off-by: Sultan Alsawaf <sultan@kerneltoast.com>
> > > > ---
> > > >  kernel/sched/cpufreq_schedutil.c | 5 +++--
> > > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> > > > index 1a19d69b91ed3..f37b999854d52 100644
> > > > --- a/kernel/sched/cpufreq_schedutil.c
> > > > +++ b/kernel/sched/cpufreq_schedutil.c
> > > > @@ -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;
> > > >
> > > > +       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().
>
> 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.

As I've just said in the original patch thread, it looks like using
sg_policy->policy->fast_switch_enabled instead of
cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS) in
sugov_should_update_freq() should do the trick and then (if this
works), CPUFREQ_NEED_UPDATE_LIMITS can be dropped.

Re: [PATCH] cpufreq: schedutil: Don't ignore limit changes when util is unchanged

[Sultan Alsawaf]

On Thu, Apr 10, 2025 at 08:47:38PM +0200, Rafael J. Wysocki wrote:
> On Thu, Apr 10, 2025 at 6:03 PM Sultan Alsawaf <sultan@kerneltoast.com> wrote:
> >
> > On Thu, Apr 10, 2025 at 05:34:39PM +0200, Rafael J. Wysocki wrote:
> > > On Thu, Apr 10, 2025 at 4:45 AM Sultan Alsawaf <sultan@kerneltoast.com> wrote:
> > > >
> > > > From: Sultan Alsawaf <sultan@kerneltoast.com>
> > > >
> > > > 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 <stephan.gerhold@linaro.org>
> > > > Link: https://lore.kernel.org/lkml/Z_Tlc6Qs-tYpxWYb@linaro.org
> > > > Fixes: 8e461a1cb43d6 ("cpufreq: schedutil: Fix superfluous updates caused by need_freq_update")
> > > > Signed-off-by: Sultan Alsawaf <sultan@kerneltoast.com>
> > > > ---
> > > >  kernel/sched/cpufreq_schedutil.c | 5 +++--
> > > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> > > > index 1a19d69b91ed3..f37b999854d52 100644
> > > > --- a/kernel/sched/cpufreq_schedutil.c
> > > > +++ b/kernel/sched/cpufreq_schedutil.c
> > > > @@ -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;
> > > >
> > > > +       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().
> 
> 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().

> 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.

> 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.

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?

> 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.
> 
> > 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.

Sultan

Re: [PATCH] cpufreq: schedutil: Don't ignore limit changes when util is unchanged

[Rafael J. Wysocki]

On Thu, Apr 10, 2025 at 9:26 PM Sultan Alsawaf <sultan@kerneltoast.com> wrote:
>
> On Thu, Apr 10, 2025 at 08:47:38PM +0200, Rafael J. Wysocki wrote:
> > On Thu, Apr 10, 2025 at 6:03 PM Sultan Alsawaf <sultan@kerneltoast.com> wrote:
> > >
> > > On Thu, Apr 10, 2025 at 05:34:39PM +0200, Rafael J. Wysocki wrote:
> > > > On Thu, Apr 10, 2025 at 4:45 AM Sultan Alsawaf <sultan@kerneltoast.com> wrote:
> > > > >
> > > > > From: Sultan Alsawaf <sultan@kerneltoast.com>
> > > > >
> > > > > 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 <stephan.gerhold@linaro.org>
> > > > > Link: https://lore.kernel.org/lkml/Z_Tlc6Qs-tYpxWYb@linaro.org
> > > > > Fixes: 8e461a1cb43d6 ("cpufreq: schedutil: Fix superfluous updates caused by need_freq_update")
> > > > > Signed-off-by: Sultan Alsawaf <sultan@kerneltoast.com>
> > > > > ---
> > > > >  kernel/sched/cpufreq_schedutil.c | 5 +++--
> > > > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> > > > > index 1a19d69b91ed3..f37b999854d52 100644
> > > > > --- a/kernel/sched/cpufreq_schedutil.c
> > > > > +++ b/kernel/sched/cpufreq_schedutil.c
> > > > > @@ -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;
> > > > >
> > > > > +       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().
> >
> > 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().
>
> > 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.
>
> > 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.
>
> 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?

Yes, it does.

> > 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.
> >
> > > 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.

Stale values can be observed, but in that case the new values will be
seen next time and limits_changed enforces picking them up.

Of course, if the limits change sufficiently often, schedutil will
fall behind, but then there is just too much noise anyway.

WRITE_ONCE() and READ_ONCE() could be used there, though, because the
compiler can do some damage in principle.

Re: [PATCH] cpufreq: schedutil: Don't ignore limit changes when util is unchanged

[Rafael J. Wysocki]

On Thu, Apr 10, 2025 at 9:50 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Thu, Apr 10, 2025 at 9:26 PM Sultan Alsawaf <sultan@kerneltoast.com> wrote:
> >
> > On Thu, Apr 10, 2025 at 08:47:38PM +0200, Rafael J. Wysocki wrote:
> > > On Thu, Apr 10, 2025 at 6:03 PM Sultan Alsawaf <sultan@kerneltoast.com> wrote:
> > > >
> > > > On Thu, Apr 10, 2025 at 05:34:39PM +0200, Rafael J. Wysocki wrote:
> > > > > On Thu, Apr 10, 2025 at 4:45 AM Sultan Alsawaf <sultan@kerneltoast.com> wrote:
> > > > > >
> > > > > > From: Sultan Alsawaf <sultan@kerneltoast.com>
> > > > > >
> > > > > > 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 <stephan.gerhold@linaro.org>
> > > > > > Link: https://lore.kernel.org/lkml/Z_Tlc6Qs-tYpxWYb@linaro.org
> > > > > > Fixes: 8e461a1cb43d6 ("cpufreq: schedutil: Fix superfluous updates caused by need_freq_update")
> > > > > > Signed-off-by: Sultan Alsawaf <sultan@kerneltoast.com>
> > > > > > ---
> > > > > >  kernel/sched/cpufreq_schedutil.c | 5 +++--
> > > > > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> > > > > > index 1a19d69b91ed3..f37b999854d52 100644
> > > > > > --- a/kernel/sched/cpufreq_schedutil.c
> > > > > > +++ b/kernel/sched/cpufreq_schedutil.c
> > > > > > @@ -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;
> > > > > >
> > > > > > +       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().
> > >
> > > 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().
> >
> > > 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.
> >
> > > 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.
> >
> > 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?
>
> Yes, it does.
>
> > > 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.
> > >
> > > > 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.

Well, I've just realized that this is not exactly the case because of
the way in which __resolve_freq() is used in cpufreq_set_policy().
That needs to be fixed.

Re: [PATCH] cpufreq: schedutil: Don't ignore limit changes when util is unchanged

[Xuewen Yan]

...
>
> 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/

Thanks!
Regards

Re: [PATCH] cpufreq: schedutil: Don't ignore limit changes when util is unchanged

[Rafael J. Wysocki]

On Fri, Apr 11, 2025 at 10:22 AM Xuewen Yan <xuewen.yan94@gmail.com> wrote:
>
> ...
> >
> > 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/

The first hunk is essentially a partial revert of the problematic
commit, but I'm not sure what you want to achieve with the second one.