Skip to contentSkip to conversation

cpufreq: optimize policy_is_shared()

PATCH Applied Yury Norov 4 participants 5 messages Mar 14 – Mar 18 CPU frequency scaling framework

lore
Share & tools
lore

Yury Norov

author
#0 lore raw

PATCH 1/1 cpufreq: optimize policy_is_shared()

The switch to cpumask_nth() over cpumask_weight(), as it may return earlier - as soon as the function counts the required number of CPUs.

Signed-off-by Yury Norov <[email protected]>
---
 include/linux/cpufreq.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
include/linux/cpufreq.h +1 -1
index cc894fc38971..8ca2bcb3d7ae 100644
@@ -232,7 +232,7 @@ static inline bool policy_is_inactive(struct cpufreq_policy *policy)
 
 static inline bool policy_is_shared(struct cpufreq_policy *policy)
 {
-	return cpumask_weight(policy->cpus) > 1;
+	return cpumask_nth(1, policy->cpus) < nr_cpumask_bits;
 }
 
 #ifdef CONFIG_CPU_FREQ
Signature · 1 line
2.43.0
#1 lore raw
Quoted 13 lines · Yury Norov

The switch to cpumask_nth() over cpumask_weight(), as it may return earlier - as soon as the function counts the required number of CPUs.

Signed-off-by Yury Norov <[email protected]>
---
  include/linux/cpufreq.h | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
include/linux/cpufreq.h +0 -0
index cc894fc38971..8ca2bcb3d7ae 100644

include/linux/cpufreq.h static inline bool policy_is_inactive(struct cpufreq_policy *policy) · line 238

Quoted 8 lines
include/linux/cpufreq.h +1 -1
@@ -232,7 +232,7 @@ static inline bool policy_is_inactive(struct cpufreq_policy *policy)
 
  static inline bool policy_is_shared(struct cpufreq_policy *policy)
  {
-	return cpumask_weight(policy->cpus) > 1;
+	return cpumask_nth(1, policy->cpus) < nr_cpumask_bits;
  }
 
  #ifdef CONFIG_CPU_FREQ

Hi Yury,

Thanks for the patch.

I see the intent of switching from cpumask_weight() to cpumask_nth() to allow an early exit once we know there are at least two CPUs in the mask. Functionally this looks equivalent.

That said, policy_is_shared() appears to be used mostly during governor /driver bring-up paths (e.g. sugov_start(),cpufreq_dbs_governor_start(), acpi_cpufreq_cpu_init(), etc.), rather than a hot runtime path. Given that, I'm not sure the micro-optimization is worth the readability cost.

Also, cpumask_nth()'s documented return value is ">= nr_cpu_ids if such cpu doesn't exist", so comparing against nr_cpu_ids would match the API contract more directly than nr_cpumask_bits (even if they are currently equivalent). [1][2]

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/cpumask.h#n540
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/cpumask.h#n78

Signature · 2 lines
Thx and BRs,
Zhongqiu Han
maintainer Acked-by on v1 1/1
#2 lore raw
In reply to Yury Norov
Quoted 13 lines · Yury Norov

The switch to cpumask_nth() over cpumask_weight(), as it may return earlier - as soon as the function counts the required number of CPUs.

Signed-off-by Yury Norov <[email protected]>
---
 include/linux/cpufreq.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
include/linux/cpufreq.h +0 -0
index cc894fc38971..8ca2bcb3d7ae 100644

include/linux/cpufreq.h static inline bool policy_is_inactive(struct cpufreq_policy *policy) · line 238

Quoted 8 lines
include/linux/cpufreq.h +1 -1
@@ -232,7 +232,7 @@ static inline bool policy_is_inactive(struct cpufreq_policy *policy)
 
 static inline bool policy_is_shared(struct cpufreq_policy *policy)
 {
-	return cpumask_weight(policy->cpus) > 1;
+	return cpumask_nth(1, policy->cpus) < nr_cpumask_bits;
 }
 
 #ifdef CONFIG_CPU_FREQ
Acked-by Viresh Kumar <[email protected]>
Signature · 1 line
viresh
maintainer Applied
#3 lore raw
Quoted 22 lines · Viresh Kumar
Quoted 20 lines · Yury Norov

The switch to cpumask_nth() over cpumask_weight(), as it may return earlier - as soon as the function counts the required number of CPUs.

Signed-off-by Yury Norov <[email protected]>
---
 include/linux/cpufreq.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
include/linux/cpufreq.h +1 -1
index cc894fc38971..8ca2bcb3d7ae 100644
@@ -232,7 +232,7 @@ static inline bool policy_is_inactive(struct cpufreq_policy *policy)
 
 static inline bool policy_is_shared(struct cpufreq_policy *policy)
 {
-     return cpumask_weight(policy->cpus) > 1;
+     return cpumask_nth(1, policy->cpus) < nr_cpumask_bits;
 }
Quoted 3 lines

include/linux/cpufreq.h static inline bool policy_is_inactive(struct cpufreq_policy *policy) · line 238

Quoted 1 line
include/linux/cpufreq.h +0 -0
 #ifdef CONFIG_CPU_FREQ
Acked-by Viresh Kumar <[email protected]>

Applied as 7.1 material, thanks!

Reviewed-by on v1 1/1
#4 lore raw
In reply to Yury Norov
Quoted 13 lines · Yury Norov

The switch to cpumask_nth() over cpumask_weight(), as it may return earlier - as soon as the function counts the required number of CPUs.

Signed-off-by Yury Norov <[email protected]>
---
  include/linux/cpufreq.h | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
include/linux/cpufreq.h +0 -0
index cc894fc38971..8ca2bcb3d7ae 100644

include/linux/cpufreq.h static inline bool policy_is_inactive(struct cpufreq_policy *policy) · line 238

Quoted 8 lines
include/linux/cpufreq.h +1 -1
@@ -232,7 +232,7 @@ static inline bool policy_is_inactive(struct cpufreq_policy *policy)
 
  static inline bool policy_is_shared(struct cpufreq_policy *policy)
  {
-	return cpumask_weight(policy->cpus) > 1;
+	return cpumask_nth(1, policy->cpus) < nr_cpumask_bits;
  }
 
  #ifdef CONFIG_CPU_FREQ

Thanks. While I had a minor readability/API-nit earlier, I'm fine with the patch as-is.

Reviewed-by Zhongqiu Han <[email protected]>
Signature · 2 lines
Thx and BRs,
Zhongqiu Han