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

[Rafael J. Wysocki]

On Mon, Aug 21, 2023 at 3:24 PM Sumit Gupta <sumitg@xxxxxxxxxx> wrote:
>
>
>
> 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.

So you should add this information to the patch changelog, ideally
along with the description of the hardware configuration in which the
improvement has been observed.

> 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.

Yes, that's how it works.

What I'm saying is that the way it works does not appear to be
particularly useful.

For example, how exactly is a distribution supposed to guess the
"right" value for their general-purpose kernel?