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

[Jeremy Linton]

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@xxxxxxx>
> > ---
> >   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@xxxxxxxxxx>
> 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