| Message ID | 20230509180503.739208-1-wyes.karny@amd.com |
|---|---|
| Headers | show |
| Series | cpufreq/schedutil: Fix null pointer dereference in sugov_update_single_freq | expand |
Hi Rafael, Thanks for reviewing the patch. On 09 May 20:39, Rafael J. Wysocki wrote: ------------------------------------------>8-------------------------------------- > > > diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c > > > index 2548ec92faa2..007893514c87 100644 > > > --- a/drivers/cpufreq/intel_pstate.c > > > +++ b/drivers/cpufreq/intel_pstate.c > > > @@ -2698,8 +2698,6 @@ static int __intel_pstate_cpu_init(struct cpufreq_policy *policy) > > > > > > intel_pstate_init_acpi_perf_limits(policy); > > > > > > - policy->fast_switch_possible = true; > > > - > > > return 0; > > > } > > > > > > @@ -2955,6 +2953,7 @@ static int intel_cpufreq_cpu_init(struct cpufreq_policy *policy) > > > if (ret) > > > return ret; > > > > > > + policy->fast_switch_possible = true; > > I'm not sure what this is about. Is it a cleanup of intel_pstate? This patch intends to remove fast_switch_possible flag dependency from drivers which only use adjust_perf as frequency/pref update callback. As intel_pstate and amd_pstate driver has only adjust_perf and not fast_switch, therefore I'm removing that flag from these drivers. But intel_cpufreq has fast_switch therefore, only adding that flag for intel_cpufreq driver. Thanks & Regards, Wyes > > > > policy->cpuinfo.transition_latency = INTEL_CPUFREQ_TRANSITION_LATENCY; > > > /* This reflects the intel_pstate_get_cpu_pstates() setting. */ > > > policy->cur = policy->cpuinfo.min_freq; > > > diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h > > > index 26e2eb399484..7a32cfca26c9 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..f993ecf731a9 100644 > > > --- a/kernel/sched/cpufreq_schedutil.c > > > +++ b/kernel/sched/cpufreq_schedutil.c > > > @@ -776,7 +776,7 @@ static int sugov_start(struct cpufreq_policy *policy) > > > > > > if (policy_is_shared(policy)) > > > uu = sugov_update_shared; > > > - else if (policy->fast_switch_enabled && cpufreq_driver_has_adjust_perf()) > > > + else if (cpufreq_driver_has_adjust_perf()) > > > uu = sugov_update_single_perf; > > > else > > > uu = sugov_update_single_freq; > > > --
On Wed, May 10, 2023 at 7:43 AM Wyes Karny <wyes.karny@amd.com> wrote: > > Hi Rafael, > > Thanks for reviewing the patch. > > On 09 May 20:39, Rafael J. Wysocki wrote: > ------------------------------------------>8-------------------------------------- > > > > diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c > > > > index 2548ec92faa2..007893514c87 100644 > > > > --- a/drivers/cpufreq/intel_pstate.c > > > > +++ b/drivers/cpufreq/intel_pstate.c > > > > @@ -2698,8 +2698,6 @@ static int __intel_pstate_cpu_init(struct cpufreq_policy *policy) > > > > > > > > intel_pstate_init_acpi_perf_limits(policy); > > > > > > > > - policy->fast_switch_possible = true; > > > > - > > > > return 0; > > > > } > > > > > > > > @@ -2955,6 +2953,7 @@ static int intel_cpufreq_cpu_init(struct cpufreq_policy *policy) > > > > if (ret) > > > > return ret; > > > > > > > > + policy->fast_switch_possible = true; > > > > I'm not sure what this is about. Is it a cleanup of intel_pstate? > > This patch intends to remove fast_switch_possible flag dependency from > drivers which only use adjust_perf as frequency/pref update callback. As > intel_pstate and amd_pstate driver has only adjust_perf and not > fast_switch, therefore I'm removing that flag from these drivers. But > intel_cpufreq has fast_switch therefore, only adding that flag for > intel_cpufreq driver. But is it really better to change it? It works correctly as-is AFAICS. In any case, the intel_pstate change should be a separate patch, because it is not directly related to the other changes in the $subject one IMV.
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 disabling `fast_switch_enabled` flag if `fast_switch` callback is not set and removing `fast_switch_enabled` flag dependency for adjust_perf callback. 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. Changelog: v1 -> v2 - Remove fast_switch_enabled flag dependency for adjust_perf callback v1: https://lore.kernel.org/linux-pm/20230410095045.14872-1-wyes.karny@amd.com/ Gautham R. Shenoy (1): amd_pstate: Add ->fast_switch() callback Wyes Karny (1): cpufreq/schedutil: Remove fast_switch_possible flag if driver doesn't set fast_switch drivers/cpufreq/amd-pstate.c | 46 +++++++++++++++++++++++++------- drivers/cpufreq/cpufreq.c | 20 +++++++++++++- drivers/cpufreq/intel_pstate.c | 3 +-- include/linux/cpufreq.h | 1 + kernel/sched/cpufreq_schedutil.c | 2 +- 5 files changed, 59 insertions(+), 13 deletions(-)