FIE assumes the delivered/relative perf registers are fast to read so
it goes ahead and hits them quite frequently. On a couple Arm
platforms though they end up in PCC regions which require mailbox
handshaking with other parts of the platform.
This results in a lot of overhead in the cppc_fie task. As such lets
runtime disable FIE if we detect it enabled on one of those platforms.
Also allow the user to manually enable/disable it via a module parameter.
v1->v2:
Apply Rafael's review comments.
Move the MODULE_PARAM into the ifdef
Fix compiler warning when ACPI_CPPC_LIB is disabled.
v2->v3:
Tristate the module param so FIE can be forced on/off
Bump pr_debug to pr_info if FIE is disabled due to PCC regions
Switch ACPI_CPPC_CPUFREQ_FIE off by default
Jeremy Linton (2):
ACPI: CPPC: Disable FIE if registers in PCC regions
cpufreq: CPPC: Change FIE default
drivers/acpi/cppc_acpi.c | 41 ++++++++++++++++++++++++++++++++++
drivers/cpufreq/Kconfig.arm | 2 +-
drivers/cpufreq/cppc_cpufreq.c | 31 +++++++++++++++++++++----
include/acpi/cppc_acpi.h | 5 +++++
4 files changed, 74 insertions(+), 5 deletions(-)
--
2.37.1
FIE is mostly implemented as PCC mailboxes on arm machines. This was
enabled by default without any data suggesting that it does anything
but hurt system performance. Lets change the default to 'n' until
hardware appears which clearly benefits.
Signed-off-by: Jeremy Linton <[email protected]>
---
drivers/cpufreq/Kconfig.arm | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
index 954749afb5fe..ad66d8f15db0 100644
--- a/drivers/cpufreq/Kconfig.arm
+++ b/drivers/cpufreq/Kconfig.arm
@@ -22,7 +22,7 @@ config ACPI_CPPC_CPUFREQ
config ACPI_CPPC_CPUFREQ_FIE
bool "Frequency Invariance support for CPPC cpufreq driver"
depends on ACPI_CPPC_CPUFREQ && GENERIC_ARCH_TOPOLOGY
- default y
+ default n
help
This extends frequency invariance support in the CPPC cpufreq driver,
by using CPPC delivered and reference performance counters.
--
2.37.1
On 18-08-22, 16:16, Jeremy Linton wrote:
> FIE is mostly implemented as PCC mailboxes on arm machines. This was
> enabled by default without any data suggesting that it does anything
> but hurt system performance. Lets change the default to 'n' until
> hardware appears which clearly benefits.
>
> Signed-off-by: Jeremy Linton <[email protected]>
> ---
> drivers/cpufreq/Kconfig.arm | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> index 954749afb5fe..ad66d8f15db0 100644
> --- a/drivers/cpufreq/Kconfig.arm
> +++ b/drivers/cpufreq/Kconfig.arm
> @@ -22,7 +22,7 @@ config ACPI_CPPC_CPUFREQ
> config ACPI_CPPC_CPUFREQ_FIE
> bool "Frequency Invariance support for CPPC cpufreq driver"
> depends on ACPI_CPPC_CPUFREQ && GENERIC_ARCH_TOPOLOGY
> - default y
> + default n
> help
> This extends frequency invariance support in the CPPC cpufreq driver,
> by using CPPC delivered and reference performance counters.
Why is this required after we have the first patch in ?
--
viresh
Hi Viresh,
+CC Dietmar, Morten and Souvik
On 8/24/22 07:14, Viresh Kumar wrote:
> On 18-08-22, 16:16, Jeremy Linton wrote:
>> FIE is mostly implemented as PCC mailboxes on arm machines. This was
>> enabled by default without any data suggesting that it does anything
>> but hurt system performance. Lets change the default to 'n' until
>> hardware appears which clearly benefits.
>>
>> Signed-off-by: Jeremy Linton <[email protected]>
>> ---
>> drivers/cpufreq/Kconfig.arm | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
>> index 954749afb5fe..ad66d8f15db0 100644
>> --- a/drivers/cpufreq/Kconfig.arm
>> +++ b/drivers/cpufreq/Kconfig.arm
>> @@ -22,7 +22,7 @@ config ACPI_CPPC_CPUFREQ
>> config ACPI_CPPC_CPUFREQ_FIE
>> bool "Frequency Invariance support for CPPC cpufreq driver"
>> depends on ACPI_CPPC_CPUFREQ && GENERIC_ARCH_TOPOLOGY
>> - default y
>> + default n
>> help
>> This extends frequency invariance support in the CPPC cpufreq driver,
>> by using CPPC delivered and reference performance counters.
>
> Why is this required after we have the first patch in ?
>
There are a few issues with this ACPI_CPPC_CPUFREQ_FIE solution:
1. The design is very heavy and that kernel thread can be ~512 util
(that's what we have been told by one of our partners from servers'
world)
2. The HW & FW design is not suited for this task. Newer HW will just
have AMU counters (on Arm64) for FIE
3. The patches haven't been tested in terms of performance overhead
AFAIK. Although, it affects existing Arm64 servers with their
workloads.
4. AFAIK non of our server partners wasn't complaining about issues with
old FIE mechanism.
In our team we are not allowed to send code that we cannot prove in many
ways.
I would just not compile this at all (or even revert this feature).
If someone compiled in this by accident - make sure we disable it
after checks like in the patch 1/2. I'll add also some comments
to that patch.
Regards,
Lukasz
On 24-08-22, 15:04, Lukasz Luba wrote:
> On 8/24/22 07:14, Viresh Kumar wrote:
> > On 18-08-22, 16:16, Jeremy Linton wrote:
> > > FIE is mostly implemented as PCC mailboxes on arm machines. This was
> > > enabled by default without any data suggesting that it does anything
> > > but hurt system performance. Lets change the default to 'n' until
> > > hardware appears which clearly benefits.
> > >
> > > Signed-off-by: Jeremy Linton <[email protected]>
> > > ---
> > > drivers/cpufreq/Kconfig.arm | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> > > index 954749afb5fe..ad66d8f15db0 100644
> > > --- a/drivers/cpufreq/Kconfig.arm
> > > +++ b/drivers/cpufreq/Kconfig.arm
> > > @@ -22,7 +22,7 @@ config ACPI_CPPC_CPUFREQ
> > > config ACPI_CPPC_CPUFREQ_FIE
> > > bool "Frequency Invariance support for CPPC cpufreq driver"
> > > depends on ACPI_CPPC_CPUFREQ && GENERIC_ARCH_TOPOLOGY
> > > - default y
> > > + default n
> > > help
> > > This extends frequency invariance support in the CPPC cpufreq driver,
> > > by using CPPC delivered and reference performance counters.
> >
> > Why is this required after we have the first patch in ?
> >
Well, my question was for the way the patches were added. If we are
disabling the feature based on platform, then there is no need to
disable the feature by default.
> There are a few issues with this ACPI_CPPC_CPUFREQ_FIE solution:
> 1. The design is very heavy and that kernel thread can be ~512 util
> (that's what we have been told by one of our partners from servers'
> world)
> 2. The HW & FW design is not suited for this task. Newer HW will just
> have AMU counters (on Arm64) for FIE
> 3. The patches haven't been tested in terms of performance overhead
> AFAIK. Although, it affects existing Arm64 servers with their
> workloads.
> 4. AFAIK non of our server partners wasn't complaining about issues with
> old FIE mechanism.
>
> In our team we are not allowed to send code that we cannot prove in many
> ways.
>
> I would just not compile this at all (or even revert this feature).
> If someone compiled in this by accident - make sure we disable it
> after checks like in the patch 1/2. I'll add also some comments
> to that patch.
If we don't really want the feature, which is open for discussion
(added Vincent to have a look as well), then we better mark it BROKEN
in Kconfig.
--
viresh