Re: [PATCH v1 2/3] cpufreq: intel_pstate: Rearrange freq QoS updates using __free()

[Zihuan Zhang]

在 2025/9/5 21:52, Rafael J. Wysocki 写道:
> From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
>
> 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@xxxxxxxxxxxxxx/
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> ---
>   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@xxxxxxxxxx>