Currently, amd_pstate only uses `adjust_perf` and `target` callback
functions to get the frequency/effective utilization data from scaling
governors. Schedutil generally avoids calling `target` and `fast_switch`
functions if `adjust_perf` function pointer is set for the driver. But in
some rare cases, schedutil tries to call `fast_switch` function even the
function pointer is not set. This happens when frequency invariance is
turned off. When frequency invariance is turned off schedutil falls
back to `sugov_update_single_freq` which currently relies on the
`fast_switch` callback.
Currently, frequency invariance is turned off when any anomaly is
detected with aperf/mperf readings. Which triggers this problem.
Fix this by adding a NULL check before calling `fast_switch` in
`sugov_update_single_freq`. But this will force schedutil to take a
slower path to update frequency. Therefore to fix this add fast_switch
function on amd_pstate to take advantage of fast frequency update.
Gautham R. Shenoy (1):
amd_pstate: Add ->fast_switch() callback
Wyes Karny (1):
cpufreq/schedutil: Add fast_switch callback check
drivers/cpufreq/amd-pstate.c | 33 ++++++++++++++++++++++++++------
drivers/cpufreq/cpufreq.c | 11 +++++++++++
include/linux/cpufreq.h | 1 +
kernel/sched/cpufreq_schedutil.c | 2 +-
4 files changed, 40 insertions(+), 7 deletions(-)
--
2.34.1
From: "Gautham R. Shenoy" <[email protected]>
Schedutil normally calls the adjust_perf callback for drivers with
adjust_perf callback available and fast_switch_possible flag set.
However, when frequency invariance is disabled and schedutil tries to
invoke fast_switch, and that callback is NULL, it schedules a kthread to
invoke the target() callback, which could slow down the frequency
update.
Prevent the frequency update slow down by implementing the fast_switch
callback for amd_pstate.
Signed-off-by: Wyes Karny <[email protected]>
Signed-off-by: Gautham R. Shenoy <[email protected]>
---
drivers/cpufreq/amd-pstate.c | 33 +++++++++++++++++++++++++++------
1 file changed, 27 insertions(+), 6 deletions(-)
diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index 73c7643b2697..b3d594b35c65 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -422,9 +422,8 @@ static int amd_pstate_verify(struct cpufreq_policy_data *policy)
return 0;
}
-static int amd_pstate_target(struct cpufreq_policy *policy,
- unsigned int target_freq,
- unsigned int relation)
+static int amd_pstate_update_freq(struct cpufreq_policy *policy,
+ unsigned int target_freq, bool fast_switch)
{
struct cpufreq_freqs freqs;
struct amd_cpudata *cpudata = policy->driver_data;
@@ -443,14 +442,35 @@ static int amd_pstate_target(struct cpufreq_policy *policy,
des_perf = DIV_ROUND_CLOSEST(target_freq * cap_perf,
cpudata->max_freq);
- cpufreq_freq_transition_begin(policy, &freqs);
+ WARN_ON(fast_switch && !policy->fast_switch_enabled);
+ /*
+ * If fast_switch is desired, then there aren't any registered
+ * transition notifiers. See comment for
+ * cpufreq_enable_fast_switch().
+ */
+ if (!fast_switch)
+ cpufreq_freq_transition_begin(policy, &freqs);
amd_pstate_update(cpudata, min_perf, des_perf,
- max_perf, false);
- cpufreq_freq_transition_end(policy, &freqs, false);
+ max_perf, fast_switch);
+ if (!fast_switch)
+ cpufreq_freq_transition_end(policy, &freqs, false);
return 0;
}
+static int amd_pstate_target(struct cpufreq_policy *policy,
+ unsigned int target_freq,
+ unsigned int relation)
+{
+ return amd_pstate_update_freq(policy, target_freq, false);
+}
+
+static unsigned int amd_pstate_fast_switch(struct cpufreq_policy *policy,
+ unsigned int target_freq)
+{
+ return amd_pstate_update_freq(policy, target_freq, true);
+}
+
static void amd_pstate_adjust_perf(unsigned int cpu,
unsigned long _min_perf,
unsigned long target_perf,
@@ -1228,6 +1248,7 @@ static struct cpufreq_driver amd_pstate_driver = {
.flags = CPUFREQ_CONST_LOOPS | CPUFREQ_NEED_UPDATE_LIMITS,
.verify = amd_pstate_verify,
.target = amd_pstate_target,
+ .fast_switch = amd_pstate_fast_switch,
.init = amd_pstate_cpu_init,
.exit = amd_pstate_cpu_exit,
.suspend = amd_pstate_cpu_suspend,
--
2.34.1
The set value of `fast_switch_enabled` flag doesn't guarantee that
fast_switch callback is set. For some drivers such as amd_pstate, the
adjust_perf callback is used but it still sets `fast_switch_possible`
flag. This is not wrong because this flag doesn't imply fast_switch
callback is set, it implies whether the driver can guarantee that
frequency can be changed on any CPU sharing the policy and that the
change will affect all of the policy CPUs without the need to send any
IPIs or issue callbacks from the notifier chain. Therefore add an extra
NULL check before calling fast_switch in sugov_update_single_freq
function.
Ideally `sugov_update_single_freq` function should not be called with
amd_pstate. But in a corner case scenario, when aperf/mperf overflow
occurs, kernel disables frequency invariance calculation which causes
schedutil to fallback to sugov_update_single_freq which currently relies
on the fast_switch callback.
Normal flow:
sugov_update_single_perf
cpufreq_driver_adjust_perf
cpufreq_driver->adjust_perf
Error case flow:
sugov_update_single_perf
sugov_update_single_freq <-- This is chosen because the freq invariant is disabled due to aperf/mperf overflow
cpufreq_driver_fast_switch
cpufreq_driver->fast_switch <-- Here NULL pointer dereference is happening, because fast_switch is not set
Fix this NULL pointer dereference issue by doing a NULL check.
Fixes: a61dec744745 ("cpufreq: schedutil: Avoid missing updates for one-CPU policies")
Signed-off-by: Wyes Karny <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: [email protected]
---
drivers/cpufreq/cpufreq.c | 11 +++++++++++
include/linux/cpufreq.h | 1 +
kernel/sched/cpufreq_schedutil.c | 2 +-
3 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 6d8fd3b8dcb5..364d31b55380 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -2138,6 +2138,17 @@ unsigned int cpufreq_driver_fast_switch(struct cpufreq_policy *policy,
}
EXPORT_SYMBOL_GPL(cpufreq_driver_fast_switch);
+/**
+ * cpufreq_driver_has_fast_switch - Check "fast switch" callback.
+ *
+ * Return 'true' if the ->fast_switch callback is present for the
+ * current driver or 'false' otherwise.
+ */
+bool cpufreq_driver_has_fast_switch(void)
+{
+ return !!cpufreq_driver->fast_switch;
+}
+
/**
* cpufreq_driver_adjust_perf - Adjust CPU performance level in one go.
* @cpu: Target CPU.
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 65623233ab2f..8a9286fc718b 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -604,6 +604,7 @@ struct cpufreq_governor {
/* Pass a target to the cpufreq driver */
unsigned int cpufreq_driver_fast_switch(struct cpufreq_policy *policy,
unsigned int target_freq);
+bool cpufreq_driver_has_fast_switch(void);
void cpufreq_driver_adjust_perf(unsigned int cpu,
unsigned long min_perf,
unsigned long target_perf,
diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
index e3211455b203..a1c449525ac2 100644
--- a/kernel/sched/cpufreq_schedutil.c
+++ b/kernel/sched/cpufreq_schedutil.c
@@ -364,7 +364,7 @@ static void sugov_update_single_freq(struct update_util_data *hook, u64 time,
* concurrently on two different CPUs for the same target and it is not
* necessary to acquire the lock in the fast switch case.
*/
- if (sg_policy->policy->fast_switch_enabled) {
+ if (sg_policy->policy->fast_switch_enabled && cpufreq_driver_has_fast_switch()) {
cpufreq_driver_fast_switch(sg_policy->policy, next_f);
} else {
raw_spin_lock(&sg_policy->update_lock);
--
2.34.1
On Mon, Apr 10, 2023 at 11:53 AM Wyes Karny <[email protected]> wrote:
>
> The set value of `fast_switch_enabled` flag doesn't guarantee that
> fast_switch callback is set. For some drivers such as amd_pstate, the
> adjust_perf callback is used but it still sets `fast_switch_possible`
> flag. This is not wrong because this flag doesn't imply fast_switch
> callback is set, it implies whether the driver can guarantee that
> frequency can be changed on any CPU sharing the policy and that the
> change will affect all of the policy CPUs without the need to send any
> IPIs or issue callbacks from the notifier chain. Therefore add an extra
> NULL check before calling fast_switch in sugov_update_single_freq
> function.
>
> Ideally `sugov_update_single_freq` function should not be called with
> amd_pstate. But in a corner case scenario, when aperf/mperf overflow
> occurs, kernel disables frequency invariance calculation which causes
> schedutil to fallback to sugov_update_single_freq which currently relies
> on the fast_switch callback.
Yes, it does. Which is why that callback must be provided if the
driver sets fast_switch_enabled.
Overall, adjust_perf is optional, but fast_switch_enabled can only be
set if fast_switch is actually present.
Please fix the driver.
>
> Normal flow:
> sugov_update_single_perf
> cpufreq_driver_adjust_perf
> cpufreq_driver->adjust_perf
>
> Error case flow:
> sugov_update_single_perf
> sugov_update_single_freq <-- This is chosen because the freq invariant is disabled due to aperf/mperf overflow
> cpufreq_driver_fast_switch
> cpufreq_driver->fast_switch <-- Here NULL pointer dereference is happening, because fast_switch is not set
>
> Fix this NULL pointer dereference issue by doing a NULL check.
>
> Fixes: a61dec744745 ("cpufreq: schedutil: Avoid missing updates for one-CPU policies")
> Signed-off-by: Wyes Karny <[email protected]>
>
> Cc: "Rafael J. Wysocki" <[email protected]>
> Cc: [email protected]
> ---
> drivers/cpufreq/cpufreq.c | 11 +++++++++++
> include/linux/cpufreq.h | 1 +
> kernel/sched/cpufreq_schedutil.c | 2 +-
> 3 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 6d8fd3b8dcb5..364d31b55380 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -2138,6 +2138,17 @@ unsigned int cpufreq_driver_fast_switch(struct cpufreq_policy *policy,
> }
> EXPORT_SYMBOL_GPL(cpufreq_driver_fast_switch);
>
> +/**
> + * cpufreq_driver_has_fast_switch - Check "fast switch" callback.
> + *
> + * Return 'true' if the ->fast_switch callback is present for the
> + * current driver or 'false' otherwise.
> + */
> +bool cpufreq_driver_has_fast_switch(void)
> +{
> + return !!cpufreq_driver->fast_switch;
> +}
> +
> /**
> * cpufreq_driver_adjust_perf - Adjust CPU performance level in one go.
> * @cpu: Target CPU.
> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> index 65623233ab2f..8a9286fc718b 100644
> --- a/include/linux/cpufreq.h
> +++ b/include/linux/cpufreq.h
> @@ -604,6 +604,7 @@ struct cpufreq_governor {
> /* Pass a target to the cpufreq driver */
> unsigned int cpufreq_driver_fast_switch(struct cpufreq_policy *policy,
> unsigned int target_freq);
> +bool cpufreq_driver_has_fast_switch(void);
> void cpufreq_driver_adjust_perf(unsigned int cpu,
> unsigned long min_perf,
> unsigned long target_perf,
> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> index e3211455b203..a1c449525ac2 100644
> --- a/kernel/sched/cpufreq_schedutil.c
> +++ b/kernel/sched/cpufreq_schedutil.c
> @@ -364,7 +364,7 @@ static void sugov_update_single_freq(struct update_util_data *hook, u64 time,
> * concurrently on two different CPUs for the same target and it is not
> * necessary to acquire the lock in the fast switch case.
> */
> - if (sg_policy->policy->fast_switch_enabled) {
> + if (sg_policy->policy->fast_switch_enabled && cpufreq_driver_has_fast_switch()) {
> cpufreq_driver_fast_switch(sg_policy->policy, next_f);
> } else {
> raw_spin_lock(&sg_policy->update_lock);
> --
> 2.34.1
>
Hi Rafael,
On 11 Apr 17:55, Rafael J. Wysocki wrote:
> On Mon, Apr 10, 2023 at 11:53 AM Wyes Karny <[email protected]> wrote:
> >
> > The set value of `fast_switch_enabled` flag doesn't guarantee that
> > fast_switch callback is set. For some drivers such as amd_pstate, the
> > adjust_perf callback is used but it still sets `fast_switch_possible`
> > flag. This is not wrong because this flag doesn't imply fast_switch
> > callback is set, it implies whether the driver can guarantee that
> > frequency can be changed on any CPU sharing the policy and that the
> > change will affect all of the policy CPUs without the need to send any
> > IPIs or issue callbacks from the notifier chain. Therefore add an extra
> > NULL check before calling fast_switch in sugov_update_single_freq
> > function.
> >
> > Ideally `sugov_update_single_freq` function should not be called with
> > amd_pstate. But in a corner case scenario, when aperf/mperf overflow
> > occurs, kernel disables frequency invariance calculation which causes
> > schedutil to fallback to sugov_update_single_freq which currently relies
> > on the fast_switch callback.
>
> Yes, it does. Which is why that callback must be provided if the
> driver sets fast_switch_enabled.
>
> Overall, adjust_perf is optional, but fast_switch_enabled can only be
> set if fast_switch is actually present.
Then should the below logic be changed in sugov_start function?
'''
else if (policy->fast_switch_enabled && cpufreq_driver_has_adjust_perf())
uu = sugov_update_single_perf;
'''
This logic restricts the selection of adjust_perf function based on
fast_switch_enabled flag. If this fast_switch_enabled check is removed
then amd_pstate driver can disable this flag and shedutil can select
adjust_perf without this dependency.
Thanks,
Wyes
>
> Please fix the driver.
>
> >
> > Normal flow:
> > sugov_update_single_perf
> > cpufreq_driver_adjust_perf
> > cpufreq_driver->adjust_perf
> >
> > Error case flow:
> > sugov_update_single_perf
> > sugov_update_single_freq <-- This is chosen because the freq invariant is disabled due to aperf/mperf overflow
> > cpufreq_driver_fast_switch
> > cpufreq_driver->fast_switch <-- Here NULL pointer dereference is happening, because fast_switch is not set
> >
> > Fix this NULL pointer dereference issue by doing a NULL check.
> >
> > Fixes: a61dec744745 ("cpufreq: schedutil: Avoid missing updates for one-CPU policies")
> > Signed-off-by: Wyes Karny <[email protected]>
> >
> > Cc: "Rafael J. Wysocki" <[email protected]>
> > Cc: [email protected]
> > ---
> > drivers/cpufreq/cpufreq.c | 11 +++++++++++
> > include/linux/cpufreq.h | 1 +
> > kernel/sched/cpufreq_schedutil.c | 2 +-
> > 3 files changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > index 6d8fd3b8dcb5..364d31b55380 100644
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -2138,6 +2138,17 @@ unsigned int cpufreq_driver_fast_switch(struct cpufreq_policy *policy,
> > }
> > EXPORT_SYMBOL_GPL(cpufreq_driver_fast_switch);
> >
> > +/**
> > + * cpufreq_driver_has_fast_switch - Check "fast switch" callback.
> > + *
> > + * Return 'true' if the ->fast_switch callback is present for the
> > + * current driver or 'false' otherwise.
> > + */
> > +bool cpufreq_driver_has_fast_switch(void)
> > +{
> > + return !!cpufreq_driver->fast_switch;
> > +}
> > +
> > /**
> > * cpufreq_driver_adjust_perf - Adjust CPU performance level in one go.
> > * @cpu: Target CPU.
> > diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> > index 65623233ab2f..8a9286fc718b 100644
> > --- a/include/linux/cpufreq.h
> > +++ b/include/linux/cpufreq.h
> > @@ -604,6 +604,7 @@ struct cpufreq_governor {
> > /* Pass a target to the cpufreq driver */
> > unsigned int cpufreq_driver_fast_switch(struct cpufreq_policy *policy,
> > unsigned int target_freq);
> > +bool cpufreq_driver_has_fast_switch(void);
> > void cpufreq_driver_adjust_perf(unsigned int cpu,
> > unsigned long min_perf,
> > unsigned long target_perf,
> > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> > index e3211455b203..a1c449525ac2 100644
> > --- a/kernel/sched/cpufreq_schedutil.c
> > +++ b/kernel/sched/cpufreq_schedutil.c
> > @@ -364,7 +364,7 @@ static void sugov_update_single_freq(struct update_util_data *hook, u64 time,
> > * concurrently on two different CPUs for the same target and it is not
> > * necessary to acquire the lock in the fast switch case.
> > */
> > - if (sg_policy->policy->fast_switch_enabled) {
> > + if (sg_policy->policy->fast_switch_enabled && cpufreq_driver_has_fast_switch()) {
> > cpufreq_driver_fast_switch(sg_policy->policy, next_f);
> > } else {
> > raw_spin_lock(&sg_policy->update_lock);
> > --
> > 2.34.1
> >
Hi Rafael,
On 11 Apr 17:55, Rafael J. Wysocki wrote:
> On Mon, Apr 10, 2023 at 11:53 AM Wyes Karny <[email protected]> wrote:
> >
> > The set value of `fast_switch_enabled` flag doesn't guarantee that
> > fast_switch callback is set. For some drivers such as amd_pstate, the
> > adjust_perf callback is used but it still sets `fast_switch_possible`
> > flag. This is not wrong because this flag doesn't imply fast_switch
> > callback is set, it implies whether the driver can guarantee that
> > frequency can be changed on any CPU sharing the policy and that the
> > change will affect all of the policy CPUs without the need to send any
> > IPIs or issue callbacks from the notifier chain. Therefore add an extra
> > NULL check before calling fast_switch in sugov_update_single_freq
> > function.
> >
> > Ideally `sugov_update_single_freq` function should not be called with
> > amd_pstate. But in a corner case scenario, when aperf/mperf overflow
> > occurs, kernel disables frequency invariance calculation which causes
> > schedutil to fallback to sugov_update_single_freq which currently relies
> > on the fast_switch callback.
>
> Yes, it does. Which is why that callback must be provided if the
> driver sets fast_switch_enabled.
>
> Overall, adjust_perf is optional, but fast_switch_enabled can only be
> set if fast_switch is actually present.
>
> Please fix the driver.
FYI this issue is not exclusive to amd_pstate driver. Even intel_pstate
driver sets fast_switch_possible = ture without setting fast_switch
callback. If the driver only has adjust_perf even then
fast_switch_possible = ture is necessary because without this flag sugov
won't choose `sugov_update_single_perf`.
Thanks,
Wyes
>
> >
> > Normal flow:
> > sugov_update_single_perf
> > cpufreq_driver_adjust_perf
> > cpufreq_driver->adjust_perf
> >
> > Error case flow:
> > sugov_update_single_perf
> > sugov_update_single_freq <-- This is chosen because the freq invariant is disabled due to aperf/mperf overflow
> > cpufreq_driver_fast_switch
> > cpufreq_driver->fast_switch <-- Here NULL pointer dereference is happening, because fast_switch is not set
> >
> > Fix this NULL pointer dereference issue by doing a NULL check.
> >
> > Fixes: a61dec744745 ("cpufreq: schedutil: Avoid missing updates for one-CPU policies")
> > Signed-off-by: Wyes Karny <[email protected]>
> >
> > Cc: "Rafael J. Wysocki" <[email protected]>
> > Cc: [email protected]
> > ---
> > drivers/cpufreq/cpufreq.c | 11 +++++++++++
> > include/linux/cpufreq.h | 1 +
> > kernel/sched/cpufreq_schedutil.c | 2 +-
> > 3 files changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > index 6d8fd3b8dcb5..364d31b55380 100644
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -2138,6 +2138,17 @@ unsigned int cpufreq_driver_fast_switch(struct cpufreq_policy *policy,
> > }
> > EXPORT_SYMBOL_GPL(cpufreq_driver_fast_switch);
> >
> > +/**
> > + * cpufreq_driver_has_fast_switch - Check "fast switch" callback.
> > + *
> > + * Return 'true' if the ->fast_switch callback is present for the
> > + * current driver or 'false' otherwise.
> > + */
> > +bool cpufreq_driver_has_fast_switch(void)
> > +{
> > + return !!cpufreq_driver->fast_switch;
> > +}
> > +
> > /**
> > * cpufreq_driver_adjust_perf - Adjust CPU performance level in one go.
> > * @cpu: Target CPU.
> > diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> > index 65623233ab2f..8a9286fc718b 100644
> > --- a/include/linux/cpufreq.h
> > +++ b/include/linux/cpufreq.h
> > @@ -604,6 +604,7 @@ struct cpufreq_governor {
> > /* Pass a target to the cpufreq driver */
> > unsigned int cpufreq_driver_fast_switch(struct cpufreq_policy *policy,
> > unsigned int target_freq);
> > +bool cpufreq_driver_has_fast_switch(void);
> > void cpufreq_driver_adjust_perf(unsigned int cpu,
> > unsigned long min_perf,
> > unsigned long target_perf,
> > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> > index e3211455b203..a1c449525ac2 100644
> > --- a/kernel/sched/cpufreq_schedutil.c
> > +++ b/kernel/sched/cpufreq_schedutil.c
> > @@ -364,7 +364,7 @@ static void sugov_update_single_freq(struct update_util_data *hook, u64 time,
> > * concurrently on two different CPUs for the same target and it is not
> > * necessary to acquire the lock in the fast switch case.
> > */
> > - if (sg_policy->policy->fast_switch_enabled) {
> > + if (sg_policy->policy->fast_switch_enabled && cpufreq_driver_has_fast_switch()) {
> > cpufreq_driver_fast_switch(sg_policy->policy, next_f);
> > } else {
> > raw_spin_lock(&sg_policy->update_lock);
> > --
> > 2.34.1
> >