Re: [PATCH v2 1/1] ACPI: CPPC: Disable FIE if registers in PCC regions

Jeremy Linton
2022-08-10T14:09:03Z

Hi,

On 8/10/22 07:29, Lukasz Luba wrote:
> Hi Jeremy,
>
> +CC Valentin since he might be interested in this finding
> +CC Ionela, Dietmar
>
> I have a few comments for this patch.
>
>
> On 7/28/22 23:10, Jeremy Linton wrote:
>> PCC regions utilize a mailbox to set/retrieve register values used by
>> the CPPC code. This is fine as long as the operations are
>> infrequent. With the FIE code enabled though the overhead can range
>> from 2-11% of system CPU overhead (ex: as measured by top) on Arm
>> based machines.
>>
>> So, before enabling FIE assure none of the registers used by
>> cppc_get_perf_ctrs() are in the PCC region. Furthermore lets also
>> enable a module parameter which can also disable it at boot or module
>> reload.
>>
>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
>> ---
>>   drivers/acpi/cppc_acpi.c       | 41 ++++++++++++++++++++++++++++++++++
>>   drivers/cpufreq/cppc_cpufreq.c | 19 ++++++++++++----
>>   include/acpi/cppc_acpi.h       |  5 +++++
>>   3 files changed, 61 insertions(+), 4 deletions(-)
>
>
> 1. You assume that all platforms would have this big overhead when
>    they have the PCC regions for this purpose.
>    Do we know which version of HW mailbox have been implemented
>    and used that have this 2-11% overhead in a platform?
>    Do also more recent MHU have such issues, so we could block
>    them by default (like in your code)?

Well, the mailbox nature of PCC pretty much assures its "slow", relative
the alternative of providing an actual register. If a platform provides
direct access to say MHU registers, then of course they won't actually
be in a PCC region and the FIE will remain on.

>
> 2. I would prefer to simply change the default Kconfig value to 'n' for
>    the ACPI_CPPC_CPUFREQ_FIE, instead of creating a runtime
>    check code which disables it.
>    We have probably introduce this overhead for older platforms with
>    this commit:

The problem here is that these ACPI kernels are being shipped as single
images in distro's which expect them to run on a wide range of platforms
(including x86/amd in this case), and preform optimally on all of them.

So the 'n' option basically is saying that the latest FIE code doesn't
provide a befit anywhere?

>
> commit 4c38f2df71c8e33c0b64865992d693f5022eeaad
> Author: Viresh Kumar <viresh.kumar@linaro.org>
> Date:   Tue Jun 23 15:49:40 2020 +0530
>
>     cpufreq: CPPC: Add support for frequency invariance
>
>
>
> If the test server with this config enabled performs well
> in the stress-tests, then on production server the config may be
> set to 'y' (or 'm' and loaded).
>
> I would vote to not add extra code, which then after a while might be
> decided to bw extended because actually some HW is actually capable (so
> we could check in runtime and enable it). IMO this create an additional
> complexity in our diverse configuration/tunnable space in our code.
>
> When we don't compile-in this, we should fallback to old-style
> FIE, which has been used on these old platforms.
>
> BTW (I have to leave it here) the first-class solution for those servers
> is to implement AMU counters, so the overhead to retrieve this info is
> really low.
>
> Regards,
> Lukasz