[v1] cpufreq: Avoid get_governor() for first policy

[PATCH v1] cpufreq: Avoid get_governor() for first policy

Posted by Zihuan Zhang 2 months, 1 week ago

When a cpufreq driver registers the first policy, it may attempt to
initialize the policy governor from `last_governor`. However, this is
meaningless for the first policy instance, because `last_governor` is
only updated when policies are removed (e.g. during CPU offline).

The `last_governor` mechanism is intended to restore the previously
used governor across CPU hotplug events. For the very first policy,
there is no "previous governor" to restore, so calling
get_governor(last_governor) is unnecessary and potentially confusing.

This patch skips looking up `last_governor` when registering the first
policy. Instead, it directly uses the default governor after all
governors have been registered and are available.

This avoids meaningless lookups, reduces unnecessary module reference
handling, and simplifies the initial policy path.

Signed-off-by: Zihuan Zhang <zhangzihuan@kylinos.cn>
---
 drivers/cpufreq/cpufreq.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index d7426e1d8bdd..b5ebd4519eab 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1121,9 +1121,9 @@ static int cpufreq_init_policy(struct cpufreq_policy *policy)
 	int ret;
 
 	if (has_target()) {
-		/* Update policy governor to the one used before hotplug. */
-		gov = get_governor(policy->last_governor);
-		if (gov) {
+		if (policy->last_governor[0] != '\0') {
+			/* Update policy governor to the one used before hotplug. */
+			gov = get_governor(policy->last_governor);
 			pr_debug("Restoring governor %s for cpu %d\n",
 				 gov->name, policy->cpu);
 		} else {
-- 
2.25.1

Re: [PATCH v1] cpufreq: Avoid get_governor() for first policy

Posted by zhenglifeng (A) 2 months, 1 week ago

On 2025/7/24 17:36, Zihuan Zhang wrote:

> When a cpufreq driver registers the first policy, it may attempt to
> initialize the policy governor from `last_governor`. However, this is
> meaningless for the first policy instance, because `last_governor` is
> only updated when policies are removed (e.g. during CPU offline).
> 
> The `last_governor` mechanism is intended to restore the previously
> used governor across CPU hotplug events. For the very first policy,
> there is no "previous governor" to restore, so calling
> get_governor(last_governor) is unnecessary and potentially confusing.
> 
> This patch skips looking up `last_governor` when registering the first
> policy. Instead, it directly uses the default governor after all
> governors have been registered and are available.
> 
> This avoids meaningless lookups, reduces unnecessary module reference
> handling, and simplifies the initial policy path.
> 
> Signed-off-by: Zihuan Zhang <zhangzihuan@kylinos.cn>
> ---
>  drivers/cpufreq/cpufreq.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index d7426e1d8bdd..b5ebd4519eab 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1121,9 +1121,9 @@ static int cpufreq_init_policy(struct cpufreq_policy *policy)
>  	int ret;
>  
>  	if (has_target()) {
> -		/* Update policy governor to the one used before hotplug. */
> -		gov = get_governor(policy->last_governor);
> -		if (gov) {
> +		if (policy->last_governor[0] != '\0') {
> +			/* Update policy governor to the one used before hotplug. */
> +			gov = get_governor(policy->last_governor);

What if gov is null here? For example, the last governor has been removed.
The default governor should be used in this situation I think.

>  			pr_debug("Restoring governor %s for cpu %d\n",
>  				 gov->name, policy->cpu);
>  		} else {

Re: [PATCH v1] cpufreq: Avoid get_governor() for first policy

Posted by Zihuan Zhang 2 months, 1 week ago

Hi,

在 2025/7/24 19:28, zhenglifeng (A) 写道:
> On 2025/7/24 17:36, Zihuan Zhang wrote:
>
>> When a cpufreq driver registers the first policy, it may attempt to
>> initialize the policy governor from `last_governor`. However, this is
>> meaningless for the first policy instance, because `last_governor` is
>> only updated when policies are removed (e.g. during CPU offline).
>>
>> The `last_governor` mechanism is intended to restore the previously
>> used governor across CPU hotplug events. For the very first policy,
>> there is no "previous governor" to restore, so calling
>> get_governor(last_governor) is unnecessary and potentially confusing.
>>
>> This patch skips looking up `last_governor` when registering the first
>> policy. Instead, it directly uses the default governor after all
>> governors have been registered and are available.
>>
>> This avoids meaningless lookups, reduces unnecessary module reference
>> handling, and simplifies the initial policy path.
>>
>> Signed-off-by: Zihuan Zhang <zhangzihuan@kylinos.cn>
>> ---
>>   drivers/cpufreq/cpufreq.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>> index d7426e1d8bdd..b5ebd4519eab 100644
>> --- a/drivers/cpufreq/cpufreq.c
>> +++ b/drivers/cpufreq/cpufreq.c
>> @@ -1121,9 +1121,9 @@ static int cpufreq_init_policy(struct cpufreq_policy *policy)
>>   	int ret;
>>   
>>   	if (has_target()) {
>> -		/* Update policy governor to the one used before hotplug. */
>> -		gov = get_governor(policy->last_governor);
>> -		if (gov) {
>> +		if (policy->last_governor[0] != '\0') {
>> +			/* Update policy governor to the one used before hotplug. */
>> +			gov = get_governor(policy->last_governor);
> What if gov is null here? For example, the last governor has been removed.
> The default governor should be used in this situation I think.
>
Thank you for pointing it out, I will fix it in the next version.

>>   			pr_debug("Restoring governor %s for cpu %d\n",
>>   				 gov->name, policy->cpu);
>>   		} else {