[RFC 0/3] cpufreq: cppc: Add support for frequency invariance

[RFC 0/3] cpufreq: cppc: Add support for frequency invariance

[Viresh Kumar]

Hello,

CPPC cpufreq driver is used for ARM servers and this patch series tries
to provide frequency invariance support for them. The same is also
provided using a specific hardware extension, known as AMU (Activity
Monitors Unit), but that is optional for platforms and at least few of
them don't have it.

This patchset allows multiple parts of the kernel to provide the same
functionality, by registering with the topology core.

This is tested with some hacks, as I didn't have access to the right
hardware, on the ARM64 hikey board to check the overall functionality
and that works fine.

Ionela/Peter Puhov, it would be nice if you guys can give this a shot.

This is based of my cpufreq/arm/linux-next branch (should work on
linux-next too) + a cleanup patch [1] that i sent this morning.

--
viresh

[1] https://lore.kernel.org/lkml/a710fc4e4e0f1d2e561320130b99bcb5167d73b4.1594277563.git.viresh.kumar@linaro.org/

Viresh Kumar (3):
  arm64: topology: Add amu_counters_supported() helper
  topology: Provide generic implementation of
    arch_freq_counters_available()
  cpufreq: cppc: Add support for frequency invariance

 arch/arm64/include/asm/topology.h |   7 --
 arch/arm64/kernel/topology.c      | 165 +++++++++++++++---------------
 drivers/base/arch_topology.c      |  43 +++++++-
 drivers/cpufreq/cppc_cpufreq.c    | 138 ++++++++++++++++++++++++-
 include/linux/arch_topology.h     |   5 +-
 kernel/sched/core.c               |   1 +
 6 files changed, 263 insertions(+), 96 deletions(-)

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

[RFC 1/3] arm64: topology: Add amu_counters_supported() helper

[Viresh Kumar]

We would need to know earlier during the boot cycle if AMUs are
supported or not for all the CPUs, export a routine for that and move
code around to make it more readable.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 arch/arm64/kernel/topology.c | 108 ++++++++++++++++++-----------------
 1 file changed, 56 insertions(+), 52 deletions(-)

diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
index b7da372819fc..74fde35b56ef 100644
--- a/arch/arm64/kernel/topology.c
+++ b/arch/arm64/kernel/topology.c
@@ -130,6 +130,9 @@ static DEFINE_PER_CPU_READ_MOSTLY(unsigned long, arch_max_freq_scale);
 static DEFINE_PER_CPU(u64, arch_const_cycles_prev);
 static DEFINE_PER_CPU(u64, arch_core_cycles_prev);
 static cpumask_var_t amu_fie_cpus;
+static cpumask_var_t valid_cpus;
+static DEFINE_STATIC_KEY_FALSE(amu_fie_key);
+#define amu_freq_invariant() static_branch_unlikely(&amu_fie_key)
 
 /* Initialize counter reference per-cpu variables for the current CPU */
 void init_cpu_freq_invariance_counters(void)
@@ -140,26 +143,14 @@ void init_cpu_freq_invariance_counters(void)
 		       read_sysreg_s(SYS_AMEVCNTR0_CONST_EL0));
 }
 
-static int validate_cpu_freq_invariance_counters(int cpu)
+static void setup_freq_invariance(int cpu)
 {
-	u64 max_freq_hz, ratio;
-
-	if (!cpu_has_amu_feat(cpu)) {
-		pr_debug("CPU%d: counters are not supported.\n", cpu);
-		return -EINVAL;
-	}
-
-	if (unlikely(!per_cpu(arch_const_cycles_prev, cpu) ||
-		     !per_cpu(arch_core_cycles_prev, cpu))) {
-		pr_debug("CPU%d: cycle counters are not enabled.\n", cpu);
-		return -EINVAL;
-	}
+	struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
+	u64 ratio;
 
-	/* Convert maximum frequency from KHz to Hz and validate */
-	max_freq_hz = cpufreq_get_hw_max_freq(cpu) * 1000;
-	if (unlikely(!max_freq_hz)) {
-		pr_debug("CPU%d: invalid maximum frequency.\n", cpu);
-		return -EINVAL;
+	if (!policy) {
+		pr_debug("CPU%d: No cpufreq policy found.\n", cpu);
+		return;
 	}
 
 	/*
@@ -176,69 +167,75 @@ static int validate_cpu_freq_invariance_counters(int cpu)
 	 * be unlikely).
 	 */
 	ratio = (u64)arch_timer_get_rate() << (2 * SCHED_CAPACITY_SHIFT);
-	ratio = div64_u64(ratio, max_freq_hz);
+	ratio = div64_u64(ratio, policy->cpuinfo.max_freq * 1000);
 	if (!ratio) {
 		WARN_ONCE(1, "System timer frequency too low.\n");
-		return -EINVAL;
+		goto out;
 	}
 
 	per_cpu(arch_max_freq_scale, cpu) = (unsigned long)ratio;
 
-	return 0;
-}
-
-static inline void update_amu_fie_cpus(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;
-	}
-
 	if (cpumask_subset(policy->related_cpus, valid_cpus))
-		cpumask_or(amu_fie_cpus, policy->related_cpus,
-			   amu_fie_cpus);
+		cpumask_or(amu_fie_cpus, policy->related_cpus, amu_fie_cpus);
 
+out:
 	cpufreq_cpu_put(policy);
 }
 
-static DEFINE_STATIC_KEY_FALSE(amu_fie_key);
-#define amu_freq_invariant() static_branch_unlikely(&amu_fie_key)
+bool amu_counters_supported(void)
+{
+	return likely(cpumask_available(valid_cpus)) &&
+		cpumask_equal(valid_cpus, cpu_present_mask);
+}
 
-static int __init init_amu_fie(void)
+static int __init early_init_amu_fie(void)
 {
-	cpumask_var_t valid_cpus;
-	int ret = 0;
 	int cpu;
 
 	if (!zalloc_cpumask_var(&valid_cpus, GFP_KERNEL))
 		return -ENOMEM;
 
-	if (!zalloc_cpumask_var(&amu_fie_cpus, GFP_KERNEL)) {
-		ret = -ENOMEM;
-		goto free_valid_mask;
-	}
-
 	for_each_present_cpu(cpu) {
-		if (validate_cpu_freq_invariance_counters(cpu))
+		if (!cpu_has_amu_feat(cpu)) {
+			pr_debug("CPU%d: counters are not supported.\n", cpu);
+			continue;
+		}
+
+		if (unlikely(!per_cpu(arch_const_cycles_prev, cpu) ||
+			     !per_cpu(arch_core_cycles_prev, cpu))) {
+			pr_debug("CPU%d: cycle counters are not enabled.\n", cpu);
 			continue;
+		}
+
 		cpumask_set_cpu(cpu, valid_cpus);
-		update_amu_fie_cpus(cpu, valid_cpus);
 	}
 
+	return 0;
+}
+core_initcall_sync(early_init_amu_fie);
+
+static int __init late_init_amu_fie(void)
+{
+	int cpu;
+
+	if (!cpumask_available(valid_cpus))
+		return -ENOMEM;
+
+	if (!zalloc_cpumask_var(&amu_fie_cpus, GFP_KERNEL))
+		return -ENOMEM;
+
+	for_each_present_cpu(cpu)
+		setup_freq_invariance(cpu);
+
 	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);
 	}
 
-free_valid_mask:
-	free_cpumask_var(valid_cpus);
-
-	return ret;
+	return 0;
 }
-late_initcall_sync(init_amu_fie);
+late_initcall_sync(late_init_amu_fie);
 
 bool arch_freq_counters_available(struct cpumask *cpus)
 {
@@ -272,7 +269,7 @@ void topology_scale_freq_tick(void)
 	 * scale =  ------- * --------------------
 	 *	    /\const   SCHED_CAPACITY_SCALE
 	 *
-	 * See validate_cpu_freq_invariance_counters() for details on
+	 * See setup_freq_invariance() for details on
 	 * arch_max_freq_scale and the use of SCHED_CAPACITY_SHIFT.
 	 */
 	scale = core_cnt - prev_core_cnt;
@@ -287,4 +284,11 @@ void topology_scale_freq_tick(void)
 	this_cpu_write(arch_core_cycles_prev, core_cnt);
 	this_cpu_write(arch_const_cycles_prev, const_cnt);
 }
+#else
+bool amu_counters_supported(void)
+{
+	return false;
+}
 #endif /* CONFIG_ARM64_AMU_EXTN */
+
+EXPORT_SYMBOL_GPL(amu_counters_supported);
-- 
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

[RFC 2/3] topology: Provide generic implementation of arch_freq_counters_available()

[Viresh Kumar]

arch_freq_counters_available() is implemented only for ARM AMU hardware
right now and this patch attempts to make it generic enough so other
parts of the kernel can also register their own implementation of
topology_scale_freq_tick() routine.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 arch/arm64/include/asm/topology.h |  7 ---
 arch/arm64/kernel/topology.c      | 81 +++++++++++++++----------------
 drivers/base/arch_topology.c      | 43 ++++++++++++++--
 include/linux/arch_topology.h     |  5 +-
 4 files changed, 81 insertions(+), 55 deletions(-)

diff --git a/arch/arm64/include/asm/topology.h b/arch/arm64/include/asm/topology.h
index 0cc835ddfcd1..93cca6a8cde3 100644
--- a/arch/arm64/include/asm/topology.h
+++ b/arch/arm64/include/asm/topology.h
@@ -16,14 +16,7 @@ int pcibus_to_node(struct pci_bus *bus);
 
 #include <linux/arch_topology.h>
 
-#ifdef CONFIG_ARM64_AMU_EXTN
-/*
- * Replace task scheduler's default counter-based
- * frequency-invariance scale factor setting.
- */
-void topology_scale_freq_tick(void);
 #define arch_scale_freq_tick topology_scale_freq_tick
-#endif /* CONFIG_ARM64_AMU_EXTN */
 
 /* Replace task scheduler's default frequency-invariant accounting */
 #define arch_scale_freq_capacity topology_get_freq_scale
diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
index 74fde35b56ef..97741da31b6d 100644
--- a/arch/arm64/kernel/topology.c
+++ b/arch/arm64/kernel/topology.c
@@ -188,6 +188,41 @@ bool amu_counters_supported(void)
 		cpumask_equal(valid_cpus, cpu_present_mask);
 }
 
+void amu_scale_freq_tick(void)
+{
+	u64 prev_core_cnt, prev_const_cnt;
+	u64 core_cnt, const_cnt, scale;
+
+	const_cnt = read_sysreg_s(SYS_AMEVCNTR0_CONST_EL0);
+	core_cnt = read_sysreg_s(SYS_AMEVCNTR0_CORE_EL0);
+	prev_const_cnt = this_cpu_read(arch_const_cycles_prev);
+	prev_core_cnt = this_cpu_read(arch_core_cycles_prev);
+
+	if (unlikely(core_cnt <= prev_core_cnt ||
+		     const_cnt <= prev_const_cnt))
+		goto store_and_exit;
+
+	/*
+	 *	    /\core    arch_max_freq_scale
+	 * scale =  ------- * --------------------
+	 *	    /\const   SCHED_CAPACITY_SCALE
+	 *
+	 * See setup_freq_invariance() for details on
+	 * arch_max_freq_scale and the use of SCHED_CAPACITY_SHIFT.
+	 */
+	scale = core_cnt - prev_core_cnt;
+	scale *= this_cpu_read(arch_max_freq_scale);
+	scale = div64_u64(scale >> SCHED_CAPACITY_SHIFT,
+			  const_cnt - prev_const_cnt);
+
+	scale = min_t(unsigned long, scale, SCHED_CAPACITY_SCALE);
+	this_cpu_write(freq_scale, (unsigned long)scale);
+
+store_and_exit:
+	this_cpu_write(arch_core_cycles_prev, core_cnt);
+	this_cpu_write(arch_const_cycles_prev, const_cnt);
+}
+
 static int __init early_init_amu_fie(void)
 {
 	int cpu;
@@ -230,9 +265,11 @@ static int __init late_init_amu_fie(void)
 	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 (!topology_set_scale_freq_tick(amu_scale_freq_tick, amu_fie_cpus))
+			pr_info("Registered amu_scale_freq_tick()\n");
 	}
 
+	free_cpumask_var(amu_fie_cpus);
 	return 0;
 }
 late_initcall_sync(late_init_amu_fie);
@@ -242,48 +279,6 @@ bool arch_freq_counters_available(struct cpumask *cpus)
 	return amu_freq_invariant() &&
 	       cpumask_subset(cpus, amu_fie_cpus);
 }
-
-void topology_scale_freq_tick(void)
-{
-	u64 prev_core_cnt, prev_const_cnt;
-	u64 core_cnt, const_cnt, scale;
-	int cpu = smp_processor_id();
-
-	if (!amu_freq_invariant())
-		return;
-
-	if (!cpumask_test_cpu(cpu, amu_fie_cpus))
-		return;
-
-	const_cnt = read_sysreg_s(SYS_AMEVCNTR0_CONST_EL0);
-	core_cnt = read_sysreg_s(SYS_AMEVCNTR0_CORE_EL0);
-	prev_const_cnt = this_cpu_read(arch_const_cycles_prev);
-	prev_core_cnt = this_cpu_read(arch_core_cycles_prev);
-
-	if (unlikely(core_cnt <= prev_core_cnt ||
-		     const_cnt <= prev_const_cnt))
-		goto store_and_exit;
-
-	/*
-	 *	    /\core    arch_max_freq_scale
-	 * scale =  ------- * --------------------
-	 *	    /\const   SCHED_CAPACITY_SCALE
-	 *
-	 * See setup_freq_invariance() for details on
-	 * arch_max_freq_scale and the use of SCHED_CAPACITY_SHIFT.
-	 */
-	scale = core_cnt - prev_core_cnt;
-	scale *= this_cpu_read(arch_max_freq_scale);
-	scale = div64_u64(scale >> SCHED_CAPACITY_SHIFT,
-			  const_cnt - prev_const_cnt);
-
-	scale = min_t(unsigned long, scale, SCHED_CAPACITY_SCALE);
-	this_cpu_write(freq_scale, (unsigned long)scale);
-
-store_and_exit:
-	this_cpu_write(arch_core_cycles_prev, core_cnt);
-	this_cpu_write(arch_const_cycles_prev, const_cnt);
-}
 #else
 bool amu_counters_supported(void)
 {
diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
index 4d0a0038b476..3820109864c1 100644
--- a/drivers/base/arch_topology.c
+++ b/drivers/base/arch_topology.c
@@ -21,11 +21,48 @@
 #include <linux/sched.h>
 #include <linux/smp.h>
 
-__weak bool arch_freq_counters_available(struct cpumask *cpus)
+static void (*scale_freq_tick)(void);
+static cpumask_var_t freq_tick_cpus;
+
+int topology_set_scale_freq_tick(void *ptr, const struct cpumask *cpus)
 {
-	return false;
+	if (scale_freq_tick)
+		return -EBUSY;
+
+	if (!zalloc_cpumask_var(&freq_tick_cpus, GFP_KERNEL))
+		return -ENOMEM;
+
+	cpumask_copy(freq_tick_cpus, cpus);
+	scale_freq_tick = ptr;
+	return 0;
 }
+EXPORT_SYMBOL_GPL(topology_set_scale_freq_tick);
+
+void topology_remove_scale_freq_tick(void *ptr)
+{
+	if (scale_freq_tick == ptr) {
+		free_cpumask_var(freq_tick_cpus);
+		scale_freq_tick = NULL;
+	} else {
+		pr_err("%s: Invalid argument\n", __func__);
+	}
+}
+EXPORT_SYMBOL_GPL(topology_remove_scale_freq_tick);
+
+void topology_scale_freq_tick(void)
+{
+	if (scale_freq_tick &&
+	    cpumask_test_cpu(raw_smp_processor_id(), freq_tick_cpus))
+		scale_freq_tick();
+}
+
+static bool support_scale_freq_tick(struct cpumask *cpus)
+{
+	return scale_freq_tick && cpumask_subset(cpus, freq_tick_cpus);
+}
+
 DEFINE_PER_CPU(unsigned long, freq_scale) = SCHED_CAPACITY_SCALE;
+EXPORT_SYMBOL_GPL(freq_scale);
 
 void arch_set_freq_scale(struct cpumask *cpus, unsigned long cur_freq,
 			 unsigned long max_freq)
@@ -38,7 +75,7 @@ void arch_set_freq_scale(struct cpumask *cpus, unsigned long cur_freq,
 	 * want to update the scale factor with information from CPUFREQ.
 	 * Instead the scale factor will be updated from arch_scale_freq_tick.
 	 */
-	if (arch_freq_counters_available(cpus))
+	if (support_scale_freq_tick(cpus))
 		return;
 
 	scale = (cur_freq << SCHED_CAPACITY_SHIFT) / max_freq;
diff --git a/include/linux/arch_topology.h b/include/linux/arch_topology.h
index 0566cb3314ef..6305148c8aa0 100644
--- a/include/linux/arch_topology.h
+++ b/include/linux/arch_topology.h
@@ -10,6 +10,9 @@
 
 void topology_normalize_cpu_scale(void);
 int topology_update_cpu_topology(void);
+void topology_scale_freq_tick(void);
+int topology_set_scale_freq_tick(void *ptr, const struct cpumask *cpus);
+void topology_remove_scale_freq_tick(void *ptr);
 
 struct device_node;
 bool topology_parse_cpu_capacity(struct device_node *cpu_node, int cpu);
@@ -30,8 +33,6 @@ static inline unsigned long topology_get_freq_scale(int cpu)
 	return per_cpu(freq_scale, cpu);
 }
 
-bool arch_freq_counters_available(struct cpumask *cpus);
-
 DECLARE_PER_CPU(unsigned long, thermal_pressure);
 
 static inline unsigned long topology_get_thermal_pressure(int cpu)
-- 
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: [RFC 0/3] cpufreq: cppc: Add support for frequency invariance

[Ionela Voinescu]

Hi Viresh,

I'll put all my comments here for now, as they refer more to the design
of the solution.

I hope it won't be too repetitive compared to what we previously discussed
offline. I understand you want to get additional points of view.

On Thursday 09 Jul 2020 at 15:43:32 (+0530), Viresh Kumar wrote:
> Hello,
> 
> CPPC cpufreq driver is used for ARM servers and this patch series tries
> to provide frequency invariance support for them. The same is also
> provided using a specific hardware extension, known as AMU (Activity
> Monitors Unit), but that is optional for platforms and at least few of
> them don't have it.
> 
> This patchset allows multiple parts of the kernel to provide the same
> functionality, by registering with the topology core.
> 
> This is tested with some hacks, as I didn't have access to the right
> hardware, on the ARM64 hikey board to check the overall functionality
> and that works fine.
> 
> Ionela/Peter Puhov, it would be nice if you guys can give this a shot.
> 

I believe the code is unnecessarily invasive for the functionality it
tries to introduce and it does break existing functionality.


 - (1) From code readability and design point of view, this switching
       between an architectural method and a driver method complicates
       an already complicated situation. We already have code that
       chooses between a cpufreq-based method and a counter based method
       for frequency invariance. This would basically introduce a choice
       between a cpufreq-based method through arch_set_freq_scale(), an
       architectural counter-based method through arch_set_freq_tick(),
       and another cpufreq-based method that piggy-backs on the
       architectural arch_set_freq_tick().

       As discussed offline, before I even try to begin accepting the
       possibility of this complicated mix, I would like to know why
       methods of obtaining the same thing by using the cpufreq
       arch_set_freq_scale() or even the more invasive wrapping of the
       counter read functions is not working. I believe those solutions
       would brings only a fraction of the complexity added through this
       set.

 - (2) For 1/3, the presence of AMU counters does not guarantee their
       usability for frequency invariance. I know you wanted to avoid
       the complications of AMUs being marked as supporting invariance
       after the cpufreq driver init function, but this breaks the
       scenario in which the maximum frequency is invalid.

 - (3) For 2/3, currently we support platforms that have partial support
       for AMUs, while this would not be supported here. The suggestions
       at (1) would give us this for free.


While I'm keen on having invariance support for CPPC when lacking part
or full support for AMUs, I think we should explore alternative designs.
I can try to come up with some code suggestions, but it will take a few
days as I have many other things in the air :).

I'm happy to test this when the design is agreed on.

Hope it helps,
Ionela.

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

Re: [RFC 0/3] cpufreq: cppc: Add support for frequency invariance

[Viresh Kumar]

Thanks for the quick reply Ionela.

On 09-07-20, 13:43, Ionela Voinescu wrote:
> I'll put all my comments here for now, as they refer more to the design
> of the solution.
> 
> I hope it won't be too repetitive compared to what we previously discussed
> offline.

> I understand you want to get additional points of view.

Not necessarily, I knew you would be one of the major reviewers here
:)

I posted so you don't need to review in private anymore and then the
code is somewhat updated since the previous time.

> On Thursday 09 Jul 2020 at 15:43:32 (+0530), Viresh Kumar wrote:
> I believe the code is unnecessarily invasive for the functionality it
> tries to introduce and it does break existing functionality.
> 
> 
>  - (1) From code readability and design point of view, this switching
>        between an architectural method and a driver method complicates
>        an already complicated situation. We already have code that
>        chooses between a cpufreq-based method and a counter based method
>        for frequency invariance. This would basically introduce a choice
>        between a cpufreq-based method through arch_set_freq_scale(), an
>        architectural counter-based method through arch_set_freq_tick(),
>        and another cpufreq-based method that piggy-backs on the
>        architectural arch_set_freq_tick().

I agree.

>        As discussed offline, before I even try to begin accepting the
>        possibility of this complicated mix, I would like to know why
>        methods of obtaining the same thing by using the cpufreq
>        arch_set_freq_scale()

The problem is same as that was in case of x86, we don't know the real
frequency the CPU may be running at and we need something that fires
up periodically in a guaranteed way to capture the freq-scale.

Though I am thinking now if we can trust the target_index() helper and
keep updating the freq-scale based on the delta between last call to
it and the latest call. I am not sure if it will be sufficient.

>        or even the more invasive wrapping of the
>        counter read functions is not working.

I am not sure I understood this one.

>  - (2) For 1/3, the presence of AMU counters does not guarantee their
>        usability for frequency invariance. I know you wanted to avoid
>        the complications of AMUs being marked as supporting invariance
>        after the cpufreq driver init function, but this breaks the
>        scenario in which the maximum frequency is invalid.

Is that really a scenario ? i.e. Invalid maximum frequency ? Why would
that ever happen ?

And I am not sure if this breaks anything which already exists,
because all we are doing in this case now is not registering cppc for
FI, which should be fine.

>  - (3) For 2/3, currently we support platforms that have partial support
>        for AMUs, while this would not be supported here. The suggestions
>        at (1) would give us this for free.

As both were counter based mechanisms, I thought it would be better
and more consistent if only one of them is picked. Though partial
support of AMUs would still work without the CPPC driver.

-- 
viresh

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

Re: [RFC 0/3] cpufreq: cppc: Add support for frequency invariance

[Vincent Guittot]

On Fri, 10 Jul 2020 at 05:00, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> Thanks for the quick reply Ionela.
>
> On 09-07-20, 13:43, Ionela Voinescu wrote:
> > I'll put all my comments here for now, as they refer more to the design
> > of the solution.
> >
> > I hope it won't be too repetitive compared to what we previously discussed
> > offline.
>
> > I understand you want to get additional points of view.
>
> Not necessarily, I knew you would be one of the major reviewers here
> :)
>
> I posted so you don't need to review in private anymore and then the
> code is somewhat updated since the previous time.
>
> > On Thursday 09 Jul 2020 at 15:43:32 (+0530), Viresh Kumar wrote:
> > I believe the code is unnecessarily invasive for the functionality it
> > tries to introduce and it does break existing functionality.
> >
> >
> >  - (1) From code readability and design point of view, this switching
> >        between an architectural method and a driver method complicates
> >        an already complicated situation. We already have code that
> >        chooses between a cpufreq-based method and a counter based method
> >        for frequency invariance. This would basically introduce a choice
> >        between a cpufreq-based method through arch_set_freq_scale(), an
> >        architectural counter-based method through arch_set_freq_tick(),
> >        and another cpufreq-based method that piggy-backs on the
> >        architectural arch_set_freq_tick().
>
> I agree.
>
> >        As discussed offline, before I even try to begin accepting the
> >        possibility of this complicated mix, I would like to know why
> >        methods of obtaining the same thing by using the cpufreq
> >        arch_set_freq_scale()
>
> The problem is same as that was in case of x86, we don't know the real
> frequency the CPU may be running at and we need something that fires
> up periodically in a guaranteed way to capture the freq-scale.

Yeah it's exactly the same behavior as x86 and re using the same
mechanism seems the  best solution

The main problem is that AMU currently assumes that it will be the
only to support such tick based mechanism whereas others like cppc can
provides similar information

>
> Though I am thinking now if we can trust the target_index() helper and
> keep updating the freq-scale based on the delta between last call to
> it and the latest call. I am not sure if it will be sufficient.
>
> >        or even the more invasive wrapping of the
> >        counter read functions is not working.
>
> I am not sure I understood this one.
>
> >  - (2) For 1/3, the presence of AMU counters does not guarantee their
> >        usability for frequency invariance. I know you wanted to avoid
> >        the complications of AMUs being marked as supporting invariance
> >        after the cpufreq driver init function, but this breaks the
> >        scenario in which the maximum frequency is invalid.
>
> Is that really a scenario ? i.e. Invalid maximum frequency ? Why would
> that ever happen ?
>
> And I am not sure if this breaks anything which already exists,
> because all we are doing in this case now is not registering cppc for
> FI, which should be fine.

IIUC, AMU must wait for cpufreq drivers to be registered in order to
get the maximum freq and being able to enable its FIE support.
Could we have a sequence like:
cppc register its scale_freq_tick function
AMU can then override the tick function for cpu which supports AMU

>
> >  - (3) For 2/3, currently we support platforms that have partial support
> >        for AMUs, while this would not be supported here. The suggestions
> >        at (1) would give us this for free.
>
> As both were counter based mechanisms, I thought it would be better
> and more consistent if only one of them is picked. Though partial
> support of AMUs would still work without the CPPC driver.
>
> --
> viresh

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

Re: [RFC 0/3] cpufreq: cppc: Add support for frequency invariance

[Viresh Kumar]

On 24-07-20, 11:38, Vincent Guittot wrote:
> Yeah it's exactly the same behavior as x86 and re using the same
> mechanism seems the  best solution
> 
> The main problem is that AMU currently assumes that it will be the
> only to support such tick based mechanism whereas others like cppc can
> provides similar information

Ionela do you have some feedback to Vincent's email?

We can't handle this apart from the tick handler, i.e. capture this
data at every tick like it is done for x86 or AMU.

-- 
viresh

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

Re: [RFC 0/3] cpufreq: cppc: Add support for frequency invariance

[Ionela Voinescu]

Hi guys,

On Friday 24 Jul 2020 at 11:38:59 (+0200), Vincent Guittot wrote:
> On Fri, 10 Jul 2020 at 05:00, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > Thanks for the quick reply Ionela.
> >
> > On 09-07-20, 13:43, Ionela Voinescu wrote:
> > > I'll put all my comments here for now, as they refer more to the design
> > > of the solution.
> > >
> > > I hope it won't be too repetitive compared to what we previously discussed
> > > offline.
> >
> > > I understand you want to get additional points of view.
> >
> > Not necessarily, I knew you would be one of the major reviewers here
> > :)
> >
> > I posted so you don't need to review in private anymore and then the
> > code is somewhat updated since the previous time.
> >
> > > On Thursday 09 Jul 2020 at 15:43:32 (+0530), Viresh Kumar wrote:
> > > I believe the code is unnecessarily invasive for the functionality it
> > > tries to introduce and it does break existing functionality.
> > >
> > >
> > >  - (1) From code readability and design point of view, this switching
> > >        between an architectural method and a driver method complicates
> > >        an already complicated situation. We already have code that
> > >        chooses between a cpufreq-based method and a counter based method
> > >        for frequency invariance. This would basically introduce a choice
> > >        between a cpufreq-based method through arch_set_freq_scale(), an
> > >        architectural counter-based method through arch_set_freq_tick(),
> > >        and another cpufreq-based method that piggy-backs on the
> > >        architectural arch_set_freq_tick().
> >
> > I agree.
> >
> > >        As discussed offline, before I even try to begin accepting the
> > >        possibility of this complicated mix, I would like to know why
> > >        methods of obtaining the same thing by using the cpufreq
> > >        arch_set_freq_scale()
> >
> > The problem is same as that was in case of x86, we don't know the real
> > frequency the CPU may be running at and we need something that fires
> > up periodically in a guaranteed way to capture the freq-scale.
> 
> Yeah it's exactly the same behavior as x86 and re using the same
> mechanism seems the  best solution
> 
> The main problem is that AMU currently assumes that it will be the
> only to support such tick based mechanism whereas others like cppc can
> provides similar information
> 

Yes, I agree that a similar method to the use of AMUs or APERF/MPERF would
result in a more accurate frequency scale factor.

> >
> > Though I am thinking now if we can trust the target_index() helper and
> > keep updating the freq-scale based on the delta between last call to
> > it and the latest call. I am not sure if it will be sufficient.
> >
> > >        or even the more invasive wrapping of the
> > >        counter read functions is not working.
> >
> > I am not sure I understood this one.
> >

I've been putting some more thought/code into this one and I believe
something as follows might look nicer as well as cover a few corner cases
(ignore implementation details for now, they can be changed):

- Have a per cpu value that marks the use of either AMUs, CPPC, or
  cpufreq for freq invariance (can be done with per-cpu variable or with
  cpumasks)

- arch_topology.c: initialization code as follows:

	for_each_present_cpu(cpu) {
		if (freq_inv_amus_valid(cpu) &&
		    !freq_inv_set_max_ratio(cpufreq_get_hw_max_freq(cpu) * 1000,
					    arch_timer_get_rate(), cpu)) {
			per_cpu(inv_source, cpu) = INV_AMU_COUNTERS;
			continue;
		}
		if (freq_inv_cppc_counters_valid(cpu) &&
		    !freq_inv_set_max_ratio(cppc_max_perf, cppc_ref_perf, cpu)) {
			per_cpu(inv_source, cpu) = INV_CPPC_COUNTERS;
			continue;
		}
		if (!cpufreq_supports_freq_invariance() ||
		    freq_inv_set_max_ratio(cpufreq_get_hw_max_freq(cpu),
					   1, cpu)) {
			pr_info("FIE disabled: no valid source for CPU%d.", cpu);
			return 0;
		}
	}
  This would live in an equivalent of the init function we have now for
  AMU counters only (init_amu_fie), made to handle more sources and moved
  to arch_topology.c.
  The freq_inv_set_max_ratio() would be a generic version of what is now
  validate_cpu_freq_invariance_counters() (only the ratio computation and
  setting).

 - Finally, 
	void freq_inv_update_counter_refs(void)
	{
		this_cpu_write(arch_core_cycles_prev, read_corecnt());
		this_cpu_write(arch_const_cycles_prev, read_constcnt());
	}
  This would be an equivalent of init_cpu_freq_invariance_counters().
  There is the option of either read_{corecnt/constcnt}() to either do AMU
  reads or CPPC counter reads depending on inv_source, or for either arm64
  or cppc code to define the entire freq_inv_update_counter_refs().

 - Given all of the above, topology_scale_freq_tick() can then be made generic

	prev_const_cnt = this_cpu_read(arch_const_cycles_prev);
	prev_core_cnt = this_cpu_read(arch_core_cycles_prev);
	freq_inv_update_counter_refs();
	const_cnt = this_cpu_read(arch_const_cycles_prev);
	core_cnt = this_cpu_read(arch_core_cycles_prev);

I have some versions of code that do this generalisation for AMUs and cpufreq
with a common topology_set_freq_scale() that is to be used for both
arch_set_freq_scale() and arch_set_freq_tick(). But it's written with this
usecase in mind so it can be extended to use CPPC counters as source as well,
as detailed above.

So, this is basically what I had in mind when recommending "wrapping of the
counter read functions" :). This would basically reuse much of what is now
the AMU invariance code while allowing for CPPC counters as a possible source.

I'll stop here for now to see what you guys think about this.

Thanks,
Ionela.

> > >  - (2) For 1/3, the presence of AMU counters does not guarantee their
> > >        usability for frequency invariance. I know you wanted to avoid
> > >        the complications of AMUs being marked as supporting invariance
> > >        after the cpufreq driver init function, but this breaks the
> > >        scenario in which the maximum frequency is invalid.
> >
> > Is that really a scenario ? i.e. Invalid maximum frequency ? Why would
> > that ever happen ?
> >
> > And I am not sure if this breaks anything which already exists,
> > because all we are doing in this case now is not registering cppc for
> > FI, which should be fine.
> 
> IIUC, AMU must wait for cpufreq drivers to be registered in order to
> get the maximum freq and being able to enable its FIE support.
> Could we have a sequence like:
> cppc register its scale_freq_tick function
> AMU can then override the tick function for cpu which supports AMU
> 
> >
> > >  - (3) For 2/3, currently we support platforms that have partial support
> > >        for AMUs, while this would not be supported here. The suggestions
> > >        at (1) would give us this for free.
> >
> > As both were counter based mechanisms, I thought it would be better
> > and more consistent if only one of them is picked. Though partial
> > support of AMUs would still work without the CPPC driver.
> >
> > --
> > viresh

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

Re: [RFC 0/3] cpufreq: cppc: Add support for frequency invariance

[Viresh Kumar]

On 25-08-20, 10:56, Ionela Voinescu wrote:
> I've been putting some more thought/code into this one and I believe
> something as follows might look nicer as well as cover a few corner cases
> (ignore implementation details for now, they can be changed):

I saw the other patchset you sent where AMU can be used as the backend
for CPPC driver, which means that if AMU IP is present on the platform
it will be used by the CPPC to get the perf counts, right ?

> - Have a per cpu value that marks the use of either AMUs, CPPC, or
>   cpufreq for freq invariance (can be done with per-cpu variable or with
>   cpumasks)
> 
> - arch_topology.c: initialization code as follows:
> 
> 	for_each_present_cpu(cpu) {
> 		if (freq_inv_amus_valid(cpu) &&
> 		    !freq_inv_set_max_ratio(cpufreq_get_hw_max_freq(cpu) * 1000,
> 					    arch_timer_get_rate(), cpu)) {
> 			per_cpu(inv_source, cpu) = INV_AMU_COUNTERS;
> 			continue;
> 		}
> 		if (freq_inv_cppc_counters_valid(cpu) &&
> 		    !freq_inv_set_max_ratio(cppc_max_perf, cppc_ref_perf, cpu)) {
> 			per_cpu(inv_source, cpu) = INV_CPPC_COUNTERS;
> 			continue;
> 		}
> 		if (!cpufreq_supports_freq_invariance() ||
> 		    freq_inv_set_max_ratio(cpufreq_get_hw_max_freq(cpu),
> 					   1, cpu)) {
> 			pr_info("FIE disabled: no valid source for CPU%d.", cpu);
> 			return 0;
> 		}
> 	}

Based on that (your other patchset), I think this can get further
simplified to whomsoever can register first for freq invariance.

i.e. if CPPC registers for it first then there is no need to check
AMUs further (as CPPC will be using AMUs anyway), else we will
fallback to AMU, else cpufreq.

Is that understanding correct ?

-- 
viresh

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

Re: [RFC 0/3] cpufreq: cppc: Add support for frequency invariance

[Ionela Voinescu]

Hi Viresh,

On Thursday 27 Aug 2020 at 13:21:49 (+0530), Viresh Kumar wrote:
> On 25-08-20, 10:56, Ionela Voinescu wrote:
> > I've been putting some more thought/code into this one and I believe
> > something as follows might look nicer as well as cover a few corner cases
> > (ignore implementation details for now, they can be changed):
> 
> I saw the other patchset you sent where AMU can be used as the backend
> for CPPC driver, which means that if AMU IP is present on the platform
> it will be used by the CPPC to get the perf counts, right ?
> 
> > - Have a per cpu value that marks the use of either AMUs, CPPC, or
> >   cpufreq for freq invariance (can be done with per-cpu variable or with
> >   cpumasks)
> > 
> > - arch_topology.c: initialization code as follows:
> > 
> > 	for_each_present_cpu(cpu) {
> > 		if (freq_inv_amus_valid(cpu) &&
> > 		    !freq_inv_set_max_ratio(cpufreq_get_hw_max_freq(cpu) * 1000,
> > 					    arch_timer_get_rate(), cpu)) {
> > 			per_cpu(inv_source, cpu) = INV_AMU_COUNTERS;
> > 			continue;
> > 		}
> > 		if (freq_inv_cppc_counters_valid(cpu) &&
> > 		    !freq_inv_set_max_ratio(cppc_max_perf, cppc_ref_perf, cpu)) {
> > 			per_cpu(inv_source, cpu) = INV_CPPC_COUNTERS;
> > 			continue;
> > 		}
> > 		if (!cpufreq_supports_freq_invariance() ||
> > 		    freq_inv_set_max_ratio(cpufreq_get_hw_max_freq(cpu),
> > 					   1, cpu)) {
> > 			pr_info("FIE disabled: no valid source for CPU%d.", cpu);
> > 			return 0;
> > 		}
> > 	}
> 
> Based on that (your other patchset), I think this can get further
> simplified to whomsoever can register first for freq invariance.
> 

I don't see it as anyone registering for freq invariance, rather the
freq invariance framework chooses its source of information (AMU, CPPC,
cpufreq).

> i.e. if CPPC registers for it first then there is no need to check
> AMUs further (as CPPC will be using AMUs anyway), else we will
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Not necessarily. Even if AMUs are present, they are only used for CPPC's
delivered and reference performance counters if the ACPI _CPC entry
specifies FFH as method:

  ResourceTemplate(){Register(FFixedHW, 0x40, 0, 1, 0x4)},
  ResourceTemplate(){Register(FFixedHW, 0x40, 0, 0, 0x4)},

> fallback to AMU, else cpufreq.
> 
> Is that understanding correct ?

While I understand your point (accessing AMUs through CPPC's read
functions to implement invariance) I don't think it's worth tying the
two together.

I see the two functionalities as independent:
 - frequency invariance with whichever source of information is valid
   (AMUs, cpufreq, etc) is separate from
 - CPPC's delivered and reference performance counters, which currently
   are used in cpufreq's .get() function.

Therefore, taking each of the scenarios one by one:
 - All CPUs support AMUs: the freq invariance initialisation code will
   find AMUs valid and it will use them to set the scale factor;
   completely independently, if the FFH method is specified for CPPC's
   delivered and reference performance counters, it will also use
   AMUs, even if, let's say, invariance is disabled.

 - None of the CPUs support AMUs, but the _CPC entry specifies some
   platform specific counters for delivered and reference performance.
   With the current mainline code neither cpufreq or counter based
   invariance is supported, but the CPPC counters can be used in the
   cppc_cpufreq driver for the .get() function.

   But with the above new functionality we can detect that AMUs are not
   supported and expose the CPPC counters to replace them in
   implementing invariance.

 - Mixed scenarios are also supported if we play our cards right and
   implement the above per-cpu.


I'm thinking that having some well defined invariance sources might work
well: it will simplify the init function (go through all registered
sources and choose (per-cpu) the one that's valid) and allow for
otherwise generic invariance support. Something like:

enum freq_inv_source {INV_CPUFREQ, INV_AMU_COUNTERS, INV_CPPC_COUNTERS};

struct freq_inv_source {
	enum freq_inv_source source;
	bool (*valid)(int cpu);
	u64 (*read_corecnt)(int cpu);
	u64 (*read_constcnt)(int cpu);
	u64 (*max_rate)(int cpu);
	u64 (*ref_rate)(int cpu);
}

I am in the middle of unifying AMU counter and cpufreq invariance through
something like this, so if you like the idea and you don't think I'm
stepping too much on your toes with this, I can consider the usecase in
my (what should be) generic support. So in the end this might end up
being just a matter of adding a new invariance source (CPPC counters).

My only worry is that while I know how a cpufreq source behaves and how
AMU counters behave, I'm not entirely sure what to expect from CPPC
counters: if they are always appropriate for updates on the tick (not
blocking), if they both stop during idle, if there is save/restore
functionality before/after idle, etc.

What do you think?

Regards,
Ionela.

> -- 
> viresh

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

Re: [RFC 0/3] cpufreq: cppc: Add support for frequency invariance

[Viresh Kumar]

On 27-08-20, 12:27, Ionela Voinescu wrote:
> I don't see it as anyone registering for freq invariance, rather the
> freq invariance framework chooses its source of information (AMU, CPPC,
> cpufreq).

Yeah, either way is fine for me.

> > i.e. if CPPC registers for it first then there is no need to check
> > AMUs further (as CPPC will be using AMUs anyway), else we will
>                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Not necessarily. Even if AMUs are present, they are only used for CPPC's
> delivered and reference performance counters if the ACPI _CPC entry
> specifies FFH as method:
> 
>   ResourceTemplate(){Register(FFixedHW, 0x40, 0, 1, 0x4)},
>   ResourceTemplate(){Register(FFixedHW, 0x40, 0, 0, 0x4)},

Right.

> While I understand your point (accessing AMUs through CPPC's read
> functions to implement invariance) I don't think it's worth tying the
> two together.
> 
> I see the two functionalities as independent:
>  - frequency invariance with whichever source of information is valid
>    (AMUs, cpufreq, etc) is separate from
>  - CPPC's delivered and reference performance counters, which currently
>    are used in cpufreq's .get() function.
> 
> Therefore, taking each of the scenarios one by one:
>  - All CPUs support AMUs: the freq invariance initialisation code will
>    find AMUs valid and it will use them to set the scale factor;
>    completely independently, if the FFH method is specified for CPPC's
>    delivered and reference performance counters, it will also use
>    AMUs, even if, let's say, invariance is disabled.
> 
>  - None of the CPUs support AMUs, but the _CPC entry specifies some
>    platform specific counters for delivered and reference performance.
>    With the current mainline code neither cpufreq or counter based
>    invariance is supported, but the CPPC counters can be used in the
>    cppc_cpufreq driver for the .get() function.
> 
>    But with the above new functionality we can detect that AMUs are not
>    supported and expose the CPPC counters to replace them in
>    implementing invariance.
> 
>  - Mixed scenarios are also supported if we play our cards right and
>    implement the above per-cpu.
> 
> 
> I'm thinking that having some well defined invariance sources might work
> well: it will simplify the init function (go through all registered
> sources and choose (per-cpu) the one that's valid) and allow for
> otherwise generic invariance support. Something like:
> 
> enum freq_inv_source {INV_CPUFREQ, INV_AMU_COUNTERS, INV_CPPC_COUNTERS};
> 
> struct freq_inv_source {
> 	enum freq_inv_source source;
> 	bool (*valid)(int cpu);
> 	u64 (*read_corecnt)(int cpu);
> 	u64 (*read_constcnt)(int cpu);
> 	u64 (*max_rate)(int cpu);
> 	u64 (*ref_rate)(int cpu);
> }
> 
> I am in the middle of unifying AMU counter and cpufreq invariance through
> something like this, so if you like the idea and you don't think I'm
> stepping too much on your toes with this, I can consider the usecase in
> my (what should be) generic support. So in the end this might end up
> being just a matter of adding a new invariance source (CPPC counters).

Okay, if you have already started working on that, no issues from my
side. I can just get the relevant stuff from CPPC added once you
provide that layer..

> My only worry is that while I know how a cpufreq source behaves and how
> AMU counters behave, I'm not entirely sure what to expect from CPPC

Neither do I :)

> counters: if they are always appropriate for updates on the tick (not
> blocking),

The update stuff may sleep here and so I had to do stuff in the
irq-work handler in my patch.

> if they both stop during idle, if there is save/restore
> functionality before/after idle, etc.

This I will check.

-- 
viresh

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

Re: [RFC 0/3] cpufreq: cppc: Add support for frequency invariance

[Viresh Kumar]

On 27-08-20, 12:27, Ionela Voinescu wrote:
> I am in the middle of unifying AMU counter and cpufreq invariance through
> something like this, so if you like the idea and you don't think I'm
> stepping too much on your toes with this, I can consider the usecase in
> my (what should be) generic support. So in the end this might end up
> being just a matter of adding a new invariance source (CPPC counters).

Any update on this ?

-- 
viresh

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

Re: [RFC 0/3] cpufreq: cppc: Add support for frequency invariance

[Ionela Voinescu]

Hi Viresh,

On Monday 05 Oct 2020 at 13:28:22 (+0530), Viresh Kumar wrote:
> On 27-08-20, 12:27, Ionela Voinescu wrote:
> > I am in the middle of unifying AMU counter and cpufreq invariance through
> > something like this, so if you like the idea and you don't think I'm
> > stepping too much on your toes with this, I can consider the usecase in
> > my (what should be) generic support. So in the end this might end up
> > being just a matter of adding a new invariance source (CPPC counters).
> 
> Any update on this ?
> 

I have some code for this, but not yet in the final state I wanted to
bring it to. The code has some dependencies/conflicts with the FFH support
and in small part with the new BL_SWITCHER patches so I wanted to get
those through first.

I'm in the middle of some distractions now, so probably it will take a
around two-three more weeks before I submit the code for review.

Sorry for the delay,
Ionela.

> -- 
> viresh

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