[PATCH 0/2]cpufreq,topology,arm: disable FI for BL_SWITCHER

[PATCH 0/2]cpufreq,topology,arm: disable FI for BL_SWITCHER

[Ionela Voinescu]

This series is the result of the discussions ([1], [2]) around the
complications that the BL_SWITCHER poses when it comes to Frequency
Invariance (FI) and it aims to restart the discussions.

To properly scale its per-entity load-tracking signals, the task
scheduler needs to be given a frequency scale factor, i.e. some image of
the current frequency the CPU is running at, relative to its maximum
frequency.

But (reiterating the message in the changelog of patch 2/2), big.LITTLE
switching complicates the setting of a correct cpufreq-based frequency
invariance scale factor due to (as observed in
drivers/cpufreq/vexpress-spc-cpufreq.c):
 - Incorrect current and maximum frequencies as a result of the
   exposure of a virtual frequency table to the cpufreq core,
 - Missed updates as a result of asynchronous frequency adjustments
   caused by frequency changes in other CPU pairs.
More information on this feature can be found at [3].

Given that its functionality is atypical in regards to FI and that this
is an old technology, patch 2/2 disable FI for when big.LITTLE switching
is configured in to prevent incorrect scale setting.

For this purpose patch 1/2 changes the way arch_set_freq_scale() is
defined in architecture code which brings it in line with the logic of
other architectural function definitions while allowing for less invasive
filtering of FI support.

In the discussions at [2], three possible solutions were suggested:
 - (1) conditioning FI by !CONFIG_BL_SWITCHER
 - (2) leave as is with note in driver specifying this FI broken
   functionality
 - (3) removing full BL_SWITCHER support

This series restructures the solution at (1). The reason for it is that
the new patch limits the ifdef filtering to the arm topology include file,
a location where frequency invariance functions are defined. Therefore,
this seems more appropriate given that the b.L switcher is an arm
technology and that the new FI filtering location seems more natural for
conditioned FI disabling.

Solutions (2) and (3) were not implemented given that there might be some
remaining users of this technology (Samsung Chromebook 2 - Samsung Exynos
5 Octa 5420, Samsung Exynos 5 Octa 5800) and therefore leaving this
broken (2) seems equally bad to removing support for it (3).

[1] https://lore.kernel.org/lkml/20200701090751.7543-5-ionela.voinescu@arm.com/
[2] https://lore.kernel.org/lkml/20200722093732.14297-4-ionela.voinescu@arm.com/
[3] https://lwn.net/Articles/481055/

Many thanks,
Ionela.


Ionela Voinescu (2):
  cpufreq,arm,arm64: restructure definitions of arch_set_freq_scale()
  arm: disable frequency invariance for CONFIG_BL_SWITCHER

 arch/arm/include/asm/topology.h   |  4 ++++
 arch/arm64/include/asm/topology.h |  1 +
 drivers/base/arch_topology.c      |  4 ++--
 drivers/cpufreq/cpufreq.c         |  7 -------
 include/linux/arch_topology.h     |  2 ++
 include/linux/cpufreq.h           | 11 ++++++++---
 6 files changed, 17 insertions(+), 12 deletions(-)

-- 
2.17.1

[PATCH 1/2] cpufreq,arm,arm64: restructure definitions of arch_set_freq_scale()

[Ionela Voinescu]

Compared to other arch_* functions, arch_set_freq_scale() has an atypical
weak definition that can be replaced by a strong architecture specific
implementation.

The more typical support for architectural functions involves defining
an empty stub in a header file if the symbol is not already defined in
architecture code. Some examples involve:
 - #define arch_scale_freq_capacity	topology_get_freq_scale
 - #define arch_scale_freq_invariant	topology_scale_freq_invariant
 - #define arch_scale_cpu_capacity	topology_get_cpu_scale
 - #define arch_update_cpu_topology	topology_update_cpu_topology
 - #define arch_scale_thermal_pressure	topology_get_thermal_pressure
 - #define arch_set_thermal_pressure	topology_set_thermal_pressure

Bring arch_set_freq_scale() in line with these functions by renaming it to
topology_set_freq_scale() in the arch topology driver, and by defining the
arch_set_freq_scale symbol to point to the new function for arm and arm64.

While there are other users of the arch_topology driver, this patch defines
arch_set_freq_scale for arm and arm64 only, due to their existing
definitions of arch_scale_freq_capacity. This is the getter function of the
frequency invariance scale factor and without a getter function, the
setter function - arch_set_freq_scale() has not purpose.

Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm/include/asm/topology.h   |  1 +
 arch/arm64/include/asm/topology.h |  1 +
 drivers/base/arch_topology.c      |  4 ++--
 drivers/cpufreq/cpufreq.c         |  7 -------
 include/linux/arch_topology.h     |  2 ++
 include/linux/cpufreq.h           | 11 ++++++++---
 6 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/arch/arm/include/asm/topology.h b/arch/arm/include/asm/topology.h
index 9219e67befbe..e5e3d5ce4d55 100644
--- a/arch/arm/include/asm/topology.h
+++ b/arch/arm/include/asm/topology.h
@@ -8,6 +8,7 @@
 #include <linux/arch_topology.h>
 
 /* Replace task scheduler's default frequency-invariant accounting */
+#define arch_set_freq_scale topology_set_freq_scale
 #define arch_scale_freq_capacity topology_get_freq_scale
 #define arch_scale_freq_invariant topology_scale_freq_invariant
 
diff --git a/arch/arm64/include/asm/topology.h b/arch/arm64/include/asm/topology.h
index 7cb519473fbd..11a465243f66 100644
--- a/arch/arm64/include/asm/topology.h
+++ b/arch/arm64/include/asm/topology.h
@@ -26,6 +26,7 @@ void topology_scale_freq_tick(void);
 #endif /* CONFIG_ARM64_AMU_EXTN */
 
 /* Replace task scheduler's default frequency-invariant accounting */
+#define arch_set_freq_scale topology_set_freq_scale
 #define arch_scale_freq_capacity topology_get_freq_scale
 #define arch_scale_freq_invariant topology_scale_freq_invariant
 
diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
index 1705c772c273..ae5a596bcf86 100644
--- a/drivers/base/arch_topology.c
+++ b/drivers/base/arch_topology.c
@@ -33,8 +33,8 @@ __weak bool arch_freq_counters_available(const struct cpumask *cpus)
 }
 DEFINE_PER_CPU(unsigned long, freq_scale) = SCHED_CAPACITY_SCALE;
 
-void arch_set_freq_scale(const struct cpumask *cpus, unsigned long cur_freq,
-			 unsigned long max_freq)
+void topology_set_freq_scale(const struct cpumask *cpus, unsigned long cur_freq,
+			     unsigned long max_freq)
 {
 	unsigned long scale;
 	int i;
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 2ea245a6c0c0..f34d3a3d5ba6 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -160,13 +160,6 @@ u64 get_cpu_idle_time(unsigned int cpu, u64 *wall, int io_busy)
 }
 EXPORT_SYMBOL_GPL(get_cpu_idle_time);
 
-__weak void arch_set_freq_scale(const struct cpumask *cpus,
-				unsigned long cur_freq,
-				unsigned long max_freq)
-{
-}
-EXPORT_SYMBOL_GPL(arch_set_freq_scale);
-
 /*
  * This is a generic cpufreq init() routine which can be used by cpufreq
  * drivers of SMP systems. It will do following:
diff --git a/include/linux/arch_topology.h b/include/linux/arch_topology.h
index 083df331a3c9..0f6cd6b73a61 100644
--- a/include/linux/arch_topology.h
+++ b/include/linux/arch_topology.h
@@ -30,6 +30,8 @@ static inline unsigned long topology_get_freq_scale(int cpu)
 	return per_cpu(freq_scale, cpu);
 }
 
+void topology_set_freq_scale(const struct cpumask *cpus, unsigned long cur_freq,
+			     unsigned long max_freq);
 bool topology_scale_freq_invariant(void);
 
 bool arch_freq_counters_available(const struct cpumask *cpus);
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 9f779fbdbe7b..fa37b1c66443 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -1011,9 +1011,14 @@ static inline void sched_cpufreq_governor_change(struct cpufreq_policy *policy,
 extern void arch_freq_prepare_all(void);
 extern unsigned int arch_freq_get_on_cpu(int cpu);
 
-extern void arch_set_freq_scale(const struct cpumask *cpus,
-				unsigned long cur_freq,
-				unsigned long max_freq);
+#ifndef arch_set_freq_scale
+static __always_inline
+void arch_set_freq_scale(const struct cpumask *cpus,
+			 unsigned long cur_freq,
+			 unsigned long max_freq)
+{
+}
+#endif
 
 /* the following are really really optional */
 extern struct freq_attr cpufreq_freq_attr_scaling_available_freqs;
-- 
2.17.1

[PATCH 2/2] arm: disable frequency invariance for CONFIG_BL_SWITCHER

[Ionela Voinescu]

big.LITTLE switching complicates the setting of a correct cpufreq-based
frequency invariance scale factor due to (as observed in
drivers/cpufreq/vexpress-spc-cpufreq.c):
 - Incorrect current and maximum frequencies as a result of the
   exposure of a virtual frequency table to the cpufreq core,
 - Missed updates as a result of asynchronous frequency adjustments
   caused by frequency changes in other CPU pairs.

Given that its functionality is atypical in regards to frequency
invariance and this is an old technology, disable frequency
invariance for when big.LITTLE switching is configured in to prevent
incorrect scale setting.

Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com>
Suggested-by: Dietmar Eggemann <dietmar.eggemann@arm.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Catalin Marinas <catalin.marinas@arm.com>
---
 arch/arm/include/asm/topology.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm/include/asm/topology.h b/arch/arm/include/asm/topology.h
index e5e3d5ce4d55..470299ee2fba 100644
--- a/arch/arm/include/asm/topology.h
+++ b/arch/arm/include/asm/topology.h
@@ -7,10 +7,13 @@
 #include <linux/cpumask.h>
 #include <linux/arch_topology.h>
 
+/* big.LITTLE switcher is incompatible with frequency invariance */
+#ifndef CONFIG_BL_SWITCHER
 /* Replace task scheduler's default frequency-invariant accounting */
 #define arch_set_freq_scale topology_set_freq_scale
 #define arch_scale_freq_capacity topology_get_freq_scale
 #define arch_scale_freq_invariant topology_scale_freq_invariant
+#endif
 
 /* Replace task scheduler's default cpu-invariant accounting */
 #define arch_scale_cpu_capacity topology_get_cpu_scale
-- 
2.17.1

Re: [PATCH 1/2] cpufreq,arm,arm64: restructure definitions of arch_set_freq_scale()

[Viresh Kumar]

On 24-09-20, 13:30, Ionela Voinescu wrote:
> Compared to other arch_* functions, arch_set_freq_scale() has an atypical
> weak definition that can be replaced by a strong architecture specific
> implementation.
> 
> The more typical support for architectural functions involves defining
> an empty stub in a header file if the symbol is not already defined in
> architecture code. Some examples involve:
>  - #define arch_scale_freq_capacity	topology_get_freq_scale
>  - #define arch_scale_freq_invariant	topology_scale_freq_invariant
>  - #define arch_scale_cpu_capacity	topology_get_cpu_scale
>  - #define arch_update_cpu_topology	topology_update_cpu_topology
>  - #define arch_scale_thermal_pressure	topology_get_thermal_pressure
>  - #define arch_set_thermal_pressure	topology_set_thermal_pressure
> 
> Bring arch_set_freq_scale() in line with these functions by renaming it to
> topology_set_freq_scale() in the arch topology driver, and by defining the
> arch_set_freq_scale symbol to point to the new function for arm and arm64.
> 
> While there are other users of the arch_topology driver, this patch defines
> arch_set_freq_scale for arm and arm64 only, due to their existing
> definitions of arch_scale_freq_capacity. This is the getter function of the
> frequency invariance scale factor and without a getter function, the
> setter function - arch_set_freq_scale() has not purpose.
> 
> Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> ---
>  arch/arm/include/asm/topology.h   |  1 +
>  arch/arm64/include/asm/topology.h |  1 +
>  drivers/base/arch_topology.c      |  4 ++--
>  drivers/cpufreq/cpufreq.c         |  7 -------
>  include/linux/arch_topology.h     |  2 ++
>  include/linux/cpufreq.h           | 11 ++++++++---
>  6 files changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/arm/include/asm/topology.h b/arch/arm/include/asm/topology.h
> index 9219e67befbe..e5e3d5ce4d55 100644
> --- a/arch/arm/include/asm/topology.h
> +++ b/arch/arm/include/asm/topology.h
> @@ -8,6 +8,7 @@
>  #include <linux/arch_topology.h>
>  
>  /* Replace task scheduler's default frequency-invariant accounting */
> +#define arch_set_freq_scale topology_set_freq_scale
>  #define arch_scale_freq_capacity topology_get_freq_scale
>  #define arch_scale_freq_invariant topology_scale_freq_invariant
>  
> diff --git a/arch/arm64/include/asm/topology.h b/arch/arm64/include/asm/topology.h
> index 7cb519473fbd..11a465243f66 100644
> --- a/arch/arm64/include/asm/topology.h
> +++ b/arch/arm64/include/asm/topology.h
> @@ -26,6 +26,7 @@ void topology_scale_freq_tick(void);
>  #endif /* CONFIG_ARM64_AMU_EXTN */
>  
>  /* Replace task scheduler's default frequency-invariant accounting */
> +#define arch_set_freq_scale topology_set_freq_scale
>  #define arch_scale_freq_capacity topology_get_freq_scale
>  #define arch_scale_freq_invariant topology_scale_freq_invariant
>  
> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> index 1705c772c273..ae5a596bcf86 100644
> --- a/drivers/base/arch_topology.c
> +++ b/drivers/base/arch_topology.c
> @@ -33,8 +33,8 @@ __weak bool arch_freq_counters_available(const struct cpumask *cpus)
>  }
>  DEFINE_PER_CPU(unsigned long, freq_scale) = SCHED_CAPACITY_SCALE;
>  
> -void arch_set_freq_scale(const struct cpumask *cpus, unsigned long cur_freq,
> -			 unsigned long max_freq)
> +void topology_set_freq_scale(const struct cpumask *cpus, unsigned long cur_freq,
> +			     unsigned long max_freq)
>  {
>  	unsigned long scale;
>  	int i;
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 2ea245a6c0c0..f34d3a3d5ba6 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -160,13 +160,6 @@ u64 get_cpu_idle_time(unsigned int cpu, u64 *wall, int io_busy)
>  }
>  EXPORT_SYMBOL_GPL(get_cpu_idle_time);
>  
> -__weak void arch_set_freq_scale(const struct cpumask *cpus,
> -				unsigned long cur_freq,
> -				unsigned long max_freq)
> -{
> -}
> -EXPORT_SYMBOL_GPL(arch_set_freq_scale);
> -

Why can't we have this anymore ? Because it is a macro now instead of a routine
for ARM ?

>  /*
>   * This is a generic cpufreq init() routine which can be used by cpufreq
>   * drivers of SMP systems. It will do following:
> diff --git a/include/linux/arch_topology.h b/include/linux/arch_topology.h
> index 083df331a3c9..0f6cd6b73a61 100644
> --- a/include/linux/arch_topology.h
> +++ b/include/linux/arch_topology.h
> @@ -30,6 +30,8 @@ static inline unsigned long topology_get_freq_scale(int cpu)
>  	return per_cpu(freq_scale, cpu);
>  }
>  
> +void topology_set_freq_scale(const struct cpumask *cpus, unsigned long cur_freq,
> +			     unsigned long max_freq);
>  bool topology_scale_freq_invariant(void);
>  
>  bool arch_freq_counters_available(const struct cpumask *cpus);
> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> index 9f779fbdbe7b..fa37b1c66443 100644
> --- a/include/linux/cpufreq.h
> +++ b/include/linux/cpufreq.h
> @@ -1011,9 +1011,14 @@ static inline void sched_cpufreq_governor_change(struct cpufreq_policy *policy,
>  extern void arch_freq_prepare_all(void);
>  extern unsigned int arch_freq_get_on_cpu(int cpu);
>  
> -extern void arch_set_freq_scale(const struct cpumask *cpus,
> -				unsigned long cur_freq,
> -				unsigned long max_freq);
> +#ifndef arch_set_freq_scale
> +static __always_inline
> +void arch_set_freq_scale(const struct cpumask *cpus,
> +			 unsigned long cur_freq,
> +			 unsigned long max_freq)
> +{
> +}
> +#endif
>  
>  /* the following are really really optional */
>  extern struct freq_attr cpufreq_freq_attr_scaling_available_freqs;
> -- 
> 2.17.1

-- 
viresh

Re: [PATCH 2/2] arm: disable frequency invariance for CONFIG_BL_SWITCHER

[Viresh Kumar]

On 24-09-20, 13:30, Ionela Voinescu wrote:
> big.LITTLE switching complicates the setting of a correct cpufreq-based
> frequency invariance scale factor due to (as observed in
> drivers/cpufreq/vexpress-spc-cpufreq.c):
>  - Incorrect current and maximum frequencies as a result of the
>    exposure of a virtual frequency table to the cpufreq core,
>  - Missed updates as a result of asynchronous frequency adjustments
>    caused by frequency changes in other CPU pairs.
> 
> Given that its functionality is atypical in regards to frequency
> invariance and this is an old technology, disable frequency
> invariance for when big.LITTLE switching is configured in to prevent
> incorrect scale setting.
> 
> Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com>
> Suggested-by: Dietmar Eggemann <dietmar.eggemann@arm.com>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> ---
>  arch/arm/include/asm/topology.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/arm/include/asm/topology.h b/arch/arm/include/asm/topology.h
> index e5e3d5ce4d55..470299ee2fba 100644
> --- a/arch/arm/include/asm/topology.h
> +++ b/arch/arm/include/asm/topology.h
> @@ -7,10 +7,13 @@
>  #include <linux/cpumask.h>
>  #include <linux/arch_topology.h>
>  
> +/* big.LITTLE switcher is incompatible with frequency invariance */
> +#ifndef CONFIG_BL_SWITCHER
>  /* Replace task scheduler's default frequency-invariant accounting */
>  #define arch_set_freq_scale topology_set_freq_scale
>  #define arch_scale_freq_capacity topology_get_freq_scale
>  #define arch_scale_freq_invariant topology_scale_freq_invariant
> +#endif
>  
>  /* Replace task scheduler's default cpu-invariant accounting */
>  #define arch_scale_cpu_capacity topology_get_cpu_scale

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

Re: [PATCH 1/2] cpufreq,arm,arm64: restructure definitions of arch_set_freq_scale()

[Ionela Voinescu]

Hi Viresh,

On Tuesday 06 Oct 2020 at 12:35:51 (+0530), Viresh Kumar wrote:
> On 24-09-20, 13:30, Ionela Voinescu wrote:
> > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > index 2ea245a6c0c0..f34d3a3d5ba6 100644
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -160,13 +160,6 @@ u64 get_cpu_idle_time(unsigned int cpu, u64 *wall, int io_busy)
> >  }
> >  EXPORT_SYMBOL_GPL(get_cpu_idle_time);
> >  
> > -__weak void arch_set_freq_scale(const struct cpumask *cpus,
> > -				unsigned long cur_freq,
> > -				unsigned long max_freq)
> > -{
> > -}
> > -EXPORT_SYMBOL_GPL(arch_set_freq_scale);
> > -
> 
> Why can't we have this anymore ? Because it is a macro now instead of a routine
> for ARM ?

Yes, instead of having a strong counterpart in arch_topology.c to be used
for both arm64 and arm, I separated the definitions of the
arch_set_freq_scale symbol in each architecture's topology.h in order to
be filtered if needed (as is the case for arm's BL_SWITCHER).

I could have kept either this weak stub definition or the stub that is
now in cpufreq.h under "#ifndef arch_set_freq_scale", but I chose the
latter as it's more aligned with the other architectural functions
referenced in the commit message.

So this patch has both the purpose of enabling a nicer filtering of
frequency invariance for BL_SWITCHER but also follows the definition
pattern of the other functions so we don't keep having two mindsets when
working with them.

Thanks,
Ionela.

Re: [PATCH 1/2] cpufreq,arm,arm64: restructure definitions of arch_set_freq_scale()

[Viresh Kumar]

On 24-09-20, 13:30, Ionela Voinescu wrote:
> Compared to other arch_* functions, arch_set_freq_scale() has an atypical
> weak definition that can be replaced by a strong architecture specific
> implementation.
> 
> The more typical support for architectural functions involves defining
> an empty stub in a header file if the symbol is not already defined in
> architecture code. Some examples involve:
>  - #define arch_scale_freq_capacity	topology_get_freq_scale
>  - #define arch_scale_freq_invariant	topology_scale_freq_invariant
>  - #define arch_scale_cpu_capacity	topology_get_cpu_scale
>  - #define arch_update_cpu_topology	topology_update_cpu_topology
>  - #define arch_scale_thermal_pressure	topology_get_thermal_pressure
>  - #define arch_set_thermal_pressure	topology_set_thermal_pressure
> 
> Bring arch_set_freq_scale() in line with these functions by renaming it to
> topology_set_freq_scale() in the arch topology driver, and by defining the
> arch_set_freq_scale symbol to point to the new function for arm and arm64.
> 
> While there are other users of the arch_topology driver, this patch defines
> arch_set_freq_scale for arm and arm64 only, due to their existing
> definitions of arch_scale_freq_capacity. This is the getter function of the
> frequency invariance scale factor and without a getter function, the
> setter function - arch_set_freq_scale() has not purpose.
> 
> Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> ---
>  arch/arm/include/asm/topology.h   |  1 +
>  arch/arm64/include/asm/topology.h |  1 +
>  drivers/base/arch_topology.c      |  4 ++--
>  drivers/cpufreq/cpufreq.c         |  7 -------
>  include/linux/arch_topology.h     |  2 ++
>  include/linux/cpufreq.h           | 11 ++++++++---
>  6 files changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/arm/include/asm/topology.h b/arch/arm/include/asm/topology.h
> index 9219e67befbe..e5e3d5ce4d55 100644
> --- a/arch/arm/include/asm/topology.h
> +++ b/arch/arm/include/asm/topology.h
> @@ -8,6 +8,7 @@
>  #include <linux/arch_topology.h>
>  
>  /* Replace task scheduler's default frequency-invariant accounting */
> +#define arch_set_freq_scale topology_set_freq_scale
>  #define arch_scale_freq_capacity topology_get_freq_scale
>  #define arch_scale_freq_invariant topology_scale_freq_invariant
>  
> diff --git a/arch/arm64/include/asm/topology.h b/arch/arm64/include/asm/topology.h
> index 7cb519473fbd..11a465243f66 100644
> --- a/arch/arm64/include/asm/topology.h
> +++ b/arch/arm64/include/asm/topology.h
> @@ -26,6 +26,7 @@ void topology_scale_freq_tick(void);
>  #endif /* CONFIG_ARM64_AMU_EXTN */
>  
>  /* Replace task scheduler's default frequency-invariant accounting */
> +#define arch_set_freq_scale topology_set_freq_scale
>  #define arch_scale_freq_capacity topology_get_freq_scale
>  #define arch_scale_freq_invariant topology_scale_freq_invariant
>  
> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> index 1705c772c273..ae5a596bcf86 100644
> --- a/drivers/base/arch_topology.c
> +++ b/drivers/base/arch_topology.c
> @@ -33,8 +33,8 @@ __weak bool arch_freq_counters_available(const struct cpumask *cpus)
>  }
>  DEFINE_PER_CPU(unsigned long, freq_scale) = SCHED_CAPACITY_SCALE;
>  
> -void arch_set_freq_scale(const struct cpumask *cpus, unsigned long cur_freq,
> -			 unsigned long max_freq)
> +void topology_set_freq_scale(const struct cpumask *cpus, unsigned long cur_freq,
> +			     unsigned long max_freq)
>  {
>  	unsigned long scale;
>  	int i;
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 2ea245a6c0c0..f34d3a3d5ba6 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -160,13 +160,6 @@ u64 get_cpu_idle_time(unsigned int cpu, u64 *wall, int io_busy)
>  }
>  EXPORT_SYMBOL_GPL(get_cpu_idle_time);
>  
> -__weak void arch_set_freq_scale(const struct cpumask *cpus,
> -				unsigned long cur_freq,
> -				unsigned long max_freq)
> -{
> -}
> -EXPORT_SYMBOL_GPL(arch_set_freq_scale);
> -
>  /*
>   * This is a generic cpufreq init() routine which can be used by cpufreq
>   * drivers of SMP systems. It will do following:
> diff --git a/include/linux/arch_topology.h b/include/linux/arch_topology.h
> index 083df331a3c9..0f6cd6b73a61 100644
> --- a/include/linux/arch_topology.h
> +++ b/include/linux/arch_topology.h
> @@ -30,6 +30,8 @@ static inline unsigned long topology_get_freq_scale(int cpu)
>  	return per_cpu(freq_scale, cpu);
>  }
>  
> +void topology_set_freq_scale(const struct cpumask *cpus, unsigned long cur_freq,
> +			     unsigned long max_freq);
>  bool topology_scale_freq_invariant(void);
>  
>  bool arch_freq_counters_available(const struct cpumask *cpus);
> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> index 9f779fbdbe7b..fa37b1c66443 100644
> --- a/include/linux/cpufreq.h
> +++ b/include/linux/cpufreq.h
> @@ -1011,9 +1011,14 @@ static inline void sched_cpufreq_governor_change(struct cpufreq_policy *policy,
>  extern void arch_freq_prepare_all(void);
>  extern unsigned int arch_freq_get_on_cpu(int cpu);
>  
> -extern void arch_set_freq_scale(const struct cpumask *cpus,
> -				unsigned long cur_freq,
> -				unsigned long max_freq);
> +#ifndef arch_set_freq_scale
> +static __always_inline
> +void arch_set_freq_scale(const struct cpumask *cpus,
> +			 unsigned long cur_freq,
> +			 unsigned long max_freq)
> +{
> +}
> +#endif
>  
>  /* the following are really really optional */
>  extern struct freq_attr cpufreq_freq_attr_scaling_available_freqs;

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

Re: [PATCH 0/2]cpufreq,topology,arm: disable FI for BL_SWITCHER

[Rafael J. Wysocki]

On Thu, Sep 24, 2020 at 2:30 PM Ionela Voinescu <ionela.voinescu@arm.com> wrote:
>
> This series is the result of the discussions ([1], [2]) around the
> complications that the BL_SWITCHER poses when it comes to Frequency
> Invariance (FI) and it aims to restart the discussions.
>
> To properly scale its per-entity load-tracking signals, the task
> scheduler needs to be given a frequency scale factor, i.e. some image of
> the current frequency the CPU is running at, relative to its maximum
> frequency.
>
> But (reiterating the message in the changelog of patch 2/2), big.LITTLE
> switching complicates the setting of a correct cpufreq-based frequency
> invariance scale factor due to (as observed in
> drivers/cpufreq/vexpress-spc-cpufreq.c):
>  - Incorrect current and maximum frequencies as a result of the
>    exposure of a virtual frequency table to the cpufreq core,
>  - Missed updates as a result of asynchronous frequency adjustments
>    caused by frequency changes in other CPU pairs.
> More information on this feature can be found at [3].
>
> Given that its functionality is atypical in regards to FI and that this
> is an old technology, patch 2/2 disable FI for when big.LITTLE switching
> is configured in to prevent incorrect scale setting.
>
> For this purpose patch 1/2 changes the way arch_set_freq_scale() is
> defined in architecture code which brings it in line with the logic of
> other architectural function definitions while allowing for less invasive
> filtering of FI support.
>
> In the discussions at [2], three possible solutions were suggested:
>  - (1) conditioning FI by !CONFIG_BL_SWITCHER
>  - (2) leave as is with note in driver specifying this FI broken
>    functionality
>  - (3) removing full BL_SWITCHER support
>
> This series restructures the solution at (1). The reason for it is that
> the new patch limits the ifdef filtering to the arm topology include file,
> a location where frequency invariance functions are defined. Therefore,
> this seems more appropriate given that the b.L switcher is an arm
> technology and that the new FI filtering location seems more natural for
> conditioned FI disabling.
>
> Solutions (2) and (3) were not implemented given that there might be some
> remaining users of this technology (Samsung Chromebook 2 - Samsung Exynos
> 5 Octa 5420, Samsung Exynos 5 Octa 5800) and therefore leaving this
> broken (2) seems equally bad to removing support for it (3).
>
> [1] https://lore.kernel.org/lkml/20200701090751.7543-5-ionela.voinescu@arm.com/
> [2] https://lore.kernel.org/lkml/20200722093732.14297-4-ionela.voinescu@arm.com/
> [3] https://lwn.net/Articles/481055/

I can take this set with the ACKs from Viresh if that's fine by
everyone.  Catalin?  Sudeep?

Re: [PATCH 1/2] cpufreq,arm,arm64: restructure definitions of arch_set_freq_scale()

[Catalin Marinas]

On Thu, Sep 24, 2020 at 01:30:15PM +0100, Ionela Voinescu wrote:
> Compared to other arch_* functions, arch_set_freq_scale() has an atypical
> weak definition that can be replaced by a strong architecture specific
> implementation.
> 
> The more typical support for architectural functions involves defining
> an empty stub in a header file if the symbol is not already defined in
> architecture code. Some examples involve:
>  - #define arch_scale_freq_capacity	topology_get_freq_scale
>  - #define arch_scale_freq_invariant	topology_scale_freq_invariant
>  - #define arch_scale_cpu_capacity	topology_get_cpu_scale
>  - #define arch_update_cpu_topology	topology_update_cpu_topology
>  - #define arch_scale_thermal_pressure	topology_get_thermal_pressure
>  - #define arch_set_thermal_pressure	topology_set_thermal_pressure
> 
> Bring arch_set_freq_scale() in line with these functions by renaming it to
> topology_set_freq_scale() in the arch topology driver, and by defining the
> arch_set_freq_scale symbol to point to the new function for arm and arm64.
> 
> While there are other users of the arch_topology driver, this patch defines
> arch_set_freq_scale for arm and arm64 only, due to their existing
> definitions of arch_scale_freq_capacity. This is the getter function of the
> frequency invariance scale factor and without a getter function, the
> setter function - arch_set_freq_scale() has not purpose.
> 
> Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>

For the arm64 changes:

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

Re: [PATCH 0/2]cpufreq,topology,arm: disable FI for BL_SWITCHER

[Sudeep Holla]

On Wed, Oct 07, 2020 at 04:34:44PM +0200, Rafael J. Wysocki wrote:
> On Thu, Sep 24, 2020 at 2:30 PM Ionela Voinescu <ionela.voinescu@arm.com> wrote:
> >
> > This series is the result of the discussions ([1], [2]) around the
> > complications that the BL_SWITCHER poses when it comes to Frequency
> > Invariance (FI) and it aims to restart the discussions.
> >
> > To properly scale its per-entity load-tracking signals, the task
> > scheduler needs to be given a frequency scale factor, i.e. some image of
> > the current frequency the CPU is running at, relative to its maximum
> > frequency.
> >
> > But (reiterating the message in the changelog of patch 2/2), big.LITTLE
> > switching complicates the setting of a correct cpufreq-based frequency
> > invariance scale factor due to (as observed in
> > drivers/cpufreq/vexpress-spc-cpufreq.c):
> >  - Incorrect current and maximum frequencies as a result of the
> >    exposure of a virtual frequency table to the cpufreq core,
> >  - Missed updates as a result of asynchronous frequency adjustments
> >    caused by frequency changes in other CPU pairs.
> > More information on this feature can be found at [3].
> >
> > Given that its functionality is atypical in regards to FI and that this
> > is an old technology, patch 2/2 disable FI for when big.LITTLE switching
> > is configured in to prevent incorrect scale setting.
> >
> > For this purpose patch 1/2 changes the way arch_set_freq_scale() is
> > defined in architecture code which brings it in line with the logic of
> > other architectural function definitions while allowing for less invasive
> > filtering of FI support.
> >
> > In the discussions at [2], three possible solutions were suggested:
> >  - (1) conditioning FI by !CONFIG_BL_SWITCHER
> >  - (2) leave as is with note in driver specifying this FI broken
> >    functionality
> >  - (3) removing full BL_SWITCHER support
> >
> > This series restructures the solution at (1). The reason for it is that
> > the new patch limits the ifdef filtering to the arm topology include file,
> > a location where frequency invariance functions are defined. Therefore,
> > this seems more appropriate given that the b.L switcher is an arm
> > technology and that the new FI filtering location seems more natural for
> > conditioned FI disabling.
> >
> > Solutions (2) and (3) were not implemented given that there might be some
> > remaining users of this technology (Samsung Chromebook 2 - Samsung Exynos
> > 5 Octa 5420, Samsung Exynos 5 Octa 5800) and therefore leaving this
> > broken (2) seems equally bad to removing support for it (3).
> >
> > [1] https://lore.kernel.org/lkml/20200701090751.7543-5-ionela.voinescu@arm.com/
> > [2] https://lore.kernel.org/lkml/20200722093732.14297-4-ionela.voinescu@arm.com/
> > [3] https://lwn.net/Articles/481055/
> 
> I can take this set with the ACKs from Viresh if that's fine by
> everyone.  Catalin?  Sudeep?

Acked-by: Sudeep Holla <sudeep.holla@arm.com> (BL_SWITCHER and topology parts)

-- 
Regards,
Sudeep

Re: [PATCH 0/2]cpufreq,topology,arm: disable FI for BL_SWITCHER

[Rafael J. Wysocki]

On Thu, Oct 8, 2020 at 4:06 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Wed, Oct 07, 2020 at 04:34:44PM +0200, Rafael J. Wysocki wrote:
> > On Thu, Sep 24, 2020 at 2:30 PM Ionela Voinescu <ionela.voinescu@arm.com> wrote:
> > >
> > > This series is the result of the discussions ([1], [2]) around the
> > > complications that the BL_SWITCHER poses when it comes to Frequency
> > > Invariance (FI) and it aims to restart the discussions.
> > >
> > > To properly scale its per-entity load-tracking signals, the task
> > > scheduler needs to be given a frequency scale factor, i.e. some image of
> > > the current frequency the CPU is running at, relative to its maximum
> > > frequency.
> > >
> > > But (reiterating the message in the changelog of patch 2/2), big.LITTLE
> > > switching complicates the setting of a correct cpufreq-based frequency
> > > invariance scale factor due to (as observed in
> > > drivers/cpufreq/vexpress-spc-cpufreq.c):
> > >  - Incorrect current and maximum frequencies as a result of the
> > >    exposure of a virtual frequency table to the cpufreq core,
> > >  - Missed updates as a result of asynchronous frequency adjustments
> > >    caused by frequency changes in other CPU pairs.
> > > More information on this feature can be found at [3].
> > >
> > > Given that its functionality is atypical in regards to FI and that this
> > > is an old technology, patch 2/2 disable FI for when big.LITTLE switching
> > > is configured in to prevent incorrect scale setting.
> > >
> > > For this purpose patch 1/2 changes the way arch_set_freq_scale() is
> > > defined in architecture code which brings it in line with the logic of
> > > other architectural function definitions while allowing for less invasive
> > > filtering of FI support.
> > >
> > > In the discussions at [2], three possible solutions were suggested:
> > >  - (1) conditioning FI by !CONFIG_BL_SWITCHER
> > >  - (2) leave as is with note in driver specifying this FI broken
> > >    functionality
> > >  - (3) removing full BL_SWITCHER support
> > >
> > > This series restructures the solution at (1). The reason for it is that
> > > the new patch limits the ifdef filtering to the arm topology include file,
> > > a location where frequency invariance functions are defined. Therefore,
> > > this seems more appropriate given that the b.L switcher is an arm
> > > technology and that the new FI filtering location seems more natural for
> > > conditioned FI disabling.
> > >
> > > Solutions (2) and (3) were not implemented given that there might be some
> > > remaining users of this technology (Samsung Chromebook 2 - Samsung Exynos
> > > 5 Octa 5420, Samsung Exynos 5 Octa 5800) and therefore leaving this
> > > broken (2) seems equally bad to removing support for it (3).
> > >
> > > [1] https://lore.kernel.org/lkml/20200701090751.7543-5-ionela.voinescu@arm.com/
> > > [2] https://lore.kernel.org/lkml/20200722093732.14297-4-ionela.voinescu@arm.com/
> > > [3] https://lwn.net/Articles/481055/
> >
> > I can take this set with the ACKs from Viresh if that's fine by
> > everyone.  Catalin?  Sudeep?
>
> Acked-by: Sudeep Holla <sudeep.holla@arm.com> (BL_SWITCHER and topology parts)

OK, the series has been applied as 5.10 material, thanks!

Re: [PATCH 0/2]cpufreq,topology,arm: disable FI for BL_SWITCHER

[Ionela Voinescu]

On Thursday 08 Oct 2020 at 17:18:25 (+0200), Rafael J. Wysocki wrote:
> On Thu, Oct 8, 2020 at 4:06 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > On Wed, Oct 07, 2020 at 04:34:44PM +0200, Rafael J. Wysocki wrote:
> > > On Thu, Sep 24, 2020 at 2:30 PM Ionela Voinescu <ionela.voinescu@arm.com> wrote:
> > > >
> > > > This series is the result of the discussions ([1], [2]) around the
> > > > complications that the BL_SWITCHER poses when it comes to Frequency
> > > > Invariance (FI) and it aims to restart the discussions.
> > > >
> > > > To properly scale its per-entity load-tracking signals, the task
> > > > scheduler needs to be given a frequency scale factor, i.e. some image of
> > > > the current frequency the CPU is running at, relative to its maximum
> > > > frequency.
> > > >
> > > > But (reiterating the message in the changelog of patch 2/2), big.LITTLE
> > > > switching complicates the setting of a correct cpufreq-based frequency
> > > > invariance scale factor due to (as observed in
> > > > drivers/cpufreq/vexpress-spc-cpufreq.c):
> > > >  - Incorrect current and maximum frequencies as a result of the
> > > >    exposure of a virtual frequency table to the cpufreq core,
> > > >  - Missed updates as a result of asynchronous frequency adjustments
> > > >    caused by frequency changes in other CPU pairs.
> > > > More information on this feature can be found at [3].
> > > >
> > > > Given that its functionality is atypical in regards to FI and that this
> > > > is an old technology, patch 2/2 disable FI for when big.LITTLE switching
> > > > is configured in to prevent incorrect scale setting.
> > > >
> > > > For this purpose patch 1/2 changes the way arch_set_freq_scale() is
> > > > defined in architecture code which brings it in line with the logic of
> > > > other architectural function definitions while allowing for less invasive
> > > > filtering of FI support.
> > > >
> > > > In the discussions at [2], three possible solutions were suggested:
> > > >  - (1) conditioning FI by !CONFIG_BL_SWITCHER
> > > >  - (2) leave as is with note in driver specifying this FI broken
> > > >    functionality
> > > >  - (3) removing full BL_SWITCHER support
> > > >
> > > > This series restructures the solution at (1). The reason for it is that
> > > > the new patch limits the ifdef filtering to the arm topology include file,
> > > > a location where frequency invariance functions are defined. Therefore,
> > > > this seems more appropriate given that the b.L switcher is an arm
> > > > technology and that the new FI filtering location seems more natural for
> > > > conditioned FI disabling.
> > > >
> > > > Solutions (2) and (3) were not implemented given that there might be some
> > > > remaining users of this technology (Samsung Chromebook 2 - Samsung Exynos
> > > > 5 Octa 5420, Samsung Exynos 5 Octa 5800) and therefore leaving this
> > > > broken (2) seems equally bad to removing support for it (3).
> > > >
> > > > [1] https://lore.kernel.org/lkml/20200701090751.7543-5-ionela.voinescu@arm.com/
> > > > [2] https://lore.kernel.org/lkml/20200722093732.14297-4-ionela.voinescu@arm.com/
> > > > [3] https://lwn.net/Articles/481055/
> > >
> > > I can take this set with the ACKs from Viresh if that's fine by
> > > everyone.  Catalin?  Sudeep?
> >
> > Acked-by: Sudeep Holla <sudeep.holla@arm.com> (BL_SWITCHER and topology parts)
> 
> OK, the series has been applied as 5.10 material, thanks!

Many thanks, guys!
Ionela.