linux-kernel - Re: [PATCH v1 2/3] cpufreq: intel_pstate: Rearrange freq QoS updates using _

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a6104009-d0ad-4d76-9079-0e81e74cd32a@kylinos.cn>
Date: Mon, 8 Sep 2025 08:53:14 +0800
From: Zihuan Zhang <zhangzihuan@...inos.cn>
To: "Rafael J. Wysocki" <rafael@...nel.org>,
 Linux PM <linux-pm@...r.kernel.org>
Cc: 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()


在 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.

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@...inos.cn>