[PATCH V2 1/2] arm64: topology: Avoid the have_policy check

[PATCH V2 1/2] arm64: topology: Avoid the have_policy check

[Viresh Kumar]

Every time I have stumbled upon this routine, I get confused with the
way 'have_policy' is used and I have to dig in to understand why is it
so. Here is an attempt to make it easier to understand, and hopefully it
is an improvement.

The 'have_policy' check was just an optimization to avoid writing
to amu_fie_cpus in case we don't have to, but that optimization itself
is creating more confusion than the real work. Lets just do that if all
the CPUs support AMUs. It is much cleaner that way.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
V2:
- Skip the have_policy check altogether
- Updated subject and log

 arch/arm64/kernel/topology.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
index f6faa697e83e..ebadc73449f9 100644
--- a/arch/arm64/kernel/topology.c
+++ b/arch/arm64/kernel/topology.c
@@ -199,14 +199,14 @@ static int freq_inv_set_max_ratio(int cpu, u64 max_rate, u64 ref_rate)
 	return 0;
 }
 
-static inline bool
+static inline void
 enable_policy_freq_counters(int cpu, cpumask_var_t valid_cpus)
 {
 	struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
 
 	if (!policy) {
 		pr_debug("CPU%d: No cpufreq policy found.\n", cpu);
-		return false;
+		return;
 	}
 
 	if (cpumask_subset(policy->related_cpus, valid_cpus))
@@ -214,8 +214,6 @@ enable_policy_freq_counters(int cpu, cpumask_var_t valid_cpus)
 			   amu_fie_cpus);
 
 	cpufreq_cpu_put(policy);
-
-	return true;
 }
 
 static DEFINE_STATIC_KEY_FALSE(amu_fie_key);
@@ -225,7 +223,6 @@ static int __init init_amu_fie(void)
 {
 	bool invariance_status = topology_scale_freq_invariant();
 	cpumask_var_t valid_cpus;
-	bool have_policy = false;
 	int ret = 0;
 	int cpu;
 
@@ -245,17 +242,12 @@ static int __init init_amu_fie(void)
 			continue;
 
 		cpumask_set_cpu(cpu, valid_cpus);
-		have_policy |= enable_policy_freq_counters(cpu, valid_cpus);
+		enable_policy_freq_counters(cpu, valid_cpus);
 	}
 
-	/*
-	 * If we are not restricted by cpufreq policies, we only enable
-	 * the use of the AMU feature for FIE if all CPUs support AMU.
-	 * Otherwise, enable_policy_freq_counters has already enabled
-	 * policy cpus.
-	 */
-	if (!have_policy && cpumask_equal(valid_cpus, cpu_present_mask))
-		cpumask_or(amu_fie_cpus, amu_fie_cpus, valid_cpus);
+	/* Overwrite amu_fie_cpus if all CPUs support AMU */
+	if (cpumask_equal(valid_cpus, cpu_present_mask))
+		cpumask_copy(amu_fie_cpus, cpu_present_mask);
 
 	if (!cpumask_empty(amu_fie_cpus)) {
 		pr_info("CPUs[%*pbl]: counters will be used for FIE.",
-- 
2.25.0.rc1.19.g042ed3e048af

[PATCH V2 1/2] arm64: topology: Avoid the have_policy check

[Viresh Kumar]

Every time I have stumbled upon this routine, I get confused with the
way 'have_policy' is used and I have to dig in to understand why is it
so. Here is an attempt to make it easier to understand, and hopefully it
is an improvement.

The 'have_policy' check was just an optimization to avoid writing
to amu_fie_cpus in case we don't have to, but that optimization itself
is creating more confusion than the real work. Lets just do that if all
the CPUs support AMUs. It is much cleaner that way.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
V2:
- Skip the have_policy check altogether
- Updated subject and log

 arch/arm64/kernel/topology.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
index f6faa697e83e..ebadc73449f9 100644
--- a/arch/arm64/kernel/topology.c
+++ b/arch/arm64/kernel/topology.c
@@ -199,14 +199,14 @@ static int freq_inv_set_max_ratio(int cpu, u64 max_rate, u64 ref_rate)
 	return 0;
 }
 
-static inline bool
+static inline void
 enable_policy_freq_counters(int cpu, cpumask_var_t valid_cpus)
 {
 	struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
 
 	if (!policy) {
 		pr_debug("CPU%d: No cpufreq policy found.\n", cpu);
-		return false;
+		return;
 	}
 
 	if (cpumask_subset(policy->related_cpus, valid_cpus))
@@ -214,8 +214,6 @@ enable_policy_freq_counters(int cpu, cpumask_var_t valid_cpus)
 			   amu_fie_cpus);
 
 	cpufreq_cpu_put(policy);
-
-	return true;
 }
 
 static DEFINE_STATIC_KEY_FALSE(amu_fie_key);
@@ -225,7 +223,6 @@ static int __init init_amu_fie(void)
 {
 	bool invariance_status = topology_scale_freq_invariant();
 	cpumask_var_t valid_cpus;
-	bool have_policy = false;
 	int ret = 0;
 	int cpu;
 
@@ -245,17 +242,12 @@ static int __init init_amu_fie(void)
 			continue;
 
 		cpumask_set_cpu(cpu, valid_cpus);
-		have_policy |= enable_policy_freq_counters(cpu, valid_cpus);
+		enable_policy_freq_counters(cpu, valid_cpus);
 	}
 
-	/*
-	 * If we are not restricted by cpufreq policies, we only enable
-	 * the use of the AMU feature for FIE if all CPUs support AMU.
-	 * Otherwise, enable_policy_freq_counters has already enabled
-	 * policy cpus.
-	 */
-	if (!have_policy && cpumask_equal(valid_cpus, cpu_present_mask))
-		cpumask_or(amu_fie_cpus, amu_fie_cpus, valid_cpus);
+	/* Overwrite amu_fie_cpus if all CPUs support AMU */
+	if (cpumask_equal(valid_cpus, cpu_present_mask))
+		cpumask_copy(amu_fie_cpus, cpu_present_mask);
 
 	if (!cpumask_empty(amu_fie_cpus)) {
 		pr_info("CPUs[%*pbl]: counters will be used for FIE.",
-- 
2.25.0.rc1.19.g042ed3e048af


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH V2 2/2] arm64: topology: Reorder init_amu_fie() a bit

[Viresh Kumar]

This patch does a couple of optimizations in init_amu_fie(), like early
exits from paths where we don't need to continue any further, moving the
calls to topology_scale_freq_invariant() just when we need
them, instead of at the top of the routine, and avoiding calling it for
the third time.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
V2:
- The enable/disable dance is actually required, just made a bunch of
  other optimizations to make it look better.

 arch/arm64/kernel/topology.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
index ebadc73449f9..1ebdb667f0d1 100644
--- a/arch/arm64/kernel/topology.c
+++ b/arch/arm64/kernel/topology.c
@@ -221,7 +221,7 @@ static DEFINE_STATIC_KEY_FALSE(amu_fie_key);
 
 static int __init init_amu_fie(void)
 {
-	bool invariance_status = topology_scale_freq_invariant();
+	bool prev, now;
 	cpumask_var_t valid_cpus;
 	int ret = 0;
 	int cpu;
@@ -249,18 +249,24 @@ static int __init init_amu_fie(void)
 	if (cpumask_equal(valid_cpus, cpu_present_mask))
 		cpumask_copy(amu_fie_cpus, cpu_present_mask);
 
-	if (!cpumask_empty(amu_fie_cpus)) {
-		pr_info("CPUs[%*pbl]: counters will be used for FIE.",
-			cpumask_pr_args(amu_fie_cpus));
-		static_branch_enable(&amu_fie_key);
-	}
+	if (cpumask_empty(amu_fie_cpus))
+		goto free_valid_mask;
+
+	prev = topology_scale_freq_invariant();
+	static_branch_enable(&amu_fie_key);
+	now = topology_scale_freq_invariant();
 
 	/*
 	 * If the system is not fully invariant after AMU init, disable
 	 * partial use of counters for frequency invariance.
 	 */
-	if (!topology_scale_freq_invariant())
+	if (!now) {
 		static_branch_disable(&amu_fie_key);
+		goto free_valid_mask;
+	}
+
+	pr_info("CPUs[%*pbl]: counters will be used for FIE.",
+		cpumask_pr_args(amu_fie_cpus));
 
 	/*
 	 * Task scheduler behavior depends on frequency invariance support,
@@ -268,7 +274,7 @@ static int __init init_amu_fie(void)
 	 * a result of counter initialisation and use, retrigger the build of
 	 * scheduling domains to ensure the information is propagated properly.
 	 */
-	if (invariance_status != topology_scale_freq_invariant())
+	if (prev != now)
 		rebuild_sched_domains_energy();
 
 free_valid_mask:
-- 
2.25.0.rc1.19.g042ed3e048af

[PATCH V2 2/2] arm64: topology: Reorder init_amu_fie() a bit

[Viresh Kumar]

This patch does a couple of optimizations in init_amu_fie(), like early
exits from paths where we don't need to continue any further, moving the
calls to topology_scale_freq_invariant() just when we need
them, instead of at the top of the routine, and avoiding calling it for
the third time.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
V2:
- The enable/disable dance is actually required, just made a bunch of
  other optimizations to make it look better.

 arch/arm64/kernel/topology.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
index ebadc73449f9..1ebdb667f0d1 100644
--- a/arch/arm64/kernel/topology.c
+++ b/arch/arm64/kernel/topology.c
@@ -221,7 +221,7 @@ static DEFINE_STATIC_KEY_FALSE(amu_fie_key);
 
 static int __init init_amu_fie(void)
 {
-	bool invariance_status = topology_scale_freq_invariant();
+	bool prev, now;
 	cpumask_var_t valid_cpus;
 	int ret = 0;
 	int cpu;
@@ -249,18 +249,24 @@ static int __init init_amu_fie(void)
 	if (cpumask_equal(valid_cpus, cpu_present_mask))
 		cpumask_copy(amu_fie_cpus, cpu_present_mask);
 
-	if (!cpumask_empty(amu_fie_cpus)) {
-		pr_info("CPUs[%*pbl]: counters will be used for FIE.",
-			cpumask_pr_args(amu_fie_cpus));
-		static_branch_enable(&amu_fie_key);
-	}
+	if (cpumask_empty(amu_fie_cpus))
+		goto free_valid_mask;
+
+	prev = topology_scale_freq_invariant();
+	static_branch_enable(&amu_fie_key);
+	now = topology_scale_freq_invariant();
 
 	/*
 	 * If the system is not fully invariant after AMU init, disable
 	 * partial use of counters for frequency invariance.
 	 */
-	if (!topology_scale_freq_invariant())
+	if (!now) {
 		static_branch_disable(&amu_fie_key);
+		goto free_valid_mask;
+	}
+
+	pr_info("CPUs[%*pbl]: counters will be used for FIE.",
+		cpumask_pr_args(amu_fie_cpus));
 
 	/*
 	 * Task scheduler behavior depends on frequency invariance support,
@@ -268,7 +274,7 @@ static int __init init_amu_fie(void)
 	 * a result of counter initialisation and use, retrigger the build of
 	 * scheduling domains to ensure the information is propagated properly.
 	 */
-	if (invariance_status != topology_scale_freq_invariant())
+	if (prev != now)
 		rebuild_sched_domains_energy();
 
 free_valid_mask:
-- 
2.25.0.rc1.19.g042ed3e048af


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2 1/2] arm64: topology: Avoid the have_policy check

[Ionela Voinescu]

On Thursday 10 Dec 2020 at 21:59:22 (+0530), Viresh Kumar wrote:
> Every time I have stumbled upon this routine, I get confused with the
> way 'have_policy' is used and I have to dig in to understand why is it
> so. Here is an attempt to make it easier to understand, and hopefully it
> is an improvement.
> 
> The 'have_policy' check was just an optimization to avoid writing
> to amu_fie_cpus in case we don't have to, but that optimization itself
> is creating more confusion than the real work. Lets just do that if all
> the CPUs support AMUs. It is much cleaner that way.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> V2:
> - Skip the have_policy check altogether
> - Updated subject and log
> 
>  arch/arm64/kernel/topology.c | 20 ++++++--------------
>  1 file changed, 6 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
> index f6faa697e83e..ebadc73449f9 100644
> --- a/arch/arm64/kernel/topology.c
> +++ b/arch/arm64/kernel/topology.c
> @@ -199,14 +199,14 @@ static int freq_inv_set_max_ratio(int cpu, u64 max_rate, u64 ref_rate)
>  	return 0;
>  }
>  
> -static inline bool
> +static inline void
>  enable_policy_freq_counters(int cpu, cpumask_var_t valid_cpus)
>  {
>  	struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
>  
>  	if (!policy) {
>  		pr_debug("CPU%d: No cpufreq policy found.\n", cpu);
> -		return false;
> +		return;
>  	}
>  
>  	if (cpumask_subset(policy->related_cpus, valid_cpus))
> @@ -214,8 +214,6 @@ enable_policy_freq_counters(int cpu, cpumask_var_t valid_cpus)
>  			   amu_fie_cpus);
>  
>  	cpufreq_cpu_put(policy);
> -
> -	return true;
>  }
>  
>  static DEFINE_STATIC_KEY_FALSE(amu_fie_key);
> @@ -225,7 +223,6 @@ static int __init init_amu_fie(void)
>  {
>  	bool invariance_status = topology_scale_freq_invariant();
>  	cpumask_var_t valid_cpus;
> -	bool have_policy = false;
>  	int ret = 0;
>  	int cpu;
>  
> @@ -245,17 +242,12 @@ static int __init init_amu_fie(void)
>  			continue;
>  
>  		cpumask_set_cpu(cpu, valid_cpus);
> -		have_policy |= enable_policy_freq_counters(cpu, valid_cpus);
> +		enable_policy_freq_counters(cpu, valid_cpus);
>  	}
>  
> -	/*
> -	 * If we are not restricted by cpufreq policies, we only enable
> -	 * the use of the AMU feature for FIE if all CPUs support AMU.
> -	 * Otherwise, enable_policy_freq_counters has already enabled
> -	 * policy cpus.
> -	 */
> -	if (!have_policy && cpumask_equal(valid_cpus, cpu_present_mask))
> -		cpumask_or(amu_fie_cpus, amu_fie_cpus, valid_cpus);
> +	/* Overwrite amu_fie_cpus if all CPUs support AMU */
> +	if (cpumask_equal(valid_cpus, cpu_present_mask))
> +		cpumask_copy(amu_fie_cpus, cpu_present_mask);
>  
>  	if (!cpumask_empty(amu_fie_cpus)) {
>  		pr_info("CPUs[%*pbl]: counters will be used for FIE.",
> -- 
> 2.25.0.rc1.19.g042ed3e048af
> 

Reviewed-by: Ionela Voinescu <ionela.voinescu@arm.com>

Thanks,
Ionela.

Re: [PATCH V2 1/2] arm64: topology: Avoid the have_policy check

[Ionela Voinescu]

On Thursday 10 Dec 2020 at 21:59:22 (+0530), Viresh Kumar wrote:
> Every time I have stumbled upon this routine, I get confused with the
> way 'have_policy' is used and I have to dig in to understand why is it
> so. Here is an attempt to make it easier to understand, and hopefully it
> is an improvement.
> 
> The 'have_policy' check was just an optimization to avoid writing
> to amu_fie_cpus in case we don't have to, but that optimization itself
> is creating more confusion than the real work. Lets just do that if all
> the CPUs support AMUs. It is much cleaner that way.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> V2:
> - Skip the have_policy check altogether
> - Updated subject and log
> 
>  arch/arm64/kernel/topology.c | 20 ++++++--------------
>  1 file changed, 6 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
> index f6faa697e83e..ebadc73449f9 100644
> --- a/arch/arm64/kernel/topology.c
> +++ b/arch/arm64/kernel/topology.c
> @@ -199,14 +199,14 @@ static int freq_inv_set_max_ratio(int cpu, u64 max_rate, u64 ref_rate)
>  	return 0;
>  }
>  
> -static inline bool
> +static inline void
>  enable_policy_freq_counters(int cpu, cpumask_var_t valid_cpus)
>  {
>  	struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
>  
>  	if (!policy) {
>  		pr_debug("CPU%d: No cpufreq policy found.\n", cpu);
> -		return false;
> +		return;
>  	}
>  
>  	if (cpumask_subset(policy->related_cpus, valid_cpus))
> @@ -214,8 +214,6 @@ enable_policy_freq_counters(int cpu, cpumask_var_t valid_cpus)
>  			   amu_fie_cpus);
>  
>  	cpufreq_cpu_put(policy);
> -
> -	return true;
>  }
>  
>  static DEFINE_STATIC_KEY_FALSE(amu_fie_key);
> @@ -225,7 +223,6 @@ static int __init init_amu_fie(void)
>  {
>  	bool invariance_status = topology_scale_freq_invariant();
>  	cpumask_var_t valid_cpus;
> -	bool have_policy = false;
>  	int ret = 0;
>  	int cpu;
>  
> @@ -245,17 +242,12 @@ static int __init init_amu_fie(void)
>  			continue;
>  
>  		cpumask_set_cpu(cpu, valid_cpus);
> -		have_policy |= enable_policy_freq_counters(cpu, valid_cpus);
> +		enable_policy_freq_counters(cpu, valid_cpus);
>  	}
>  
> -	/*
> -	 * If we are not restricted by cpufreq policies, we only enable
> -	 * the use of the AMU feature for FIE if all CPUs support AMU.
> -	 * Otherwise, enable_policy_freq_counters has already enabled
> -	 * policy cpus.
> -	 */
> -	if (!have_policy && cpumask_equal(valid_cpus, cpu_present_mask))
> -		cpumask_or(amu_fie_cpus, amu_fie_cpus, valid_cpus);
> +	/* Overwrite amu_fie_cpus if all CPUs support AMU */
> +	if (cpumask_equal(valid_cpus, cpu_present_mask))
> +		cpumask_copy(amu_fie_cpus, cpu_present_mask);
>  
>  	if (!cpumask_empty(amu_fie_cpus)) {
>  		pr_info("CPUs[%*pbl]: counters will be used for FIE.",
> -- 
> 2.25.0.rc1.19.g042ed3e048af
> 

Reviewed-by: Ionela Voinescu <ionela.voinescu@arm.com>

Thanks,
Ionela.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2 2/2] arm64: topology: Reorder init_amu_fie() a bit

[Ionela Voinescu]

Hey,

On Thursday 10 Dec 2020 at 21:59:23 (+0530), Viresh Kumar wrote:
> This patch does a couple of optimizations in init_amu_fie(), like early
> exits from paths where we don't need to continue any further, moving the
> calls to topology_scale_freq_invariant() just when we need
> them, instead of at the top of the routine, and avoiding calling it for
> the third time.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> V2:
> - The enable/disable dance is actually required, just made a bunch of
>   other optimizations to make it look better.
> 
>  arch/arm64/kernel/topology.c | 22 ++++++++++++++--------
>  1 file changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
> index ebadc73449f9..1ebdb667f0d1 100644
> --- a/arch/arm64/kernel/topology.c
> +++ b/arch/arm64/kernel/topology.c
> @@ -221,7 +221,7 @@ static DEFINE_STATIC_KEY_FALSE(amu_fie_key);
>  
>  static int __init init_amu_fie(void)
>  {
> -	bool invariance_status = topology_scale_freq_invariant();
> +	bool prev, now;

Nit: can you move this below valid_cpus? It makes the section nicer if
they are in decreasing order of line length.

>  	cpumask_var_t valid_cpus;
>  	int ret = 0;
>  	int cpu;
> @@ -249,18 +249,24 @@ static int __init init_amu_fie(void)
>  	if (cpumask_equal(valid_cpus, cpu_present_mask))
>  		cpumask_copy(amu_fie_cpus, cpu_present_mask);
>  
> -	if (!cpumask_empty(amu_fie_cpus)) {
> -		pr_info("CPUs[%*pbl]: counters will be used for FIE.",
> -			cpumask_pr_args(amu_fie_cpus));
> -		static_branch_enable(&amu_fie_key);
> -	}
> +	if (cpumask_empty(amu_fie_cpus))
> +		goto free_valid_mask;
> +
> +	prev = topology_scale_freq_invariant();
> +	static_branch_enable(&amu_fie_key);

I think there could be a potential problem here (it would be unlikely
but why not fix it :) ). It was in the code before your changes.

When we enable amu_fie_key here, topology_scale_freq_tick() could be
called for AMU CPUs, which will compute and set a scale factor. Later
on, if we happen to find the system not invariant, we disable counter
based invariance, but a scale factor might have been set already for a
CPU, which would and should have returned 1024 otherwise (the
initialisation value of freq_scale).


Therefore, while here, you could instead do the following:

cpufreq_inv = cpufreq_supports_freq_invariance();

if (!cpufreq_inv &&
    !cpumask_subset(cpu_online_mask, amu_fie_cpus))
    goto free_valid_mask;

static_branch_enable(&amu_fie_key);

pr_info(..);

if (!cpufreq_inv)
    rebuild_sched_domains_energy();

What do you think?

I can submit this separately, if you don't want the hassle.

Thanks,
Ionela.


> +	now = topology_scale_freq_invariant();
>  
>  	/*
>  	 * If the system is not fully invariant after AMU init, disable
>  	 * partial use of counters for frequency invariance.
>  	 */
> -	if (!topology_scale_freq_invariant())
> +	if (!now) {
>  		static_branch_disable(&amu_fie_key);
> +		goto free_valid_mask;
> +	}
> +
> +	pr_info("CPUs[%*pbl]: counters will be used for FIE.",
> +		cpumask_pr_args(amu_fie_cpus));
>  
>  	/*
>  	 * Task scheduler behavior depends on frequency invariance support,
> @@ -268,7 +274,7 @@ static int __init init_amu_fie(void)
>  	 * a result of counter initialisation and use, retrigger the build of
>  	 * scheduling domains to ensure the information is propagated properly.
>  	 */
> -	if (invariance_status != topology_scale_freq_invariant())
> +	if (prev != now)
>  		rebuild_sched_domains_energy();
>  
>  free_valid_mask:
> -- 
> 2.25.0.rc1.19.g042ed3e048af
> 

Re: [PATCH V2 2/2] arm64: topology: Reorder init_amu_fie() a bit

[Ionela Voinescu]

Hey,

On Thursday 10 Dec 2020 at 21:59:23 (+0530), Viresh Kumar wrote:
> This patch does a couple of optimizations in init_amu_fie(), like early
> exits from paths where we don't need to continue any further, moving the
> calls to topology_scale_freq_invariant() just when we need
> them, instead of at the top of the routine, and avoiding calling it for
> the third time.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> V2:
> - The enable/disable dance is actually required, just made a bunch of
>   other optimizations to make it look better.
> 
>  arch/arm64/kernel/topology.c | 22 ++++++++++++++--------
>  1 file changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
> index ebadc73449f9..1ebdb667f0d1 100644
> --- a/arch/arm64/kernel/topology.c
> +++ b/arch/arm64/kernel/topology.c
> @@ -221,7 +221,7 @@ static DEFINE_STATIC_KEY_FALSE(amu_fie_key);
>  
>  static int __init init_amu_fie(void)
>  {
> -	bool invariance_status = topology_scale_freq_invariant();
> +	bool prev, now;

Nit: can you move this below valid_cpus? It makes the section nicer if
they are in decreasing order of line length.

>  	cpumask_var_t valid_cpus;
>  	int ret = 0;
>  	int cpu;
> @@ -249,18 +249,24 @@ static int __init init_amu_fie(void)
>  	if (cpumask_equal(valid_cpus, cpu_present_mask))
>  		cpumask_copy(amu_fie_cpus, cpu_present_mask);
>  
> -	if (!cpumask_empty(amu_fie_cpus)) {
> -		pr_info("CPUs[%*pbl]: counters will be used for FIE.",
> -			cpumask_pr_args(amu_fie_cpus));
> -		static_branch_enable(&amu_fie_key);
> -	}
> +	if (cpumask_empty(amu_fie_cpus))
> +		goto free_valid_mask;
> +
> +	prev = topology_scale_freq_invariant();
> +	static_branch_enable(&amu_fie_key);

I think there could be a potential problem here (it would be unlikely
but why not fix it :) ). It was in the code before your changes.

When we enable amu_fie_key here, topology_scale_freq_tick() could be
called for AMU CPUs, which will compute and set a scale factor. Later
on, if we happen to find the system not invariant, we disable counter
based invariance, but a scale factor might have been set already for a
CPU, which would and should have returned 1024 otherwise (the
initialisation value of freq_scale).


Therefore, while here, you could instead do the following:

cpufreq_inv = cpufreq_supports_freq_invariance();

if (!cpufreq_inv &&
    !cpumask_subset(cpu_online_mask, amu_fie_cpus))
    goto free_valid_mask;

static_branch_enable(&amu_fie_key);

pr_info(..);

if (!cpufreq_inv)
    rebuild_sched_domains_energy();

What do you think?

I can submit this separately, if you don't want the hassle.

Thanks,
Ionela.


> +	now = topology_scale_freq_invariant();
>  
>  	/*
>  	 * If the system is not fully invariant after AMU init, disable
>  	 * partial use of counters for frequency invariance.
>  	 */
> -	if (!topology_scale_freq_invariant())
> +	if (!now) {
>  		static_branch_disable(&amu_fie_key);
> +		goto free_valid_mask;
> +	}
> +
> +	pr_info("CPUs[%*pbl]: counters will be used for FIE.",
> +		cpumask_pr_args(amu_fie_cpus));
>  
>  	/*
>  	 * Task scheduler behavior depends on frequency invariance support,
> @@ -268,7 +274,7 @@ static int __init init_amu_fie(void)
>  	 * a result of counter initialisation and use, retrigger the build of
>  	 * scheduling domains to ensure the information is propagated properly.
>  	 */
> -	if (invariance_status != topology_scale_freq_invariant())
> +	if (prev != now)
>  		rebuild_sched_domains_energy();
>  
>  free_valid_mask:
> -- 
> 2.25.0.rc1.19.g042ed3e048af
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2 2/2] arm64: topology: Reorder init_amu_fie() a bit

[Viresh Kumar]

On 14-12-20, 14:00, Ionela Voinescu wrote:
> I think there could be a potential problem here (it would be unlikely
> but why not fix it :) ). It was in the code before your changes.
> 
> When we enable amu_fie_key here, topology_scale_freq_tick() could be
> called for AMU CPUs, which will compute and set a scale factor. Later
> on, if we happen to find the system not invariant, we disable counter
> based invariance, but a scale factor might have been set already for a
> CPU, which would and should have returned 1024 otherwise (the
> initialisation value of freq_scale).
> 
> 
> Therefore, while here, you could instead do the following:
> 
> cpufreq_inv = cpufreq_supports_freq_invariance();
> 
> if (!cpufreq_inv &&
>     !cpumask_subset(cpu_online_mask, amu_fie_cpus))
>     goto free_valid_mask;
> 
> static_branch_enable(&amu_fie_key);
> 
> pr_info(..);
> 
> if (!cpufreq_inv)
>     rebuild_sched_domains_energy();
> 
> What do you think?

I already had a patch for this, but for a different reason, i.e. to
avoid the enable/disable dance.

       /* We aren't fully invariant yet */
       if (!prev && !cpumask_equal(amu_fie_cpus, cpu_present_mask))
               return;

And this is quite similar to what you have here.

-- 
viresh

Re: [PATCH V2 2/2] arm64: topology: Reorder init_amu_fie() a bit

[Viresh Kumar]

On 14-12-20, 14:00, Ionela Voinescu wrote:
> I think there could be a potential problem here (it would be unlikely
> but why not fix it :) ). It was in the code before your changes.
> 
> When we enable amu_fie_key here, topology_scale_freq_tick() could be
> called for AMU CPUs, which will compute and set a scale factor. Later
> on, if we happen to find the system not invariant, we disable counter
> based invariance, but a scale factor might have been set already for a
> CPU, which would and should have returned 1024 otherwise (the
> initialisation value of freq_scale).
> 
> 
> Therefore, while here, you could instead do the following:
> 
> cpufreq_inv = cpufreq_supports_freq_invariance();
> 
> if (!cpufreq_inv &&
>     !cpumask_subset(cpu_online_mask, amu_fie_cpus))
>     goto free_valid_mask;
> 
> static_branch_enable(&amu_fie_key);
> 
> pr_info(..);
> 
> if (!cpufreq_inv)
>     rebuild_sched_domains_energy();
> 
> What do you think?

I already had a patch for this, but for a different reason, i.e. to
avoid the enable/disable dance.

       /* We aren't fully invariant yet */
       if (!prev && !cpumask_equal(amu_fie_cpus, cpu_present_mask))
               return;

And this is quite similar to what you have here.

-- 
viresh

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel