| Message ID | 8560367.NyiUUSuA9g@rjwysocki.net |
|---|---|
| State | New |
| Headers | show |
| Series | cpufreq: cpufreq_update_limits() fix and some cleanups | expand |
On Fri, Mar 28, 2025 at 9:49 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Since cpufreq_update_limits() obtains a cpufreq policy pointer for the > given CPU and reference counts the corresponding policy object, it may > as well pass the policy pointer to the cpufreq driver's ->update_limits() > callback which allows that callback to avoid invoking cpufreq_cpu_get() > for the same CPU. > > Accordingly, redefine ->update_limits() to take a policy pointer instead > of a CPU number and update both drivers implementing it, intel_pstate > and amd-pstate, as needed. > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Hi Srinivas, If you have any concerns regarding this patch, please let me know (note that it is based on the [05/10]). > --- > drivers/cpufreq/amd-pstate.c | 7 ++----- > drivers/cpufreq/cpufreq.c | 2 +- > drivers/cpufreq/intel_pstate.c | 29 ++++++++++++++++++----------- > include/linux/cpufreq.h | 2 +- > 4 files changed, 22 insertions(+), 18 deletions(-) > > --- a/drivers/cpufreq/amd-pstate.c > +++ b/drivers/cpufreq/amd-pstate.c > @@ -821,19 +821,16 @@ > schedule_work(&sched_prefcore_work); > } > > -static void amd_pstate_update_limits(unsigned int cpu) > +static void amd_pstate_update_limits(struct cpufreq_policy *policy) > { > - struct cpufreq_policy *policy __free(put_cpufreq_policy) = cpufreq_cpu_get(cpu); > struct amd_cpudata *cpudata; > u32 prev_high = 0, cur_high = 0; > bool highest_perf_changed = false; > + unsigned int cpu = policy->cpu; > > if (!amd_pstate_prefcore) > return; > > - if (!policy) > - return; > - > if (amd_get_highest_perf(cpu, &cur_high)) > return; > > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -2741,7 +2741,7 @@ > return; > > if (cpufreq_driver->update_limits) > - cpufreq_driver->update_limits(cpu); > + cpufreq_driver->update_limits(policy); > else > cpufreq_policy_refresh(policy); > } > --- a/drivers/cpufreq/intel_pstate.c > +++ b/drivers/cpufreq/intel_pstate.c > @@ -1353,14 +1353,9 @@ > cpufreq_update_policy(cpu); > } > > -static bool intel_pstate_update_max_freq(struct cpudata *cpudata) > +static void __intel_pstate_update_max_freq(struct cpufreq_policy *policy, > + struct cpudata *cpudata) > { > - struct cpufreq_policy *policy __free(put_cpufreq_policy); > - > - policy = cpufreq_cpu_get(cpudata->cpu); > - if (!policy) > - return false; > - > guard(cpufreq_policy_write)(policy); > > if (hwp_active) > @@ -1370,16 +1365,28 @@ > cpudata->pstate.max_freq : cpudata->pstate.turbo_freq; > > refresh_frequency_limits(policy); > +} > + > +static bool intel_pstate_update_max_freq(struct cpudata *cpudata) > +{ > + struct cpufreq_policy *policy __free(put_cpufreq_policy); > + > + policy = cpufreq_cpu_get(cpudata->cpu); > + if (!policy) > + return false; > + > + __intel_pstate_update_max_freq(policy, cpudata); > > return true; > } > > -static void intel_pstate_update_limits(unsigned int cpu) > +static void intel_pstate_update_limits(struct cpufreq_policy *policy) > { > - struct cpudata *cpudata = all_cpu_data[cpu]; > + struct cpudata *cpudata = all_cpu_data[policy->cpu]; > + > + __intel_pstate_update_max_freq(policy, cpudata); > > - if (intel_pstate_update_max_freq(cpudata)) > - hybrid_update_capacity(cpudata); > + hybrid_update_capacity(cpudata); > } > > static void intel_pstate_update_limits_for_all(void) > --- a/include/linux/cpufreq.h > +++ b/include/linux/cpufreq.h > @@ -399,7 +399,7 @@ > unsigned int (*get)(unsigned int cpu); > > /* Called to update policy limits on firmware notifications. */ > - void (*update_limits)(unsigned int cpu); > + void (*update_limits)(struct cpufreq_policy *policy); > > /* optional */ > int (*bios_limit)(int cpu, unsigned int *limit); > > > >
On Mon, 2025-04-07 at 20:48 +0200, Rafael J. Wysocki wrote: > On Fri, Mar 28, 2025 at 9:49 PM Rafael J. Wysocki <rjw@rjwysocki.net> > wrote: > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > Since cpufreq_update_limits() obtains a cpufreq policy pointer for > > the > > given CPU and reference counts the corresponding policy object, it > > may > > as well pass the policy pointer to the cpufreq driver's - > > >update_limits() > > callback which allows that callback to avoid invoking > > cpufreq_cpu_get() > > for the same CPU. > > > > Accordingly, redefine ->update_limits() to take a policy pointer > > instead > > of a CPU number and update both drivers implementing it, > > intel_pstate > > and amd-pstate, as needed. > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > Hi Rafael, > Hi Srinivas, > > If you have any concerns regarding this patch, please let me know > (note that it is based on the [05/10]). > Changes looks fine, but wants to test out some update limits from interrupt path. Checked your branches at linux-pm, not able to locate in any branch to apply. Please point me to a branch. Thanks, Srinivas > > --- > > drivers/cpufreq/amd-pstate.c | 7 ++----- > > drivers/cpufreq/cpufreq.c | 2 +- > > drivers/cpufreq/intel_pstate.c | 29 ++++++++++++++++++---------- > > - > > include/linux/cpufreq.h | 2 +- > > 4 files changed, 22 insertions(+), 18 deletions(-) > > > > --- a/drivers/cpufreq/amd-pstate.c > > +++ b/drivers/cpufreq/amd-pstate.c > > @@ -821,19 +821,16 @@ > > schedule_work(&sched_prefcore_work); > > } > > > > -static void amd_pstate_update_limits(unsigned int cpu) > > +static void amd_pstate_update_limits(struct cpufreq_policy > > *policy) > > { > > - struct cpufreq_policy *policy __free(put_cpufreq_policy) = > > cpufreq_cpu_get(cpu); > > struct amd_cpudata *cpudata; > > u32 prev_high = 0, cur_high = 0; > > bool highest_perf_changed = false; > > + unsigned int cpu = policy->cpu; > > > > if (!amd_pstate_prefcore) > > return; > > > > - if (!policy) > > - return; > > - > > if (amd_get_highest_perf(cpu, &cur_high)) > > return; > > > > --- a/drivers/cpufreq/cpufreq.c > > +++ b/drivers/cpufreq/cpufreq.c > > @@ -2741,7 +2741,7 @@ > > return; > > > > if (cpufreq_driver->update_limits) > > - cpufreq_driver->update_limits(cpu); > > + cpufreq_driver->update_limits(policy); > > else > > cpufreq_policy_refresh(policy); > > } > > --- a/drivers/cpufreq/intel_pstate.c > > +++ b/drivers/cpufreq/intel_pstate.c > > @@ -1353,14 +1353,9 @@ > > cpufreq_update_policy(cpu); > > } > > > > -static bool intel_pstate_update_max_freq(struct cpudata *cpudata) > > +static void __intel_pstate_update_max_freq(struct cpufreq_policy > > *policy, > > + struct cpudata *cpudata) > > { > > - struct cpufreq_policy *policy __free(put_cpufreq_policy); > > - > > - policy = cpufreq_cpu_get(cpudata->cpu); > > - if (!policy) > > - return false; > > - > > guard(cpufreq_policy_write)(policy); > > > > if (hwp_active) > > @@ -1370,16 +1365,28 @@ > > cpudata->pstate.max_freq : cpudata- > > >pstate.turbo_freq; > > > > refresh_frequency_limits(policy); > > +} > > + > > +static bool intel_pstate_update_max_freq(struct cpudata *cpudata) > > +{ > > + struct cpufreq_policy *policy __free(put_cpufreq_policy); > > + > > + policy = cpufreq_cpu_get(cpudata->cpu); > > + if (!policy) > > + return false; > > + > > + __intel_pstate_update_max_freq(policy, cpudata); > > > > return true; > > } > > > > -static void intel_pstate_update_limits(unsigned int cpu) > > +static void intel_pstate_update_limits(struct cpufreq_policy > > *policy) > > { > > - struct cpudata *cpudata = all_cpu_data[cpu]; > > + struct cpudata *cpudata = all_cpu_data[policy->cpu]; > > + > > + __intel_pstate_update_max_freq(policy, cpudata); > > > > - if (intel_pstate_update_max_freq(cpudata)) > > - hybrid_update_capacity(cpudata); > > + hybrid_update_capacity(cpudata); > > } > > > > static void intel_pstate_update_limits_for_all(void) > > --- a/include/linux/cpufreq.h > > +++ b/include/linux/cpufreq.h > > @@ -399,7 +399,7 @@ > > unsigned int (*get)(unsigned int cpu); > > > > /* Called to update policy limits on firmware > > notifications. */ > > - void (*update_limits)(unsigned int cpu); > > + void (*update_limits)(struct cpufreq_policy > > *policy); > > > > /* optional */ > > int (*bios_limit)(int cpu, unsigned int > > *limit); > > > > > > > > >
On 2025.04.07 15:38 srinivas pandruvada wrote: > On Mon, 2025-04-07 at 20:48 +0200, Rafael J. Wysocki wrote: >> On Fri, Mar 28, 2025 at 9:49 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote: >>> >>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >>> >>> Since cpufreq_update_limits() obtains a cpufreq policy pointer for >>> the >>> given CPU and reference counts the corresponding policy object, it >>> may >>> as well pass the policy pointer to the cpufreq driver's - >>> >update_limits() >>> callback which allows that callback to avoid invoking >>> cpufreq_cpu_get() >>> for the same CPU. >>> >>> Accordingly, redefine ->update_limits() to take a policy pointer >>> instead >>> of a CPU number and update both drivers implementing it, >>> intel_pstate >>> and amd-pstate, as needed. >>> >>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >> > Hi Rafael, > >> Hi Srinivas, >> >> If you have any concerns regarding this patch, please let me know >> (note that it is based on the [05/10]). >> > Changes looks fine, but wants to test out some update limits from > interrupt path. > Checked your branches at linux-pm, not able to locate in any branch to > apply. > Please point me to a branch. Hi Srinivas, You can get the series from patchworks [1]. Then just edit it, deleting patch 1 of 10, because that one was included in kernel 6.15-rc1 The rest will apply cleanly to kernel 6.15-rc1. I just did all this in the last hour, because I wanted to check if the patchset fixed a years old issue with HWP enabled, intel_cpufreq, schedutil, minimum frequency set above hardware minimum was properly reflected in scaling_cur_freq when the frequency was stale. [2] The issue is not fixed. [1] https://patchwork.kernel.org/project/linux-pm/patch/2315023.iZASKD2KPV@rjwysocki.net/ [2] https://lore.kernel.org/linux-pm/CAAYoRsU2=qOUhBKSRskcoRXSgBudWgDNVvKtJA+c22cPa8EZ1Q@mail.gmail.com/
On Mon, 2025-04-07 at 16:49 -0700, Doug Smythies wrote: > On 2025.04.07 15:38 srinivas pandruvada wrote: > > On Mon, 2025-04-07 at 20:48 +0200, Rafael J. Wysocki wrote: > > > On Fri, Mar 28, 2025 at 9:49 PM Rafael J. Wysocki > > > <rjw@rjwysocki.net> wrote: > > > > > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > > > Since cpufreq_update_limits() obtains a cpufreq policy pointer > > > > for > > > > the > > > > given CPU and reference counts the corresponding policy object, > > > > it > > > > may > > > > as well pass the policy pointer to the cpufreq driver's - > > > > > update_limits() > > > > callback which allows that callback to avoid invoking > > > > cpufreq_cpu_get() > > > > for the same CPU. > > > > > > > > Accordingly, redefine ->update_limits() to take a policy > > > > pointer > > > > instead > > > > of a CPU number and update both drivers implementing it, > > > > intel_pstate > > > > and amd-pstate, as needed. > > > > > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > Hi Rafael, > > > > > Hi Srinivas, > > > > > > If you have any concerns regarding this patch, please let me know > > > (note that it is based on the [05/10]). > > > > > Changes looks fine, but wants to test out some update limits from > > interrupt path. > > Checked your branches at linux-pm, not able to locate in any branch > > to > > apply. > > Please point me to a branch. > > Hi Srinivas, > > You can get the series from patchworks [1]. > Then just edit it, deleting patch 1 of 10, because that one was > included in kernel 6.15-rc1 > The rest will apply cleanly to kernel 6.15-rc1. > Hi Doug, You are correct. But I prefer a branch usually as there may be other fixes so that I can verify once. Thanks, Srinivas > I just did all this in the last hour, because I wanted to check if > the patchset fixed a years old > issue with HWP enabled, intel_cpufreq, schedutil, minimum frequency > set above hardware > minimum was properly reflected in scaling_cur_freq when the > frequency was stale. [2] > The issue is not fixed. > > [1] > https://patchwork.kernel.org/project/linux-pm/patch/2315023.iZASKD2KPV@rjwysocki.net/ > [2] > https://lore.kernel.org/linux-pm/CAAYoRsU2=qOUhBKSRskcoRXSgBudWgDNVvKtJA+c22cPa8EZ1Q@mail.gmail.com/ > > >
On Tue, Apr 8, 2025 at 7:47 PM srinivas pandruvada <srinivas.pandruvada@linux.intel.com> wrote: > > On Tue, 2025-04-08 at 15:37 +0200, Rafael J. Wysocki wrote: > > On Tue, Apr 8, 2025 at 1:41 PM Rafael J. Wysocki <rafael@kernel.org> > > wrote: > > > > > > On Tue, Apr 8, 2025 at 12:28 AM srinivas pandruvada > > > <srinivas.pandruvada@linux.intel.com> wrote: > > > > > > > > On Mon, 2025-04-07 at 20:48 +0200, Rafael J. Wysocki wrote: > > > > > On Fri, Mar 28, 2025 at 9:49 PM Rafael J. Wysocki > > > > > <rjw@rjwysocki.net> > > > > > wrote: > > > > > > > > > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > > > > > > > Since cpufreq_update_limits() obtains a cpufreq policy > > > > > > pointer for > > > > > > the > > > > > > given CPU and reference counts the corresponding policy > > > > > > object, it > > > > > > may > > > > > > as well pass the policy pointer to the cpufreq driver's - > > > > > > > update_limits() > > > > > > callback which allows that callback to avoid invoking > > > > > > cpufreq_cpu_get() > > > > > > for the same CPU. > > > > > > > > > > > > Accordingly, redefine ->update_limits() to take a policy > > > > > > pointer > > > > > > instead > > > > > > of a CPU number and update both drivers implementing it, > > > > > > intel_pstate > > > > > > and amd-pstate, as needed. > > > > > > > > > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > > > > Hi Rafael, > > > > > > > > > Hi Srinivas, > > > > > > > > > > If you have any concerns regarding this patch, please let me > > > > > know > > > > > (note that it is based on the [05/10]). > > > > > > > > > Changes looks fine, but wants to test out some update limits from > > > > interrupt path. > > > > Checked your branches at linux-pm, not able to locate in any > > > > branch to > > > > apply. > > > > Please point me to a branch. > > > > > > I'll put it in 'testing' later today. > > > > Now available from > > > > git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git > > testing > > > Looks good. > > Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> Thanks! IIUC this applies to both [5/10] and [10/10], or please let me know if that's not the case.
--- a/drivers/cpufreq/amd-pstate.c +++ b/drivers/cpufreq/amd-pstate.c @@ -821,19 +821,16 @@ schedule_work(&sched_prefcore_work); } -static void amd_pstate_update_limits(unsigned int cpu) +static void amd_pstate_update_limits(struct cpufreq_policy *policy) { - struct cpufreq_policy *policy __free(put_cpufreq_policy) = cpufreq_cpu_get(cpu); struct amd_cpudata *cpudata; u32 prev_high = 0, cur_high = 0; bool highest_perf_changed = false; + unsigned int cpu = policy->cpu; if (!amd_pstate_prefcore) return; - if (!policy) - return; - if (amd_get_highest_perf(cpu, &cur_high)) return; --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -2741,7 +2741,7 @@ return; if (cpufreq_driver->update_limits) - cpufreq_driver->update_limits(cpu); + cpufreq_driver->update_limits(policy); else cpufreq_policy_refresh(policy); } --- a/drivers/cpufreq/intel_pstate.c +++ b/drivers/cpufreq/intel_pstate.c @@ -1353,14 +1353,9 @@ cpufreq_update_policy(cpu); } -static bool intel_pstate_update_max_freq(struct cpudata *cpudata) +static void __intel_pstate_update_max_freq(struct cpufreq_policy *policy, + struct cpudata *cpudata) { - struct cpufreq_policy *policy __free(put_cpufreq_policy); - - policy = cpufreq_cpu_get(cpudata->cpu); - if (!policy) - return false; - guard(cpufreq_policy_write)(policy); if (hwp_active) @@ -1370,16 +1365,28 @@ cpudata->pstate.max_freq : cpudata->pstate.turbo_freq; refresh_frequency_limits(policy); +} + +static bool intel_pstate_update_max_freq(struct cpudata *cpudata) +{ + struct cpufreq_policy *policy __free(put_cpufreq_policy); + + policy = cpufreq_cpu_get(cpudata->cpu); + if (!policy) + return false; + + __intel_pstate_update_max_freq(policy, cpudata); return true; } -static void intel_pstate_update_limits(unsigned int cpu) +static void intel_pstate_update_limits(struct cpufreq_policy *policy) { - struct cpudata *cpudata = all_cpu_data[cpu]; + struct cpudata *cpudata = all_cpu_data[policy->cpu]; + + __intel_pstate_update_max_freq(policy, cpudata); - if (intel_pstate_update_max_freq(cpudata)) - hybrid_update_capacity(cpudata); + hybrid_update_capacity(cpudata); } static void intel_pstate_update_limits_for_all(void) --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -399,7 +399,7 @@ unsigned int (*get)(unsigned int cpu); /* Called to update policy limits on firmware notifications. */ - void (*update_limits)(unsigned int cpu); + void (*update_limits)(struct cpufreq_policy *policy); /* optional */ int (*bios_limit)(int cpu, unsigned int *limit);