Message-ID: <CAJZ5v0i+GBpv_mGnYMbNjr2d1pvXJ_NqYCp8+pgOTNpCTuue8A@mail.gmail.com>
Date: Mon, 8 Sep 2025 20:10:53 +0200
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Zihuan Zhang <zhangzihuan@...inos.cn>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>, Linux PM <linux-pm@...r.kernel.org>, 
	Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>, LKML <linux-kernel@...r.kernel.org>, 
	Viresh Kumar <viresh.kumar@...aro.org>, Jonathan Cameron <jonathan.cameron@...wei.com>
Subject: Re: [PATCH v1 2/3] cpufreq: intel_pstate: Rearrange freq QoS updates
 using __free()

On Mon, Sep 8, 2025 at 2:53 AM Zihuan Zhang <zhangzihuan@...inos.cn> wrote:
>
>
> 在 2025/9/5 21:52, Rafael J. Wysocki 写道:
> > From: Rafael J. Wysocki <rafael.j.wysocki@...el.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@...el.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@...inos.cn>

Thanks!

Powered by blists - more mailing lists