drivers/cpufreq/intel_pstate.c | 65 ++++++++++++++++++++--------------------- 1 file changed, 32 insertions(+), 33 deletions(-)
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Move the code from the for_each_possible_cpu() loop in update_qos_request()
to a separate function and use __free() for cpufreq policy reference
counting in it to avoid having to call cpufreq_cpu_put() repeatedly (or
using goto).
While at it, rename update_qos_request() to update_qos_requests()
because it updates multiple requests in one go.
No intentional functional impact.
Link: https://lore.kernel.org/linux-pm/CAJZ5v0gN1T5woSF0tO=TbAh+2-sWzxFjWyDbB7wG2TFCOU01iQ@mail.gmail.com/
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
drivers/cpufreq/intel_pstate.c | 65 ++++++++++++++++++++---------------------
1 file changed, 32 insertions(+), 33 deletions(-)
--- a/drivers/cpufreq/intel_pstate.c
+++ b/drivers/cpufreq/intel_pstate.c
@@ -1693,43 +1693,42 @@ unlock_driver:
return count;
}
-static void update_qos_request(enum freq_qos_req_type type)
+static void update_cpu_qos_request(int cpunum, enum freq_qos_req_type type)
{
+ struct cpudata *cpu = all_cpu_data[cpunum];
struct freq_qos_request *req;
- struct cpufreq_policy *policy;
- int i;
+ unsigned int freq, perf_pct;
- for_each_possible_cpu(i) {
- struct cpudata *cpu = all_cpu_data[i];
- unsigned int freq, perf_pct;
-
- policy = cpufreq_cpu_get(i);
- if (!policy)
- continue;
-
- req = policy->driver_data;
- if (!req) {
- cpufreq_cpu_put(policy);
- continue;
- }
-
- if (hwp_active)
- intel_pstate_get_hwp_cap(cpu);
-
- if (type == FREQ_QOS_MIN) {
- perf_pct = global.min_perf_pct;
- } else {
- req++;
- perf_pct = global.max_perf_pct;
- }
+ struct cpufreq_policy *policy __free(put_cpufreq_policy) = cpufreq_cpu_get(cpunum);
+ if (!policy)
+ return;
+
+ req = policy->driver_data;
+ if (!req)
+ return;
+
+ if (hwp_active)
+ intel_pstate_get_hwp_cap(cpu);
+
+ if (type == FREQ_QOS_MIN) {
+ perf_pct = global.min_perf_pct;
+ } else {
+ req++;
+ perf_pct = global.max_perf_pct;
+ }
- freq = DIV_ROUND_UP(cpu->pstate.turbo_freq * perf_pct, 100);
+ freq = DIV_ROUND_UP(cpu->pstate.turbo_freq * perf_pct, 100);
- if (freq_qos_update_request(req, freq) < 0)
- pr_warn("Failed to update freq constraint: CPU%d\n", i);
+ if (freq_qos_update_request(req, freq) < 0)
+ pr_warn("Failed to update freq constraint: CPU%d\n", cpunum);
+}
- cpufreq_cpu_put(policy);
- }
+static void update_qos_requests(enum freq_qos_req_type type)
+{
+ int i;
+
+ for_each_possible_cpu(i)
+ update_cpu_qos_request(i, type);
}
static ssize_t store_max_perf_pct(struct kobject *a, struct kobj_attribute *b,
@@ -1758,7 +1757,7 @@ static ssize_t store_max_perf_pct(struct
if (intel_pstate_driver == &intel_pstate)
intel_pstate_update_policies();
else
- update_qos_request(FREQ_QOS_MAX);
+ update_qos_requests(FREQ_QOS_MAX);
mutex_unlock(&intel_pstate_driver_lock);
@@ -1792,7 +1791,7 @@ static ssize_t store_min_perf_pct(struct
if (intel_pstate_driver == &intel_pstate)
intel_pstate_update_policies();
else
- update_qos_request(FREQ_QOS_MIN);
+ update_qos_requests(FREQ_QOS_MIN);
mutex_unlock(&intel_pstate_driver_lock);
在 2025/9/5 21:52, Rafael J. Wysocki 写道:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Move the code from the for_each_possible_cpu() loop in update_qos_request()
> to a separate function and use __free() for cpufreq policy reference
> counting in it to avoid having to call cpufreq_cpu_put() repeatedly (or
> using goto).
>
> While at it, rename update_qos_request() to update_qos_requests()
> because it updates multiple requests in one go.
>
> No intentional functional impact.
>
> Link: https://lore.kernel.org/linux-pm/CAJZ5v0gN1T5woSF0tO=TbAh+2-sWzxFjWyDbB7wG2TFCOU01iQ@mail.gmail.com/
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> drivers/cpufreq/intel_pstate.c | 65 ++++++++++++++++++++---------------------
> 1 file changed, 32 insertions(+), 33 deletions(-)
>
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -1693,43 +1693,42 @@ unlock_driver:
> return count;
> }
>
> -static void update_qos_request(enum freq_qos_req_type type)
> +static void update_cpu_qos_request(int cpunum, enum freq_qos_req_type type)
> {
> + struct cpudata *cpu = all_cpu_data[cpunum];
Maybe the parameter could be named int cpu instead of cpunum, and the
struct cpudata * variable could be renamed to cpudata — this might read
a bit cleaner and help reduce potential confusion.
I also noticed that in this driver some places use one naming style and
others use another, so it might be worth unifying the style here.
Other than that, looks good to me.
> struct freq_qos_request *req;
> - struct cpufreq_policy *policy;
> - int i;
> + unsigned int freq, perf_pct;
>
> - for_each_possible_cpu(i) {
> - struct cpudata *cpu = all_cpu_data[i];
> - unsigned int freq, perf_pct;
> -
> - policy = cpufreq_cpu_get(i);
> - if (!policy)
> - continue;
> -
> - req = policy->driver_data;
> - if (!req) {
> - cpufreq_cpu_put(policy);
> - continue;
> - }
> -
> - if (hwp_active)
> - intel_pstate_get_hwp_cap(cpu);
> -
> - if (type == FREQ_QOS_MIN) {
> - perf_pct = global.min_perf_pct;
> - } else {
> - req++;
> - perf_pct = global.max_perf_pct;
> - }
> + struct cpufreq_policy *policy __free(put_cpufreq_policy) = cpufreq_cpu_get(cpunum);
> + if (!policy)
> + return;
> +
> + req = policy->driver_data;
> + if (!req)
> + return;
> +
> + if (hwp_active)
> + intel_pstate_get_hwp_cap(cpu);
> +
> + if (type == FREQ_QOS_MIN) {
> + perf_pct = global.min_perf_pct;
> + } else {
> + req++;
> + perf_pct = global.max_perf_pct;
> + }
>
> - freq = DIV_ROUND_UP(cpu->pstate.turbo_freq * perf_pct, 100);
> + freq = DIV_ROUND_UP(cpu->pstate.turbo_freq * perf_pct, 100);
>
> - if (freq_qos_update_request(req, freq) < 0)
> - pr_warn("Failed to update freq constraint: CPU%d\n", i);
> + if (freq_qos_update_request(req, freq) < 0)
> + pr_warn("Failed to update freq constraint: CPU%d\n", cpunum);
> +}
>
> - cpufreq_cpu_put(policy);
> - }
> +static void update_qos_requests(enum freq_qos_req_type type)
> +{
> + int i;
> +
> + for_each_possible_cpu(i)
> + update_cpu_qos_request(i, type);
> }
>
> static ssize_t store_max_perf_pct(struct kobject *a, struct kobj_attribute *b,
> @@ -1758,7 +1757,7 @@ static ssize_t store_max_perf_pct(struct
> if (intel_pstate_driver == &intel_pstate)
> intel_pstate_update_policies();
> else
> - update_qos_request(FREQ_QOS_MAX);
> + update_qos_requests(FREQ_QOS_MAX);
>
> mutex_unlock(&intel_pstate_driver_lock);
>
> @@ -1792,7 +1791,7 @@ static ssize_t store_min_perf_pct(struct
> if (intel_pstate_driver == &intel_pstate)
> intel_pstate_update_policies();
> else
> - update_qos_request(FREQ_QOS_MIN);
> + update_qos_requests(FREQ_QOS_MIN);
>
> mutex_unlock(&intel_pstate_driver_lock);
>
>
>
>
Reviewed-by: Zihuan Zhang <zhangzihuan@kylinos.cn>
On Mon, Sep 8, 2025 at 2:53 AM Zihuan Zhang <zhangzihuan@kylinos.cn> wrote:
>
>
> 在 2025/9/5 21:52, Rafael J. Wysocki 写道:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Move the code from the for_each_possible_cpu() loop in update_qos_request()
> > to a separate function and use __free() for cpufreq policy reference
> > counting in it to avoid having to call cpufreq_cpu_put() repeatedly (or
> > using goto).
> >
> > While at it, rename update_qos_request() to update_qos_requests()
> > because it updates multiple requests in one go.
> >
> > No intentional functional impact.
> >
> > Link: https://lore.kernel.org/linux-pm/CAJZ5v0gN1T5woSF0tO=TbAh+2-sWzxFjWyDbB7wG2TFCOU01iQ@mail.gmail.com/
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> > drivers/cpufreq/intel_pstate.c | 65 ++++++++++++++++++++---------------------
> > 1 file changed, 32 insertions(+), 33 deletions(-)
> >
> > --- a/drivers/cpufreq/intel_pstate.c
> > +++ b/drivers/cpufreq/intel_pstate.c
> > @@ -1693,43 +1693,42 @@ unlock_driver:
> > return count;
> > }
> >
> > -static void update_qos_request(enum freq_qos_req_type type)
> > +static void update_cpu_qos_request(int cpunum, enum freq_qos_req_type type)
> > {
> > + struct cpudata *cpu = all_cpu_data[cpunum];
>
>
> Maybe the parameter could be named int cpu instead of cpunum, and the
> struct cpudata * variable could be renamed to cpudata — this might read
> a bit cleaner and help reduce potential confusion.
Yeah, you are right that it could be done this way.
I kind of wanted the new code to be possibly similar to the new one,
but since this is a new function anyway, I guess this doesn't matter.
I'll change the naming when applying the patch.
> I also noticed that in this driver some places use one naming style and
> others use another, so it might be worth unifying the style here.
Well, I'm not sure about this.
I guess it may be confusing sometimes, but then I'm not sure if that
justifies the code churn that would result from changing it.
> Other than that, looks good to me.
>
> > struct freq_qos_request *req;
> > - struct cpufreq_policy *policy;
> > - int i;
> > + unsigned int freq, perf_pct;
> >
> > - for_each_possible_cpu(i) {
> > - struct cpudata *cpu = all_cpu_data[i];
> > - unsigned int freq, perf_pct;
> > -
> > - policy = cpufreq_cpu_get(i);
> > - if (!policy)
> > - continue;
> > -
> > - req = policy->driver_data;
> > - if (!req) {
> > - cpufreq_cpu_put(policy);
> > - continue;
> > - }
> > -
> > - if (hwp_active)
> > - intel_pstate_get_hwp_cap(cpu);
> > -
> > - if (type == FREQ_QOS_MIN) {
> > - perf_pct = global.min_perf_pct;
> > - } else {
> > - req++;
> > - perf_pct = global.max_perf_pct;
> > - }
> > + struct cpufreq_policy *policy __free(put_cpufreq_policy) = cpufreq_cpu_get(cpunum);
> > + if (!policy)
> > + return;
> > +
> > + req = policy->driver_data;
> > + if (!req)
> > + return;
> > +
> > + if (hwp_active)
> > + intel_pstate_get_hwp_cap(cpu);
> > +
> > + if (type == FREQ_QOS_MIN) {
> > + perf_pct = global.min_perf_pct;
> > + } else {
> > + req++;
> > + perf_pct = global.max_perf_pct;
> > + }
> >
> > - freq = DIV_ROUND_UP(cpu->pstate.turbo_freq * perf_pct, 100);
> > + freq = DIV_ROUND_UP(cpu->pstate.turbo_freq * perf_pct, 100);
> >
> > - if (freq_qos_update_request(req, freq) < 0)
> > - pr_warn("Failed to update freq constraint: CPU%d\n", i);
> > + if (freq_qos_update_request(req, freq) < 0)
> > + pr_warn("Failed to update freq constraint: CPU%d\n", cpunum);
> > +}
> >
> > - cpufreq_cpu_put(policy);
> > - }
> > +static void update_qos_requests(enum freq_qos_req_type type)
> > +{
> > + int i;
> > +
> > + for_each_possible_cpu(i)
> > + update_cpu_qos_request(i, type);
> > }
> >
> > static ssize_t store_max_perf_pct(struct kobject *a, struct kobj_attribute *b,
> > @@ -1758,7 +1757,7 @@ static ssize_t store_max_perf_pct(struct
> > if (intel_pstate_driver == &intel_pstate)
> > intel_pstate_update_policies();
> > else
> > - update_qos_request(FREQ_QOS_MAX);
> > + update_qos_requests(FREQ_QOS_MAX);
> >
> > mutex_unlock(&intel_pstate_driver_lock);
> >
> > @@ -1792,7 +1791,7 @@ static ssize_t store_min_perf_pct(struct
> > if (intel_pstate_driver == &intel_pstate)
> > intel_pstate_update_policies();
> > else
> > - update_qos_request(FREQ_QOS_MIN);
> > + update_qos_requests(FREQ_QOS_MIN);
> >
> > mutex_unlock(&intel_pstate_driver_lock);
> >
> >
> >
> >
> Reviewed-by: Zihuan Zhang <zhangzihuan@kylinos.cn>
Thanks!
在 2025/9/9 02:10, Rafael J. Wysocki 写道: >> I also noticed that in this driver some places use one naming style and >> others use another, so it might be worth unifying the style here. > Well, I'm not sure about this. > > I guess it may be confusing sometimes, but then I'm not sure if that > justifies the code churn that would result from changing it. Agreed — there are quite a lot of places that would need to be changed, and perhaps it’s better to keep the current style as is.
© 2016 - 2025 Red Hat, Inc.