| Message ID | 20241104205446.3874509-1-superm1@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] ACPI: processor: Move arch_init_invariance_cppc() call later | expand |
On Mon, Nov 4, 2024 at 9:54 PM Mario Limonciello <superm1@kernel.org> wrote: > > From: Mario Limonciello <mario.limonciello@amd.com> > > arch_init_invariance_cppc() is called at the end of > acpi_cppc_processor_probe() in order to configure frequency invariance > based upon the values from _CPC. > > This however doesn't work on AMD CPPC shared memory designs that have > AMD preferred cores enabled because _CPC needs to be analyzed from all > cores to judge if preferred cores are enabled. > > This issue manifests to users as a warning since commit 21fb59ab4b97 > ("ACPI: CPPC: Adjust debug messages in amd_set_max_freq_ratio() to warn"): > ``` > Could not retrieve highest performance (-19) > ``` > > However the warning isn't the cause of this, it was actually > commit 279f838a61f9 ("x86/amd: Detect preferred cores in > amd_get_boost_ratio_numerator()") which exposed the issue. > > To fix this problem, change arch_init_invariance_cppc() into a new weak > symbol that is called at the end of acpi_processor_driver_init(). > Each architecture that supports it can declare the symbol to override > the weak one. > > Fixes: 279f838a61f9 ("x86/amd: Detect preferred cores in amd_get_boost_ratio_numerator()") > Reported-by: Ivan Shapovalov <intelfx@intelfx.name> > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219431 > Tested-by: Oleksandr Natalenko <oleksandr@natalenko.name> > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > --- > v3: > * Weak symbol instead of macro to help riscv build failure > * Update commit message > * Add comment > --- > arch/arm64/include/asm/topology.h | 2 +- > arch/x86/include/asm/topology.h | 2 +- > drivers/acpi/cppc_acpi.c | 6 ------ > drivers/acpi/processor_driver.c | 9 +++++++++ > include/acpi/processor.h | 2 ++ > 5 files changed, 13 insertions(+), 8 deletions(-) > > diff --git a/arch/arm64/include/asm/topology.h b/arch/arm64/include/asm/topology.h > index 5fc3af9f8f29b..8a1860877967e 100644 > --- a/arch/arm64/include/asm/topology.h > +++ b/arch/arm64/include/asm/topology.h > @@ -27,7 +27,7 @@ void update_freq_counters_refs(void); > #define arch_scale_freq_ref topology_get_freq_ref > > #ifdef CONFIG_ACPI_CPPC_LIB > -#define arch_init_invariance_cppc topology_init_cpu_capacity_cppc > +#define acpi_processor_init_invariance_cppc topology_init_cpu_capacity_cppc > #endif > > /* Replace task scheduler's default cpu-invariant accounting */ > diff --git a/arch/x86/include/asm/topology.h b/arch/x86/include/asm/topology.h > index aef70336d6247..0fb705524aeaa 100644 > --- a/arch/x86/include/asm/topology.h > +++ b/arch/x86/include/asm/topology.h > @@ -307,7 +307,7 @@ extern void arch_scale_freq_tick(void); > > #ifdef CONFIG_ACPI_CPPC_LIB > void init_freq_invariance_cppc(void); > -#define arch_init_invariance_cppc init_freq_invariance_cppc > +#define acpi_processor_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 1a40f0514eaa3..5c0cc7aae8726 100644 > --- a/drivers/acpi/cppc_acpi.c > +++ b/drivers/acpi/cppc_acpi.c > @@ -671,10 +671,6 @@ static int pcc_data_alloc(int pcc_ss_id) > * ) > */ > > -#ifndef arch_init_invariance_cppc > -static inline void arch_init_invariance_cppc(void) { } > -#endif > - > /** > * acpi_cppc_processor_probe - Search for per CPU _CPC objects. > * @pr: Ptr to acpi_processor containing this CPU's logical ID. > @@ -905,8 +901,6 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr) > goto out_free; > } > > - arch_init_invariance_cppc(); > - > kfree(output.pointer); > return 0; > > diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c > index cb52dd000b958..3b281bc1e73c3 100644 > --- a/drivers/acpi/processor_driver.c > +++ b/drivers/acpi/processor_driver.c > @@ -237,6 +237,9 @@ static struct notifier_block acpi_processor_notifier_block = { > .notifier_call = acpi_processor_notifier, > }; > > +void __weak acpi_processor_init_invariance_cppc(void) > +{ } Does this actually work if acpi_processor_init_invariance_cppc is a macro? How does the compiler know that it needs to use init_freq_invariance_cppc() instead of this? It would work if a __weak definition of init_freq_invariance_cppc() was present. > + > /* > * We keep the driver loaded even when ACPI is not running. > * This is needed for the powernow-k8 driver, that works even without > @@ -270,6 +273,12 @@ static int __init acpi_processor_driver_init(void) > NULL, acpi_soft_cpu_dead); > > acpi_processor_throttling_init(); > + > + /* > + * Frequency invariance calculations on AMD platforms can't be run until > + * after acpi_cppc_processor_probe() has been called for all online CPUs. > + */ > + acpi_processor_init_invariance_cppc(); > return 0; > err: > driver_unregister(&acpi_processor_driver); > diff --git a/include/acpi/processor.h b/include/acpi/processor.h > index e6f6074eadbf3..a17e97e634a68 100644 > --- a/include/acpi/processor.h > +++ b/include/acpi/processor.h > @@ -465,4 +465,6 @@ extern int acpi_processor_ffh_lpi_probe(unsigned int cpu); > extern int acpi_processor_ffh_lpi_enter(struct acpi_lpi_state *lpi); > #endif > > +void acpi_processor_init_invariance_cppc(void); > + > #endif > > base-commit: 6db936d4ac0fe281af48b4d1ebf69b1523bbac31 > -- > 2.43.0 >
On Mon, Nov 4, 2024 at 10:14 PM Mario Limonciello <superm1@kernel.org> wrote: > > On 11/4/2024 15:10, Rafael J. Wysocki wrote: > > On Mon, Nov 4, 2024 at 9:54 PM Mario Limonciello <superm1@kernel.org> wrote: > >> > >> From: Mario Limonciello <mario.limonciello@amd.com> > >> > >> arch_init_invariance_cppc() is called at the end of > >> acpi_cppc_processor_probe() in order to configure frequency invariance > >> based upon the values from _CPC. > >> > >> This however doesn't work on AMD CPPC shared memory designs that have > >> AMD preferred cores enabled because _CPC needs to be analyzed from all > >> cores to judge if preferred cores are enabled. > >> > >> This issue manifests to users as a warning since commit 21fb59ab4b97 > >> ("ACPI: CPPC: Adjust debug messages in amd_set_max_freq_ratio() to warn"): > >> ``` > >> Could not retrieve highest performance (-19) > >> ``` > >> > >> However the warning isn't the cause of this, it was actually > >> commit 279f838a61f9 ("x86/amd: Detect preferred cores in > >> amd_get_boost_ratio_numerator()") which exposed the issue. > >> > >> To fix this problem, change arch_init_invariance_cppc() into a new weak > >> symbol that is called at the end of acpi_processor_driver_init(). > >> Each architecture that supports it can declare the symbol to override > >> the weak one. > >> > >> Fixes: 279f838a61f9 ("x86/amd: Detect preferred cores in amd_get_boost_ratio_numerator()") > >> Reported-by: Ivan Shapovalov <intelfx@intelfx.name> > >> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219431 > >> Tested-by: Oleksandr Natalenko <oleksandr@natalenko.name> > >> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > >> --- > >> v3: > >> * Weak symbol instead of macro to help riscv build failure > >> * Update commit message > >> * Add comment > >> --- > >> arch/arm64/include/asm/topology.h | 2 +- > >> arch/x86/include/asm/topology.h | 2 +- > >> drivers/acpi/cppc_acpi.c | 6 ------ > >> drivers/acpi/processor_driver.c | 9 +++++++++ > >> include/acpi/processor.h | 2 ++ > >> 5 files changed, 13 insertions(+), 8 deletions(-) > >> > >> diff --git a/arch/arm64/include/asm/topology.h b/arch/arm64/include/asm/topology.h > >> index 5fc3af9f8f29b..8a1860877967e 100644 > >> --- a/arch/arm64/include/asm/topology.h > >> +++ b/arch/arm64/include/asm/topology.h > >> @@ -27,7 +27,7 @@ void update_freq_counters_refs(void); > >> #define arch_scale_freq_ref topology_get_freq_ref > >> > >> #ifdef CONFIG_ACPI_CPPC_LIB > >> -#define arch_init_invariance_cppc topology_init_cpu_capacity_cppc > >> +#define acpi_processor_init_invariance_cppc topology_init_cpu_capacity_cppc > >> #endif > >> > >> /* Replace task scheduler's default cpu-invariant accounting */ > >> diff --git a/arch/x86/include/asm/topology.h b/arch/x86/include/asm/topology.h > >> index aef70336d6247..0fb705524aeaa 100644 > >> --- a/arch/x86/include/asm/topology.h > >> +++ b/arch/x86/include/asm/topology.h > >> @@ -307,7 +307,7 @@ extern void arch_scale_freq_tick(void); > >> > >> #ifdef CONFIG_ACPI_CPPC_LIB > >> void init_freq_invariance_cppc(void); > >> -#define arch_init_invariance_cppc init_freq_invariance_cppc > >> +#define acpi_processor_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 1a40f0514eaa3..5c0cc7aae8726 100644 > >> --- a/drivers/acpi/cppc_acpi.c > >> +++ b/drivers/acpi/cppc_acpi.c > >> @@ -671,10 +671,6 @@ static int pcc_data_alloc(int pcc_ss_id) > >> * ) > >> */ > >> > >> -#ifndef arch_init_invariance_cppc > >> -static inline void arch_init_invariance_cppc(void) { } > >> -#endif > >> - > >> /** > >> * acpi_cppc_processor_probe - Search for per CPU _CPC objects. > >> * @pr: Ptr to acpi_processor containing this CPU's logical ID. > >> @@ -905,8 +901,6 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr) > >> goto out_free; > >> } > >> > >> - arch_init_invariance_cppc(); > >> - > >> kfree(output.pointer); > >> return 0; > >> > >> diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c > >> index cb52dd000b958..3b281bc1e73c3 100644 > >> --- a/drivers/acpi/processor_driver.c > >> +++ b/drivers/acpi/processor_driver.c > >> @@ -237,6 +237,9 @@ static struct notifier_block acpi_processor_notifier_block = { > >> .notifier_call = acpi_processor_notifier, > >> }; > >> > >> +void __weak acpi_processor_init_invariance_cppc(void) > >> +{ } > > > > Does this actually work if acpi_processor_init_invariance_cppc is a > > macro? How does the compiler know that it needs to use > > init_freq_invariance_cppc() instead of this? > > > > It would work if a __weak definition of init_freq_invariance_cppc() was present. > > I also wasn't sure, so I explicitly added some tracing in > init_freq_invariance_cppc() to make sure it got called and checked it > (GCC 13.2.0). > > But I'll admit it's a confusing behavior. If you think it's too > confusing I'll swap it around to just axe the macros. Yes, please. I'm kind of worried that different compilers may do different things here (say clang vs gcc) and it really is confusing.
On Mon, Nov 4, 2024 at 10:38 PM Ivan Shapovalov <intelfx@intelfx.name> wrote: > > On 2024-11-04 at 15:14 -0600, Mario Limonciello wrote: > > On 11/4/2024 15:10, Rafael J. Wysocki wrote: > > > On Mon, Nov 4, 2024 at 9:54 PM Mario Limonciello <superm1@kernel.org> wrote: > > > > > > > > From: Mario Limonciello <mario.limonciello@amd.com> > > > > > > > > arch_init_invariance_cppc() is called at the end of > > > > acpi_cppc_processor_probe() in order to configure frequency invariance > > > > based upon the values from _CPC. > > > > > > > > This however doesn't work on AMD CPPC shared memory designs that have > > > > AMD preferred cores enabled because _CPC needs to be analyzed from all > > > > cores to judge if preferred cores are enabled. > > > > > > > > This issue manifests to users as a warning since commit 21fb59ab4b97 > > > > ("ACPI: CPPC: Adjust debug messages in amd_set_max_freq_ratio() to warn"): > > > > ``` > > > > Could not retrieve highest performance (-19) > > > > ``` > > > > > > > > However the warning isn't the cause of this, it was actually > > > > commit 279f838a61f9 ("x86/amd: Detect preferred cores in > > > > amd_get_boost_ratio_numerator()") which exposed the issue. > > > > > > > > To fix this problem, change arch_init_invariance_cppc() into a new weak > > > > symbol that is called at the end of acpi_processor_driver_init(). > > > > Each architecture that supports it can declare the symbol to override > > > > the weak one. > > > > > > > > Fixes: 279f838a61f9 ("x86/amd: Detect preferred cores in amd_get_boost_ratio_numerator()") > > > > Reported-by: Ivan Shapovalov <intelfx@intelfx.name> > > > > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219431 > > > > Tested-by: Oleksandr Natalenko <oleksandr@natalenko.name> > > > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > > > > --- > > > > v3: > > > > * Weak symbol instead of macro to help riscv build failure > > > > * Update commit message > > > > * Add comment > > > > --- > > > > arch/arm64/include/asm/topology.h | 2 +- > > > > arch/x86/include/asm/topology.h | 2 +- > > > > drivers/acpi/cppc_acpi.c | 6 ------ > > > > drivers/acpi/processor_driver.c | 9 +++++++++ > > > > include/acpi/processor.h | 2 ++ > > > > 5 files changed, 13 insertions(+), 8 deletions(-) > > > > > > > > diff --git a/arch/arm64/include/asm/topology.h b/arch/arm64/include/asm/topology.h > > > > index 5fc3af9f8f29b..8a1860877967e 100644 > > > > --- a/arch/arm64/include/asm/topology.h > > > > +++ b/arch/arm64/include/asm/topology.h > > > > @@ -27,7 +27,7 @@ void update_freq_counters_refs(void); > > > > #define arch_scale_freq_ref topology_get_freq_ref > > > > > > > > #ifdef CONFIG_ACPI_CPPC_LIB > > > > -#define arch_init_invariance_cppc topology_init_cpu_capacity_cppc > > > > +#define acpi_processor_init_invariance_cppc topology_init_cpu_capacity_cppc > > > > #endif > > > > > > > > /* Replace task scheduler's default cpu-invariant accounting */ > > > > diff --git a/arch/x86/include/asm/topology.h b/arch/x86/include/asm/topology.h > > > > index aef70336d6247..0fb705524aeaa 100644 > > > > --- a/arch/x86/include/asm/topology.h > > > > +++ b/arch/x86/include/asm/topology.h > > > > @@ -307,7 +307,7 @@ extern void arch_scale_freq_tick(void); > > > > > > > > #ifdef CONFIG_ACPI_CPPC_LIB > > > > void init_freq_invariance_cppc(void); > > > > -#define arch_init_invariance_cppc init_freq_invariance_cppc > > > > +#define acpi_processor_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 1a40f0514eaa3..5c0cc7aae8726 100644 > > > > --- a/drivers/acpi/cppc_acpi.c > > > > +++ b/drivers/acpi/cppc_acpi.c > > > > @@ -671,10 +671,6 @@ static int pcc_data_alloc(int pcc_ss_id) > > > > * ) > > > > */ > > > > > > > > -#ifndef arch_init_invariance_cppc > > > > -static inline void arch_init_invariance_cppc(void) { } > > > > -#endif > > > > - > > > > /** > > > > * acpi_cppc_processor_probe - Search for per CPU _CPC objects. > > > > * @pr: Ptr to acpi_processor containing this CPU's logical ID. > > > > @@ -905,8 +901,6 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr) > > > > goto out_free; > > > > } > > > > > > > > - arch_init_invariance_cppc(); > > > > - > > > > kfree(output.pointer); > > > > return 0; > > > > > > > > diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c > > > > index cb52dd000b958..3b281bc1e73c3 100644 > > > > --- a/drivers/acpi/processor_driver.c > > > > +++ b/drivers/acpi/processor_driver.c > > > > @@ -237,6 +237,9 @@ static struct notifier_block acpi_processor_notifier_block = { > > > > .notifier_call = acpi_processor_notifier, > > > > }; > > > > > > > > +void __weak acpi_processor_init_invariance_cppc(void) > > > > +{ } > > > > > > Does this actually work if acpi_processor_init_invariance_cppc is a > > > macro? How does the compiler know that it needs to use > > > init_freq_invariance_cppc() instead of this? > > > > > > It would work if a __weak definition of init_freq_invariance_cppc() was present. > > > > I also wasn't sure, so I explicitly added some tracing in > > init_freq_invariance_cppc() to make sure it got called and checked it > > (GCC 13.2.0). > > Aren't C macros substituted strictly lexically, i.e. if the #define is > present by the time the function definition is parsed, it's just > > void __weak acpi_processor_init_invariance_cppc(void) {} > -> void __weak init_freq_invariance_cppc(void) {} > > ? So it _is_ a weak definition of init_freq_invariance_cppc(). Yes, you're right. > > > > But I'll admit it's a confusing behavior. If you think it's too > > confusing I'll swap it around to just axe the macros. > > ...That said, it does look kinda confusing. Seems to be norm for that > arch/ header file though. So let's not make it even more confusing than it already is.
diff --git a/arch/arm64/include/asm/topology.h b/arch/arm64/include/asm/topology.h index 5fc3af9f8f29b..8a1860877967e 100644 --- a/arch/arm64/include/asm/topology.h +++ b/arch/arm64/include/asm/topology.h @@ -27,7 +27,7 @@ void update_freq_counters_refs(void); #define arch_scale_freq_ref topology_get_freq_ref #ifdef CONFIG_ACPI_CPPC_LIB -#define arch_init_invariance_cppc topology_init_cpu_capacity_cppc +#define acpi_processor_init_invariance_cppc topology_init_cpu_capacity_cppc #endif /* Replace task scheduler's default cpu-invariant accounting */ diff --git a/arch/x86/include/asm/topology.h b/arch/x86/include/asm/topology.h index aef70336d6247..0fb705524aeaa 100644 --- a/arch/x86/include/asm/topology.h +++ b/arch/x86/include/asm/topology.h @@ -307,7 +307,7 @@ extern void arch_scale_freq_tick(void); #ifdef CONFIG_ACPI_CPPC_LIB void init_freq_invariance_cppc(void); -#define arch_init_invariance_cppc init_freq_invariance_cppc +#define acpi_processor_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 1a40f0514eaa3..5c0cc7aae8726 100644 --- a/drivers/acpi/cppc_acpi.c +++ b/drivers/acpi/cppc_acpi.c @@ -671,10 +671,6 @@ static int pcc_data_alloc(int pcc_ss_id) * ) */ -#ifndef arch_init_invariance_cppc -static inline void arch_init_invariance_cppc(void) { } -#endif - /** * acpi_cppc_processor_probe - Search for per CPU _CPC objects. * @pr: Ptr to acpi_processor containing this CPU's logical ID. @@ -905,8 +901,6 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr) goto out_free; } - arch_init_invariance_cppc(); - kfree(output.pointer); return 0; diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c index cb52dd000b958..3b281bc1e73c3 100644 --- a/drivers/acpi/processor_driver.c +++ b/drivers/acpi/processor_driver.c @@ -237,6 +237,9 @@ static struct notifier_block acpi_processor_notifier_block = { .notifier_call = acpi_processor_notifier, }; +void __weak acpi_processor_init_invariance_cppc(void) +{ } + /* * We keep the driver loaded even when ACPI is not running. * This is needed for the powernow-k8 driver, that works even without @@ -270,6 +273,12 @@ static int __init acpi_processor_driver_init(void) NULL, acpi_soft_cpu_dead); acpi_processor_throttling_init(); + + /* + * Frequency invariance calculations on AMD platforms can't be run until + * after acpi_cppc_processor_probe() has been called for all online CPUs. + */ + acpi_processor_init_invariance_cppc(); return 0; err: driver_unregister(&acpi_processor_driver); diff --git a/include/acpi/processor.h b/include/acpi/processor.h index e6f6074eadbf3..a17e97e634a68 100644 --- a/include/acpi/processor.h +++ b/include/acpi/processor.h @@ -465,4 +465,6 @@ extern int acpi_processor_ffh_lpi_probe(unsigned int cpu); extern int acpi_processor_ffh_lpi_enter(struct acpi_lpi_state *lpi); #endif +void acpi_processor_init_invariance_cppc(void); + #endif