linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] arch_topology, ACPI: populate cpu capacity from CPPC
@ 2021-05-14  9:53 Ionela Voinescu
  2021-05-14  9:53 ` [PATCH 1/3] x86, ACPI: rename init_freq_invariance_cppc to arch_init_invariance_cppc Ionela Voinescu
                   ` (4 more replies)
  0 siblings, 5 replies; 13+ messages in thread
From: Ionela Voinescu @ 2021-05-14  9:53 UTC (permalink / raw)
  To: Sudeep Holla, Rafael J . Wysocki, Thomas Gleixner, Ingo Molnar,
	Giovanni Gherdovich, Catalin Marinas, Will Deacon,
	Valentin Schneider
  Cc: linux-kernel, linux-acpi, linux-arm-kernel

Hi all,

These are a few trivial patches to populate cpu capacity information
using performance information from ACPI's CPPC.

I've tied this functionality to the existing function
init_freq_invariance_cppc() called in acpi_cppc_processor_probe().
This function is renamed to a more generic arch_init_invariance_cppc().

The patches have been build tested on x86 and more thoroughly tested on
Juno R2 (arm64), which uses the new functionality, with the following
results:


root@ubuntu:~# dmesg | grep cpu_capacity
[    2.157494] init_cpu_capacity_cppc: CPU0 cpu_capacity=38300 (raw).
[    2.163699] init_cpu_capacity_cppc: CPU1 cpu_capacity=38300 (raw).
[    2.169899] init_cpu_capacity_cppc: CPU2 cpu_capacity=38300 (raw).
[    2.176098] init_cpu_capacity_cppc: CPU3 cpu_capacity=38300 (raw).
[    2.182296] init_cpu_capacity_cppc: CPU4 cpu_capacity=102400 (raw).
[    2.188581] init_cpu_capacity_cppc: CPU5 cpu_capacity=102400 (raw).
[    2.194867] cpu_capacity: capacity_scale=102400
[    2.199409] cpu_capacity: CPU0 cpu_capacity=383
[    2.203952] cpu_capacity: CPU1 cpu_capacity=383
[    2.208495] cpu_capacity: CPU2 cpu_capacity=383
[    2.213037] cpu_capacity: CPU3 cpu_capacity=383
[    2.217580] cpu_capacity: CPU4 cpu_capacity=1024
[    2.222209] cpu_capacity: CPU5 cpu_capacity=1024
[    2.226886] init_cpu_capacity_cppc: cpu_capacity initialization done

root@ubuntu:~# tail -n +1 /sys/devices/system/cpu/cpu*/cpu_capacity
==> /sys/devices/system/cpu/cpu0/cpu_capacity <==
383
==> /sys/devices/system/cpu/cpu1/cpu_capacity <==
383
==> /sys/devices/system/cpu/cpu2/cpu_capacity <==
383
==> /sys/devices/system/cpu/cpu3/cpu_capacity <==
383
==> /sys/devices/system/cpu/cpu4/cpu_capacity <==
1024
==> /sys/devices/system/cpu/cpu5/cpu_capacity <==
1024

All works as expected even if ACPI processor support is built as a
module.

Patches are based on v5.13-rc1.

Let me know what you think!

Thanks,
Ionela.

Ionela Voinescu (3):
  x86, ACPI: rename init_freq_invariance_cppc to
    arch_init_invariance_cppc
  arch_topology: obtain cpu capacity using information from CPPC
  arm64, topology: enable use of init_cpu_capacity_cppc()

 arch/arm64/include/asm/topology.h |  4 ++++
 arch/x86/include/asm/topology.h   |  2 +-
 drivers/acpi/cppc_acpi.c          |  6 ++---
 drivers/base/arch_topology.c      | 39 +++++++++++++++++++++++++++++++
 include/linux/arch_topology.h     |  4 ++++
 5 files changed, 51 insertions(+), 4 deletions(-)

-- 
2.29.2.dirty


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/3] x86, ACPI: rename init_freq_invariance_cppc to arch_init_invariance_cppc
  2021-05-14  9:53 [PATCH 0/3] arch_topology, ACPI: populate cpu capacity from CPPC Ionela Voinescu
@ 2021-05-14  9:53 ` Ionela Voinescu
  2021-05-14  9:53 ` [PATCH 2/3] arch_topology: obtain cpu capacity using information from CPPC Ionela Voinescu
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 13+ messages in thread
From: Ionela Voinescu @ 2021-05-14  9:53 UTC (permalink / raw)
  To: Sudeep Holla, Rafael J . Wysocki, Thomas Gleixner, Ingo Molnar,
	Giovanni Gherdovich, Catalin Marinas, Will Deacon,
	Valentin Schneider
  Cc: linux-kernel, linux-acpi, linux-arm-kernel

init_freq_invariance_cppc() was called in acpi_cppc_processor_probe(),
after CPU performance information and controls were populated from the
per-cpu _CPC objects.

But these _CPC objects provide information that helps with both CPU
(u-arch) and frequency invariance. Therefore, change the function name
to a more generic one, while adding the arch_ prefix, as this function
is expected to be defined differently by different architectures.

Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Giovanni Gherdovich <ggherdovich@suse.cz>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
---
 arch/x86/include/asm/topology.h | 2 +-
 drivers/acpi/cppc_acpi.c        | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/topology.h b/arch/x86/include/asm/topology.h
index 9239399e5491..61d73013cab8 100644
--- a/arch/x86/include/asm/topology.h
+++ b/arch/x86/include/asm/topology.h
@@ -220,7 +220,7 @@ static inline void arch_set_max_freq_ratio(bool turbo_disabled)
 
 #ifdef CONFIG_ACPI_CPPC_LIB
 void init_freq_invariance_cppc(void);
-#define init_freq_invariance_cppc init_freq_invariance_cppc
+#define arch_init_invariance_cppc init_freq_invariance_cppc
 #endif
 
 #endif /* _ASM_X86_TOPOLOGY_H */
diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index a4d4eebba1da..c211d77310e8 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -660,8 +660,8 @@ static bool is_cppc_supported(int revision, int num_ent)
  *	}
  */
 
-#ifndef init_freq_invariance_cppc
-static inline void init_freq_invariance_cppc(void) { }
+#ifndef arch_init_invariance_cppc
+static inline void arch_init_invariance_cppc(void) { }
 #endif
 
 /**
@@ -826,7 +826,7 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr)
 		goto out_free;
 	}
 
-	init_freq_invariance_cppc();
+	arch_init_invariance_cppc();
 
 	kfree(output.pointer);
 	return 0;
-- 
2.29.2.dirty


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 2/3] arch_topology: obtain cpu capacity using information from CPPC
  2021-05-14  9:53 [PATCH 0/3] arch_topology, ACPI: populate cpu capacity from CPPC Ionela Voinescu
  2021-05-14  9:53 ` [PATCH 1/3] x86, ACPI: rename init_freq_invariance_cppc to arch_init_invariance_cppc Ionela Voinescu
@ 2021-05-14  9:53 ` Ionela Voinescu
  2021-05-14 16:16   ` Dietmar Eggemann
  2021-05-18 13:12   ` Valentin Schneider
  2021-05-14  9:53 ` [PATCH 3/3] arm64, topology: enable use of init_cpu_capacity_cppc() Ionela Voinescu
                   ` (2 subsequent siblings)
  4 siblings, 2 replies; 13+ messages in thread
From: Ionela Voinescu @ 2021-05-14  9:53 UTC (permalink / raw)
  To: Sudeep Holla, Rafael J . Wysocki, Thomas Gleixner, Ingo Molnar,
	Giovanni Gherdovich, Catalin Marinas, Will Deacon,
	Valentin Schneider
  Cc: linux-kernel, linux-acpi, linux-arm-kernel

Define init_cpu_capacity_cppc() to use highest performance values
from _CPC objects to obtain and set maximum capacity information for
each CPU. acpi_cppc_processor_probe() is a good point at which to
trigger the initialization of CPU (u-arch) capacity values, as at this
point the highest performance values can be obtained from each CPU's
_CPC objects. Architectures can therefore use this functionality
through arch_init_invariance_cppc().

The performance scale used by CPPC is a unified scale for all CPUs in
the system. Therefore, by obtaining the raw highest performance values
from the _CPC objects, and normalizing them on the [0, 1024] capacity
scale, used by the task scheduler, we obtain the CPU capacity of each
CPU.

While an ACPI Notify(0x85) could alert about a change in the highest
performance value, which should in turn retrigger the CPU capacity
computations, this notification is not currently handled by the ACPI
processor driver. When supported, a call to arch_init_invariance_cppc()
would perform the update.

Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com>
Cc: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/base/arch_topology.c  | 39 +++++++++++++++++++++++++++++++++++
 include/linux/arch_topology.h |  4 ++++
 2 files changed, 43 insertions(+)

diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
index c1179edc0f3b..f710d64f125b 100644
--- a/drivers/base/arch_topology.c
+++ b/drivers/base/arch_topology.c
@@ -291,6 +291,45 @@ bool __init topology_parse_cpu_capacity(struct device_node *cpu_node, int cpu)
 	return !ret;
 }
 
+#ifdef CONFIG_ACPI_CPPC_LIB
+#include <acpi/cppc_acpi.h>
+
+void init_cpu_capacity_cppc(void)
+{
+	struct cppc_perf_caps perf_caps;
+	int cpu;
+
+	if (likely(acpi_disabled || !acpi_cpc_valid()))
+		return;
+
+	raw_capacity = kcalloc(num_possible_cpus(), sizeof(*raw_capacity),
+			       GFP_KERNEL);
+	if (!raw_capacity)
+		return;
+
+	for_each_possible_cpu(cpu) {
+		if (!cppc_get_perf_caps(cpu, &perf_caps)) {
+			raw_capacity[cpu] = perf_caps.highest_perf;
+			pr_debug("%s: CPU%d cpu_capacity=%u (raw).\n",
+				 __func__, cpu, raw_capacity[cpu]);
+		} else {
+			pr_err("%s: CPU%d missing highest performance.\n",
+				 __func__, cpu);
+			pr_err("%s: fallback to 1024 for all CPUs\n",
+				 __func__);
+			goto exit;
+		}
+	}
+
+	topology_normalize_cpu_scale();
+	schedule_work(&update_topology_flags_work);
+	pr_debug("%s: cpu_capacity initialization done\n", __func__);
+
+exit:
+	free_raw_capacity();
+}
+#endif
+
 #ifdef CONFIG_CPU_FREQ
 static cpumask_var_t cpus_to_visit;
 static void parsing_done_workfn(struct work_struct *work);
diff --git a/include/linux/arch_topology.h b/include/linux/arch_topology.h
index f180240dc95f..fbd829c3b7f7 100644
--- a/include/linux/arch_topology.h
+++ b/include/linux/arch_topology.h
@@ -11,6 +11,10 @@
 void topology_normalize_cpu_scale(void);
 int topology_update_cpu_topology(void);
 
+#ifdef CONFIG_ACPI_CPPC_LIB
+void init_cpu_capacity_cppc(void);
+#endif
+
 struct device_node;
 bool topology_parse_cpu_capacity(struct device_node *cpu_node, int cpu);
 
-- 
2.29.2.dirty


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 3/3] arm64, topology: enable use of init_cpu_capacity_cppc()
  2021-05-14  9:53 [PATCH 0/3] arch_topology, ACPI: populate cpu capacity from CPPC Ionela Voinescu
  2021-05-14  9:53 ` [PATCH 1/3] x86, ACPI: rename init_freq_invariance_cppc to arch_init_invariance_cppc Ionela Voinescu
  2021-05-14  9:53 ` [PATCH 2/3] arch_topology: obtain cpu capacity using information from CPPC Ionela Voinescu
@ 2021-05-14  9:53 ` Ionela Voinescu
  2021-05-14 10:35   ` Catalin Marinas
  2021-05-14 16:17   ` Dietmar Eggemann
  2021-05-18 13:12 ` [PATCH 0/3] arch_topology, ACPI: populate cpu capacity from CPPC Valentin Schneider
  2021-06-28 13:58 ` [tip: sched/core] sched/debug: Don't update sched_domain debug directories before sched_debug_init() tip-bot2 for Valentin Schneider
  4 siblings, 2 replies; 13+ messages in thread
From: Ionela Voinescu @ 2021-05-14  9:53 UTC (permalink / raw)
  To: Sudeep Holla, Rafael J . Wysocki, Thomas Gleixner, Ingo Molnar,
	Giovanni Gherdovich, Catalin Marinas, Will Deacon,
	Valentin Schneider
  Cc: linux-kernel, linux-acpi, linux-arm-kernel

Now that the arch topology driver provides a method of setting CPU
capacity values based on information on highest performance from CPPC,
use this functionality on arm64 platforms.

Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm64/include/asm/topology.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/include/asm/topology.h b/arch/arm64/include/asm/topology.h
index ec2db3419c41..c6ae6b55789c 100644
--- a/arch/arm64/include/asm/topology.h
+++ b/arch/arm64/include/asm/topology.h
@@ -24,6 +24,10 @@ void update_freq_counters_refs(void);
 #define arch_scale_freq_capacity topology_get_freq_scale
 #define arch_scale_freq_invariant topology_scale_freq_invariant
 
+#ifdef CONFIG_ACPI_CPPC_LIB
+#define arch_init_invariance_cppc init_cpu_capacity_cppc
+#endif
+
 /* Replace task scheduler's default cpu-invariant accounting */
 #define arch_scale_cpu_capacity topology_get_cpu_scale
 
-- 
2.29.2.dirty


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/3] arm64, topology: enable use of init_cpu_capacity_cppc()
  2021-05-14  9:53 ` [PATCH 3/3] arm64, topology: enable use of init_cpu_capacity_cppc() Ionela Voinescu
@ 2021-05-14 10:35   ` Catalin Marinas
  2021-05-14 16:17   ` Dietmar Eggemann
  1 sibling, 0 replies; 13+ messages in thread
From: Catalin Marinas @ 2021-05-14 10:35 UTC (permalink / raw)
  To: Ionela Voinescu
  Cc: Sudeep Holla, Rafael J . Wysocki, Thomas Gleixner, Ingo Molnar,
	Giovanni Gherdovich, Will Deacon, Valentin Schneider,
	linux-kernel, linux-acpi, linux-arm-kernel

On Fri, May 14, 2021 at 10:53:39AM +0100, Ionela Voinescu wrote:
> Now that the arch topology driver provides a method of setting CPU
> capacity values based on information on highest performance from CPPC,
> use this functionality on arm64 platforms.
> 
> Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/3] arch_topology: obtain cpu capacity using information from CPPC
  2021-05-14  9:53 ` [PATCH 2/3] arch_topology: obtain cpu capacity using information from CPPC Ionela Voinescu
@ 2021-05-14 16:16   ` Dietmar Eggemann
  2021-05-19  9:46     ` Ionela Voinescu
  2021-05-18 13:12   ` Valentin Schneider
  1 sibling, 1 reply; 13+ messages in thread
From: Dietmar Eggemann @ 2021-05-14 16:16 UTC (permalink / raw)
  To: Ionela Voinescu, Sudeep Holla, Rafael J . Wysocki,
	Thomas Gleixner, Ingo Molnar, Giovanni Gherdovich,
	Catalin Marinas, Will Deacon, Valentin Schneider
  Cc: linux-kernel, linux-acpi, linux-arm-kernel

On 14/05/2021 11:53, Ionela Voinescu wrote:

[...]

> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> index c1179edc0f3b..f710d64f125b 100644
> --- a/drivers/base/arch_topology.c
> +++ b/drivers/base/arch_topology.c
> @@ -291,6 +291,45 @@ bool __init topology_parse_cpu_capacity(struct device_node *cpu_node, int cpu)
>  	return !ret;
>  }
>  
> +#ifdef CONFIG_ACPI_CPPC_LIB
> +#include <acpi/cppc_acpi.h>

init_cpu_capacity_cppc() shares a lot of functionality with the existing
DT/CPUfreq-based approach (topology_parse_cpu_capacity(),
register_cpufreq_notifier(), init_cpu_capacity_callback()). It looks
like that the different ways of invocation (two steps per cpu vs. one
step for all cpus) makes it hard to restructure the code to create more
common bits.

> +void init_cpu_capacity_cppc(void)
> +{
> +	struct cppc_perf_caps perf_caps;
> +	int cpu;
> +
> +	if (likely(acpi_disabled || !acpi_cpc_valid()))

likely(acpi_disabled) ?

> +		return;
> +
> +	raw_capacity = kcalloc(num_possible_cpus(), sizeof(*raw_capacity),
> +			       GFP_KERNEL);
> +	if (!raw_capacity)
> +		return;
> +
> +	for_each_possible_cpu(cpu) {
> +		if (!cppc_get_perf_caps(cpu, &perf_caps)) {
> +			raw_capacity[cpu] = perf_caps.highest_perf;
> +			pr_debug("%s: CPU%d cpu_capacity=%u (raw).\n",
> +				 __func__, cpu, raw_capacity[cpu]);

There is quite a variety in the layout of the pr_xxx() log messages in
this file. Originally the 'cpu_capacity:' was used to indicate that this
log is from drivers/base/arch_topology.c. Now the GCC __func__
identifier is used. Maybe this can be aligned better? Especially since
the functionality used in the existing DT-driven and now in the new
CPPC-driven functionality is quite similar. Debugging is so much easier
with consistent log strings.


> +		} else {
> +			pr_err("%s: CPU%d missing highest performance.\n",
> +				 __func__, cpu);
> +			pr_err("%s: fallback to 1024 for all CPUs\n",
> +				 __func__);
> +			goto exit;
> +		}
> +	}
> +
> +	topology_normalize_cpu_scale();
> +	schedule_work(&update_topology_flags_work);
> +	pr_debug("%s: cpu_capacity initialization done\n", __func__);
> +
> +exit:
> +	free_raw_capacity();
> +}
> +#endif

In case a system has CONFIG_ACPI_CPPC_LIB what does this mean for the
DT-based approach via `register_cpufreq_notifier()`?

Looks like we rely on:

376 static int __init register_cpufreq_notifier(void)
...
385         if (!acpi_disabled || ...)
386                 return -EINVAL;

to disable the CPUfreq part of the DT/CPUfreq-based approach on an ACPI
system.

[...]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/3] arm64, topology: enable use of init_cpu_capacity_cppc()
  2021-05-14  9:53 ` [PATCH 3/3] arm64, topology: enable use of init_cpu_capacity_cppc() Ionela Voinescu
  2021-05-14 10:35   ` Catalin Marinas
@ 2021-05-14 16:17   ` Dietmar Eggemann
  2021-05-19  9:48     ` Ionela Voinescu
  1 sibling, 1 reply; 13+ messages in thread
From: Dietmar Eggemann @ 2021-05-14 16:17 UTC (permalink / raw)
  To: Ionela Voinescu, Sudeep Holla, Rafael J . Wysocki,
	Thomas Gleixner, Ingo Molnar, Giovanni Gherdovich,
	Catalin Marinas, Will Deacon, Valentin Schneider
  Cc: linux-kernel, linux-acpi, linux-arm-kernel

On 14/05/2021 11:53, Ionela Voinescu wrote:

[...]

> +#ifdef CONFIG_ACPI_CPPC_LIB
> +#define arch_init_invariance_cppc init_cpu_capacity_cppc
> +#endif

The prefix `topology_` was meant to indicate that those functions come
from drivers/base/arch_topology.c. You probably refrained from it since

topology_init_cpu_capacity_cppc()

is a pretty long function name ... Still more consistent though.

[...]


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/3] arch_topology, ACPI: populate cpu capacity from CPPC
  2021-05-14  9:53 [PATCH 0/3] arch_topology, ACPI: populate cpu capacity from CPPC Ionela Voinescu
                   ` (2 preceding siblings ...)
  2021-05-14  9:53 ` [PATCH 3/3] arm64, topology: enable use of init_cpu_capacity_cppc() Ionela Voinescu
@ 2021-05-18 13:12 ` Valentin Schneider
  2021-05-19  9:51   ` Ionela Voinescu
  2021-06-28 13:58 ` [tip: sched/core] sched/debug: Don't update sched_domain debug directories before sched_debug_init() tip-bot2 for Valentin Schneider
  4 siblings, 1 reply; 13+ messages in thread
From: Valentin Schneider @ 2021-05-18 13:12 UTC (permalink / raw)
  To: Ionela Voinescu, Sudeep Holla, Rafael J . Wysocki,
	Thomas Gleixner, Ingo Molnar, Giovanni Gherdovich,
	Catalin Marinas, Will Deacon
  Cc: linux-kernel, linux-acpi, linux-arm-kernel

Hi,

On 14/05/21 10:53, Ionela Voinescu wrote:
> Hi all,
>
> These are a few trivial patches to populate cpu capacity information
> using performance information from ACPI's CPPC.
>
> I've tied this functionality to the existing function
> init_freq_invariance_cppc() called in acpi_cppc_processor_probe().
> This function is renamed to a more generic arch_init_invariance_cppc().
>
> The patches have been build tested on x86 and more thoroughly tested on
> Juno R2 (arm64), which uses the new functionality, with the following
> results:
>
>
> root@ubuntu:~# dmesg | grep cpu_capacity
> [    2.157494] init_cpu_capacity_cppc: CPU0 cpu_capacity=38300 (raw).
> [    2.163699] init_cpu_capacity_cppc: CPU1 cpu_capacity=38300 (raw).
> [    2.169899] init_cpu_capacity_cppc: CPU2 cpu_capacity=38300 (raw).
> [    2.176098] init_cpu_capacity_cppc: CPU3 cpu_capacity=38300 (raw).
> [    2.182296] init_cpu_capacity_cppc: CPU4 cpu_capacity=102400 (raw).
> [    2.188581] init_cpu_capacity_cppc: CPU5 cpu_capacity=102400 (raw).
> [    2.194867] cpu_capacity: capacity_scale=102400
> [    2.199409] cpu_capacity: CPU0 cpu_capacity=383
> [    2.203952] cpu_capacity: CPU1 cpu_capacity=383
> [    2.208495] cpu_capacity: CPU2 cpu_capacity=383
> [    2.213037] cpu_capacity: CPU3 cpu_capacity=383
> [    2.217580] cpu_capacity: CPU4 cpu_capacity=1024
> [    2.222209] cpu_capacity: CPU5 cpu_capacity=1024
> [    2.226886] init_cpu_capacity_cppc: cpu_capacity initialization done
>
> root@ubuntu:~# tail -n +1 /sys/devices/system/cpu/cpu*/cpu_capacity
> ==> /sys/devices/system/cpu/cpu0/cpu_capacity <==
> 383
> ==> /sys/devices/system/cpu/cpu1/cpu_capacity <==
> 383
> ==> /sys/devices/system/cpu/cpu2/cpu_capacity <==
> 383
> ==> /sys/devices/system/cpu/cpu3/cpu_capacity <==
> 383
> ==> /sys/devices/system/cpu/cpu4/cpu_capacity <==
> 1024
> ==> /sys/devices/system/cpu/cpu5/cpu_capacity <==
> 1024
>
> All works as expected even if ACPI processor support is built as a
> module.
>

Tested on my Ampere eMAG; this all seems to work fine except for some
scheduler debug stuff that gets confused; see

  http://lore.kernel.org/r/20210518130725.3563132-1-valentin.schneider@arm.com

With that in mind:
Tested-by: Valentin Schneider <valentin.schneider@arm.com>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/3] arch_topology: obtain cpu capacity using information from CPPC
  2021-05-14  9:53 ` [PATCH 2/3] arch_topology: obtain cpu capacity using information from CPPC Ionela Voinescu
  2021-05-14 16:16   ` Dietmar Eggemann
@ 2021-05-18 13:12   ` Valentin Schneider
  1 sibling, 0 replies; 13+ messages in thread
From: Valentin Schneider @ 2021-05-18 13:12 UTC (permalink / raw)
  To: Ionela Voinescu, Sudeep Holla, Rafael J . Wysocki,
	Thomas Gleixner, Ingo Molnar, Giovanni Gherdovich,
	Catalin Marinas, Will Deacon
  Cc: linux-kernel, linux-acpi, linux-arm-kernel

On 14/05/21 10:53, Ionela Voinescu wrote:
> +#ifdef CONFIG_ACPI_CPPC_LIB
> +#include <acpi/cppc_acpi.h>
> +
> +void init_cpu_capacity_cppc(void)
> +{
> +	struct cppc_perf_caps perf_caps;
> +	int cpu;
> +
> +	if (likely(acpi_disabled || !acpi_cpc_valid()))
> +		return;
> +
> +	raw_capacity = kcalloc(num_possible_cpus(), sizeof(*raw_capacity),
> +			       GFP_KERNEL);

Per the below loop, the memory shouldn't need to be cleared.

> +	if (!raw_capacity)
> +		return;
> +
> +	for_each_possible_cpu(cpu) {
> +		if (!cppc_get_perf_caps(cpu, &perf_caps)) {
> +			raw_capacity[cpu] = perf_caps.highest_perf;
> +			pr_debug("%s: CPU%d cpu_capacity=%u (raw).\n",
> +				 __func__, cpu, raw_capacity[cpu]);
> +		} else {
> +			pr_err("%s: CPU%d missing highest performance.\n",
> +				 __func__, cpu);
> +			pr_err("%s: fallback to 1024 for all CPUs\n",
> +				 __func__);
> +			goto exit;
> +		}
> +	}
> +
> +	topology_normalize_cpu_scale();
> +	schedule_work(&update_topology_flags_work);
> +	pr_debug("%s: cpu_capacity initialization done\n", __func__);
> +
> +exit:
> +	free_raw_capacity();
> +}
> +#endif

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/3] arch_topology: obtain cpu capacity using information from CPPC
  2021-05-14 16:16   ` Dietmar Eggemann
@ 2021-05-19  9:46     ` Ionela Voinescu
  0 siblings, 0 replies; 13+ messages in thread
From: Ionela Voinescu @ 2021-05-19  9:46 UTC (permalink / raw)
  To: Dietmar Eggemann
  Cc: Sudeep Holla, Rafael J . Wysocki, Thomas Gleixner, Ingo Molnar,
	Giovanni Gherdovich, Catalin Marinas, Will Deacon,
	Valentin Schneider, linux-kernel, linux-acpi, linux-arm-kernel

Hi Dietmar,

Many thanks for the review!

On Friday 14 May 2021 at 18:16:50 (+0200), Dietmar Eggemann wrote:
> On 14/05/2021 11:53, Ionela Voinescu wrote:
> 
> [...]
> 
> > diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> > index c1179edc0f3b..f710d64f125b 100644
> > --- a/drivers/base/arch_topology.c
> > +++ b/drivers/base/arch_topology.c
> > @@ -291,6 +291,45 @@ bool __init topology_parse_cpu_capacity(struct device_node *cpu_node, int cpu)
> >  	return !ret;
> >  }
> >  
> > +#ifdef CONFIG_ACPI_CPPC_LIB
> > +#include <acpi/cppc_acpi.h>
> 
> init_cpu_capacity_cppc() shares a lot of functionality with the existing
> DT/CPUfreq-based approach (topology_parse_cpu_capacity(),
> register_cpufreq_notifier(), init_cpu_capacity_callback()). It looks
> like that the different ways of invocation (two steps per cpu vs. one
> step for all cpus) makes it hard to restructure the code to create more
> common bits.
> 

Yes, I looked at ways to reuse more of the DT-based functionality, but I
did not find a better way. We reuse the normalization functionality and
the rebuild of the scheduling domains, but I'm not sure there's room for
much more, as the rest is specific to each source of capacity
information, DT+cpufreq or CPPC.

I also did not want to tie the new CPPC based functionality to cpufreq.
While the DT-based path needs cpufreq policies initialized as it needs
information on maximum frequency to obtain capacity, this is not needed
in the new code. This results in simpler code and ensures support even
for systems that do not have a cpufreq driver.

> > +void init_cpu_capacity_cppc(void)
> > +{
> > +	struct cppc_perf_caps perf_caps;
> > +	int cpu;
> > +
> > +	if (likely(acpi_disabled || !acpi_cpc_valid()))
> 
> likely(acpi_disabled) ?
> 

likely (acpi_disabled || !acpi_cpc_valid()) :)

It's "likely" useless, but this function gets called for each CPU from
acpi_cppc_processor_probe(), but it only continues with setting the
cpu_scale after all possible CPUs have their _CPC objects populated.

Therefore it's a lot more likely we return here.

> > +		return;
> > +
> > +	raw_capacity = kcalloc(num_possible_cpus(), sizeof(*raw_capacity),
> > +			       GFP_KERNEL);
> > +	if (!raw_capacity)
> > +		return;
> > +
> > +	for_each_possible_cpu(cpu) {
> > +		if (!cppc_get_perf_caps(cpu, &perf_caps)) {
> > +			raw_capacity[cpu] = perf_caps.highest_perf;
> > +			pr_debug("%s: CPU%d cpu_capacity=%u (raw).\n",
> > +				 __func__, cpu, raw_capacity[cpu]);
> 
> There is quite a variety in the layout of the pr_xxx() log messages in
> this file. Originally the 'cpu_capacity:' was used to indicate that this
> log is from drivers/base/arch_topology.c. Now the GCC __func__
> identifier is used. Maybe this can be aligned better? Especially since
> the functionality used in the existing DT-driven and now in the new
> CPPC-driven functionality is quite similar. Debugging is so much easier
> with consistent log strings.
> 

Right! My intention was to keep the prints relatively similar, but my
wanting to reduce the line length got the better of me. I'll keep the
prints consistent.

> 
> > +		} else {
> > +			pr_err("%s: CPU%d missing highest performance.\n",
> > +				 __func__, cpu);
> > +			pr_err("%s: fallback to 1024 for all CPUs\n",
> > +				 __func__);
> > +			goto exit;
> > +		}
> > +	}
> > +
> > +	topology_normalize_cpu_scale();
> > +	schedule_work(&update_topology_flags_work);
> > +	pr_debug("%s: cpu_capacity initialization done\n", __func__);
> > +
> > +exit:
> > +	free_raw_capacity();
> > +}
> > +#endif
> 
> In case a system has CONFIG_ACPI_CPPC_LIB what does this mean for the
> DT-based approach via `register_cpufreq_notifier()`?
> 

CONFIG_ACPI_CPPC_LIB is enabled by default on arm64. This only ensures
that we have the functionality to parse and work with the ACPI _CPC
objects and it does not guarantee that ACPI will be used.

> Looks like we rely on:
> 
> 376 static int __init register_cpufreq_notifier(void)
> ...
> 385         if (!acpi_disabled || ...)
> 386                 return -EINVAL;
> 
> to disable the CPUfreq part of the DT/CPUfreq-based approach on an ACPI
> system.
> 

It's both acpi_disabled and raw_capacity that guard the DT path. You
need both to not use ACPI (therefore using DT) and to have valid
capacity-dmips-mhz in DT for the cpufreq notifier that will eventually
populate the cpu_scale variables to be registered.

Thank you,
Ionela.
> [...]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/3] arm64, topology: enable use of init_cpu_capacity_cppc()
  2021-05-14 16:17   ` Dietmar Eggemann
@ 2021-05-19  9:48     ` Ionela Voinescu
  0 siblings, 0 replies; 13+ messages in thread
From: Ionela Voinescu @ 2021-05-19  9:48 UTC (permalink / raw)
  To: Dietmar Eggemann
  Cc: Sudeep Holla, Rafael J . Wysocki, Thomas Gleixner, Ingo Molnar,
	Giovanni Gherdovich, Catalin Marinas, Will Deacon,
	Valentin Schneider, linux-kernel, linux-acpi, linux-arm-kernel

On Friday 14 May 2021 at 18:17:00 (+0200), Dietmar Eggemann wrote:
> On 14/05/2021 11:53, Ionela Voinescu wrote:
> 
> [...]
> 
> > +#ifdef CONFIG_ACPI_CPPC_LIB
> > +#define arch_init_invariance_cppc init_cpu_capacity_cppc
> > +#endif
> 
> The prefix `topology_` was meant to indicate that those functions come
> from drivers/base/arch_topology.c. You probably refrained from it since
> 
> topology_init_cpu_capacity_cppc()
> 
> is a pretty long function name ... Still more consistent though.
> 

I'll rename it, thanks!

Ionela.

> [...]
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/3] arch_topology, ACPI: populate cpu capacity from CPPC
  2021-05-18 13:12 ` [PATCH 0/3] arch_topology, ACPI: populate cpu capacity from CPPC Valentin Schneider
@ 2021-05-19  9:51   ` Ionela Voinescu
  0 siblings, 0 replies; 13+ messages in thread
From: Ionela Voinescu @ 2021-05-19  9:51 UTC (permalink / raw)
  To: Valentin Schneider
  Cc: Sudeep Holla, Rafael J . Wysocki, Thomas Gleixner, Ingo Molnar,
	Giovanni Gherdovich, Catalin Marinas, Will Deacon, linux-kernel,
	linux-acpi, linux-arm-kernel

Hi Valentin,

On Tuesday 18 May 2021 at 14:12:03 (+0100), Valentin Schneider wrote:
> Hi,
> 
> On 14/05/21 10:53, Ionela Voinescu wrote:
> > Hi all,
> >
> > These are a few trivial patches to populate cpu capacity information
> > using performance information from ACPI's CPPC.
> >
> > I've tied this functionality to the existing function
> > init_freq_invariance_cppc() called in acpi_cppc_processor_probe().
> > This function is renamed to a more generic arch_init_invariance_cppc().
> >
> > The patches have been build tested on x86 and more thoroughly tested on
> > Juno R2 (arm64), which uses the new functionality, with the following
> > results:
> >
> >
> > root@ubuntu:~# dmesg | grep cpu_capacity
> > [    2.157494] init_cpu_capacity_cppc: CPU0 cpu_capacity=38300 (raw).
> > [    2.163699] init_cpu_capacity_cppc: CPU1 cpu_capacity=38300 (raw).
> > [    2.169899] init_cpu_capacity_cppc: CPU2 cpu_capacity=38300 (raw).
> > [    2.176098] init_cpu_capacity_cppc: CPU3 cpu_capacity=38300 (raw).
> > [    2.182296] init_cpu_capacity_cppc: CPU4 cpu_capacity=102400 (raw).
> > [    2.188581] init_cpu_capacity_cppc: CPU5 cpu_capacity=102400 (raw).
> > [    2.194867] cpu_capacity: capacity_scale=102400
> > [    2.199409] cpu_capacity: CPU0 cpu_capacity=383
> > [    2.203952] cpu_capacity: CPU1 cpu_capacity=383
> > [    2.208495] cpu_capacity: CPU2 cpu_capacity=383
> > [    2.213037] cpu_capacity: CPU3 cpu_capacity=383
> > [    2.217580] cpu_capacity: CPU4 cpu_capacity=1024
> > [    2.222209] cpu_capacity: CPU5 cpu_capacity=1024
> > [    2.226886] init_cpu_capacity_cppc: cpu_capacity initialization done
> >
> > root@ubuntu:~# tail -n +1 /sys/devices/system/cpu/cpu*/cpu_capacity
> > ==> /sys/devices/system/cpu/cpu0/cpu_capacity <==
> > 383
> > ==> /sys/devices/system/cpu/cpu1/cpu_capacity <==
> > 383
> > ==> /sys/devices/system/cpu/cpu2/cpu_capacity <==
> > 383
> > ==> /sys/devices/system/cpu/cpu3/cpu_capacity <==
> > 383
> > ==> /sys/devices/system/cpu/cpu4/cpu_capacity <==
> > 1024
> > ==> /sys/devices/system/cpu/cpu5/cpu_capacity <==
> > 1024
> >
> > All works as expected even if ACPI processor support is built as a
> > module.
> >
> 
> Tested on my Ampere eMAG; this all seems to work fine except for some
> scheduler debug stuff that gets confused; see
> 
>   http://lore.kernel.org/r/20210518130725.3563132-1-valentin.schneider@arm.com
> 
> With that in mind:
> Tested-by: Valentin Schneider <valentin.schneider@arm.com>

Many thanks for testing and fixing the debugfs problem. I'll take a look
over your patch.

Ionela.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [tip: sched/core] sched/debug: Don't update sched_domain debug directories before sched_debug_init()
  2021-05-14  9:53 [PATCH 0/3] arch_topology, ACPI: populate cpu capacity from CPPC Ionela Voinescu
                   ` (3 preceding siblings ...)
  2021-05-18 13:12 ` [PATCH 0/3] arch_topology, ACPI: populate cpu capacity from CPPC Valentin Schneider
@ 2021-06-28 13:58 ` tip-bot2 for Valentin Schneider
  4 siblings, 0 replies; 13+ messages in thread
From: tip-bot2 for Valentin Schneider @ 2021-06-28 13:58 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: Valentin Schneider, Peter Zijlstra (Intel), x86, linux-kernel

The following commit has been merged into the sched/core branch of tip:

Commit-ID:     459b09b5a3254008b63382bf41a9b36d0b590f57
Gitweb:        https://git.kernel.org/tip/459b09b5a3254008b63382bf41a9b36d0b590f57
Author:        Valentin Schneider <valentin.schneider@arm.com>
AuthorDate:    Tue, 18 May 2021 14:07:25 +01:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Mon, 28 Jun 2021 15:42:24 +02:00

sched/debug: Don't update sched_domain debug directories before sched_debug_init()

Since CPU capacity asymmetry can stem purely from maximum frequency
differences (e.g. Pixel 1), a rebuild of the scheduler topology can be
issued upon loading cpufreq, see:

  arch_topology.c::init_cpu_capacity_callback()

Turns out that if this rebuild happens *before* sched_debug_init() is
run (which is a late initcall), we end up messing up the sched_domain debug
directory: passing a NULL parent to debugfs_create_dir() ends up creating
the directory at the debugfs root, which in this case creates
/sys/kernel/debug/domains (instead of /sys/kernel/debug/sched/domains).

This currently doesn't happen on asymmetric systems which use cpufreq-scpi
or cpufreq-dt drivers, as those are loaded via
deferred_probe_initcall() (it is also a late initcall, but appears to be
ordered *after* sched_debug_init()).

Ionela has been working on detecting maximum frequency asymmetry via ACPI,
and that actually happens via a *device* initcall, thus before
sched_debug_init(), and causes the aforementionned debugfs mayhem.

One option would be to punt sched_debug_init() down to
fs_initcall_sync(). Preventing update_sched_domain_debugfs() from running
before sched_debug_init() appears to be the safer option.

Fixes: 3b87f136f8fc ("sched,debug: Convert sysctl sched_domains to debugfs")
Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: http://lore.kernel.org/r/20210514095339.12979-1-ionela.voinescu@arm.com
---
 kernel/sched/debug.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/kernel/sched/debug.c b/kernel/sched/debug.c
index 0c5ec27..7e08e3d 100644
--- a/kernel/sched/debug.c
+++ b/kernel/sched/debug.c
@@ -388,6 +388,13 @@ void update_sched_domain_debugfs(void)
 {
 	int cpu, i;
 
+	/*
+	 * This can unfortunately be invoked before sched_debug_init() creates
+	 * the debug directory. Don't touch sd_sysctl_cpus until then.
+	 */
+	if (!debugfs_sched)
+		return;
+
 	if (!cpumask_available(sd_sysctl_cpus)) {
 		if (!alloc_cpumask_var(&sd_sysctl_cpus, GFP_KERNEL))
 			return;

^ permalink raw reply related	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2021-06-28 13:58 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-14  9:53 [PATCH 0/3] arch_topology, ACPI: populate cpu capacity from CPPC Ionela Voinescu
2021-05-14  9:53 ` [PATCH 1/3] x86, ACPI: rename init_freq_invariance_cppc to arch_init_invariance_cppc Ionela Voinescu
2021-05-14  9:53 ` [PATCH 2/3] arch_topology: obtain cpu capacity using information from CPPC Ionela Voinescu
2021-05-14 16:16   ` Dietmar Eggemann
2021-05-19  9:46     ` Ionela Voinescu
2021-05-18 13:12   ` Valentin Schneider
2021-05-14  9:53 ` [PATCH 3/3] arm64, topology: enable use of init_cpu_capacity_cppc() Ionela Voinescu
2021-05-14 10:35   ` Catalin Marinas
2021-05-14 16:17   ` Dietmar Eggemann
2021-05-19  9:48     ` Ionela Voinescu
2021-05-18 13:12 ` [PATCH 0/3] arch_topology, ACPI: populate cpu capacity from CPPC Valentin Schneider
2021-05-19  9:51   ` Ionela Voinescu
2021-06-28 13:58 ` [tip: sched/core] sched/debug: Don't update sched_domain debug directories before sched_debug_init() tip-bot2 for Valentin Schneider

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).