Re: [Patch 2/2] ACPI: processor: Add support to configure CPUFREQ reduction pctg

[Sumit Gupta]

On 19/08/23 00:10, Rafael J. Wysocki wrote:
> External email: Use caution opening links or attachments
>
>
> On Thu, Aug 17, 2023 at 11:31 AM Sumit Gupta <sumitg@xxxxxxxxxx> wrote:
> > From: Srikar Srimath Tirumala <srikars@xxxxxxxxxx>
> >
> > Add support to configure the CPUFREQ reduction percentage and set the
> > maximum number of throttling steps accordingly. Current implementation
> > of processor_thermal performs software throttling in fixed steps of
> > "20%" which can be too coarse for some platforms. Change that by adding
> > new config to provide the reduction percentage.
> >
> > Signed-off-by: Srikar Srimath Tirumala <srikars@xxxxxxxxxx>
> > Signed-off-by: Sumit Gupta <sumitg@xxxxxxxxxx>
> > ---
> >   drivers/acpi/Kconfig             | 15 +++++++++++++++
> >   drivers/acpi/processor_thermal.c | 19 ++++++++++++++++---
> >   2 files changed, 31 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> > index 00dd309b6682..287cf58defbf 100644
> > --- a/drivers/acpi/Kconfig
> > +++ b/drivers/acpi/Kconfig
> > @@ -254,6 +254,21 @@ config ACPI_DOCK
> >   config ACPI_CPU_FREQ_PSS
> >          bool
> >
> > +config ACPI_CPU_FREQ_THERM_HAS_PARAMS
> > +       bool "CPU frequency throttling control"
> > +       depends on ACPI_PROCESSOR
> > +
> > +config ACPI_CPU_FREQ_THERM_MIN_THROT_PCTG
> > +       int "Minimum throttle percentage for processor_thermal cooling device"
> > +       depends on ACPI_CPU_FREQ_THERM_HAS_PARAMS
> > +       default 20
> > +       help
> > +         The processor_thermal driver uses this config to calculate the
> > +         percentage amount by which cpu frequency must be reduced for each
> > +         cooling state. The config is also used to calculate the maximum number
> > +         of throttling steps or cooling states supported by the driver. Value
> > +         must be an unsigned integer in the range [1, 50].
> > +
>
> I don't think that the new Kconfig symbols are particularly useful.
> At least they don't help the distro vendors that each would need to
> pick up a specific value for their kernel anyway.
>
> I also wonder how the users building their own kernels are supposed to
> determine the values suitable for the target systems.

We observed some perf gain after reducing the throttle percentage.
Currently, kept the default to '20%' as before.

Based on need, a vendor can overwrite the default value with macro
'CONFIG_ACPI_CPU_FREQ_THERM_MIN_THROT_PCTG'. Otherwise, the behavior
will remain same.

Thank you,
Sumit Gupta

> >   config ACPI_PROCESSOR_CSTATE
> >          def_bool y
> >          depends on ACPI_PROCESSOR
> > diff --git a/drivers/acpi/processor_thermal.c b/drivers/acpi/processor_thermal.c
> > index b7c6287eccca..ee443cc69b73 100644
> > --- a/drivers/acpi/processor_thermal.c
> > +++ b/drivers/acpi/processor_thermal.c
> > @@ -25,8 +25,20 @@
> >    * _any_ cpufreq driver and not only the acpi-cpufreq driver.
> >    */
> >
> > -#define CPUFREQ_THERMAL_MIN_STEP 0
> > -#define CPUFREQ_THERMAL_MAX_STEP 3
> > +#define CPUFREQ_THERMAL_MIN_STEP       0
> > +#ifdef CONFIG_ACPI_CPU_FREQ_THERM_HAS_PARAMS
> > +#define CPUFREQ_THERMAL_PCTG           CONFIG_ACPI_CPU_FREQ_THERM_MIN_THROT_PCTG
> > +
> > +/* Derive the MAX_STEP from minimum throttle percentage so that the reduction
> > + * percentage does end up becoming negative. Also cap the MAX_STEP so that
> > + * the CPU performance doesn't become 0.
> > + */
> > +#define CPUFREQ_THERMAL_MAX_STEP       ((100 / CPUFREQ_THERMAL_PCTG) - 1)
> > +
> > +#else
> > +#define CPUFREQ_THERMAL_MAX_STEP       3
> > +#define CPUFREQ_THERMAL_PCTG           20
> > +#endif
> >
> >   static DEFINE_PER_CPU(unsigned int, cpufreq_thermal_reduction_pctg);
> >
> > @@ -113,7 +125,8 @@ static int cpufreq_set_cur_state(unsigned int cpu, int state)
> >                  if (!policy)
> >                          return -EINVAL;
> >
> > -               max_freq = (policy->cpuinfo.max_freq * (100 - reduction_pctg(i) * 20)) / 100;
> > +               max_freq = (policy->cpuinfo.max_freq *
> > +                           (100 - reduction_pctg(i) * CPUFREQ_THERMAL_PCTG)) / 100;
> >
> >                  cpufreq_cpu_put(policy);
> >
> > --