* [PATCH v3] ACPI: processor: Move arch_init_invariance_cppc() call later
@ 2024-11-04 20:54 Mario Limonciello
2024-11-04 21:10 ` Rafael J. Wysocki
0 siblings, 1 reply; 6+ messages in thread
From: Mario Limonciello @ 2024-11-04 20:54 UTC (permalink / raw)
To: Borislav Petkov, Rafael J . Wysocki
Cc: Thomas Gleixner, Ingo Molnar, Dave Hansen,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),
H . Peter Anvin, Len Brown, Peter Zijlstra, Mario Limonciello,
Gautham R . Shenoy,
open list:X86 ARCHITECTURE (32-BIT AND 64-BIT),
open list:ACPI, Ivan Shapovalov, Oleksandr Natalenko
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)
+{ }
+
/*
* 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] ACPI: processor: Move arch_init_invariance_cppc() call later
2024-11-04 20:54 [PATCH v3] ACPI: processor: Move arch_init_invariance_cppc() call later Mario Limonciello
@ 2024-11-04 21:10 ` Rafael J. Wysocki
2024-11-04 21:14 ` Mario Limonciello
0 siblings, 1 reply; 6+ messages in thread
From: Rafael J. Wysocki @ 2024-11-04 21:10 UTC (permalink / raw)
To: Mario Limonciello
Cc: Borislav Petkov, Rafael J . Wysocki, Thomas Gleixner,
Ingo Molnar, Dave Hansen,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),
H . Peter Anvin, Len Brown, Peter Zijlstra, Mario Limonciello,
Gautham R . Shenoy,
open list:X86 ARCHITECTURE (32-BIT AND 64-BIT),
open list:ACPI, Ivan Shapovalov, Oleksandr Natalenko
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] ACPI: processor: Move arch_init_invariance_cppc() call later
2024-11-04 21:10 ` Rafael J. Wysocki
@ 2024-11-04 21:14 ` Mario Limonciello
2024-11-04 21:37 ` Rafael J. Wysocki
2024-11-04 21:38 ` Ivan Shapovalov
0 siblings, 2 replies; 6+ messages in thread
From: Mario Limonciello @ 2024-11-04 21:14 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Borislav Petkov, Thomas Gleixner, Ingo Molnar, Dave Hansen,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),
H . Peter Anvin, Len Brown, Peter Zijlstra, Mario Limonciello,
Gautham R . Shenoy,
open list:X86 ARCHITECTURE (32-BIT AND 64-BIT),
open list:ACPI, Ivan Shapovalov, Oleksandr Natalenko
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.
>
>> +
>> /*
>> * 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
>>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] ACPI: processor: Move arch_init_invariance_cppc() call later
2024-11-04 21:14 ` Mario Limonciello
@ 2024-11-04 21:37 ` Rafael J. Wysocki
2024-11-04 21:38 ` Ivan Shapovalov
1 sibling, 0 replies; 6+ messages in thread
From: Rafael J. Wysocki @ 2024-11-04 21:37 UTC (permalink / raw)
To: Mario Limonciello
Cc: Rafael J. Wysocki, Borislav Petkov, Thomas Gleixner, Ingo Molnar,
Dave Hansen, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),
H . Peter Anvin, Len Brown, Peter Zijlstra, Mario Limonciello,
Gautham R . Shenoy,
open list:X86 ARCHITECTURE (32-BIT AND 64-BIT),
open list:ACPI, Ivan Shapovalov, Oleksandr Natalenko
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] ACPI: processor: Move arch_init_invariance_cppc() call later
2024-11-04 21:14 ` Mario Limonciello
2024-11-04 21:37 ` Rafael J. Wysocki
@ 2024-11-04 21:38 ` Ivan Shapovalov
2024-11-04 21:42 ` Rafael J. Wysocki
1 sibling, 1 reply; 6+ messages in thread
From: Ivan Shapovalov @ 2024-11-04 21:38 UTC (permalink / raw)
To: Mario Limonciello, Rafael J. Wysocki
Cc: Borislav Petkov, Thomas Gleixner, Ingo Molnar, Dave Hansen,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),
H . Peter Anvin, Len Brown, Peter Zijlstra, Mario Limonciello,
Gautham R . Shenoy,
open list:X86 ARCHITECTURE (32-BIT AND 64-BIT),
open list:ACPI, Oleksandr Natalenko
[-- Attachment #1: Type: text/plain, Size: 7183 bytes --]
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().
>
> 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.
Anyway,
Tested-by: Ivan Shapovalov <intelfx@intelfx.name>
>
> >
> > > +
> > > /*
> > > * 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
> > >
>
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 862 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] ACPI: processor: Move arch_init_invariance_cppc() call later
2024-11-04 21:38 ` Ivan Shapovalov
@ 2024-11-04 21:42 ` Rafael J. Wysocki
0 siblings, 0 replies; 6+ messages in thread
From: Rafael J. Wysocki @ 2024-11-04 21:42 UTC (permalink / raw)
To: Ivan Shapovalov
Cc: Mario Limonciello, Rafael J. Wysocki, Borislav Petkov,
Thomas Gleixner, Ingo Molnar, Dave Hansen,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),
H . Peter Anvin, Len Brown, Peter Zijlstra, Mario Limonciello,
Gautham R . Shenoy,
open list:X86 ARCHITECTURE (32-BIT AND 64-BIT),
open list:ACPI, Oleksandr Natalenko
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-11-04 21:43 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-11-04 20:54 [PATCH v3] ACPI: processor: Move arch_init_invariance_cppc() call later Mario Limonciello
2024-11-04 21:10 ` Rafael J. Wysocki
2024-11-04 21:14 ` Mario Limonciello
2024-11-04 21:37 ` Rafael J. Wysocki
2024-11-04 21:38 ` Ivan Shapovalov
2024-11-04 21:42 ` Rafael J. Wysocki
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®