Re: [PATCH v2 1/3] Documentation: admin-guide: pm: Add efficiency vs. latency tradeoff to uncore documentation

[Hans de Goede]

Hi,

On 8/30/24 12:12 PM, Ilpo Järvinen wrote:
> On Fri, 30 Aug 2024, Tero Kristo wrote:
> 
>>    1. On Thu, 2024-08-29 at 14:39 +0300, Tero Kristo wrote:
>>> On Thu, 2024-08-29 at 12:18 +0300, Ilpo Järvinen wrote:
>>>> On Wed, 28 Aug 2024, Tero Kristo wrote:
>>>>
>>>>> Added documentation about the functionality of efficiency vs.
>>>>> latency tradeoff
>>>>> control in intel Xeon processors, and how this is configured via
>>>>> sysfs.
>>>>>
>>>>> Signed-off-by: Tero Kristo <tero.kristo@xxxxxxxxxxxxxxx>
>>>>> ---
>>>>> v2:
>>>>>   * Largely re-wrote the documentation
>>>>>
>>>>>  .../pm/intel_uncore_frequency_scaling.rst     | 59
>>>>> +++++++++++++++++++
>>>>>  1 file changed, 59 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/admin-
>>>>> guide/pm/intel_uncore_frequency_scaling.rst
>>>>> b/Documentation/admin-
>>>>> guide/pm/intel_uncore_frequency_scaling.rst
>>>>> index 5ab3440e6cee..26ded32b06f5 100644
>>>>> --- a/Documentation/admin-
>>>>> guide/pm/intel_uncore_frequency_scaling.rst
>>>>> +++ b/Documentation/admin-
>>>>> guide/pm/intel_uncore_frequency_scaling.rst
>>>>> @@ -113,3 +113,62 @@ to apply at each uncore* level.
>>>>>  
>>>>>  Support for "current_freq_khz" is available only at each fabric
>>>>> cluster
>>>>>  level (i.e., in uncore* directory).
>>>>> +
>>>>> +Efficiency vs. Latency Tradeoff
>>>>> +-------------------------------
>>>>> +
>>>>> +The Efficiency Latency Control (ELC) feature improves
>>>>> performance
>>>>> +per watt. With this feature hardware power management algorithms
>>>>> +optimize trade-off between latency and power consumption. For
>>>>> some
>>>>> +latency sensitive workloads further tuning can be done by SW to
>>>>> +get desired performance.
>>>>> +
>>>>> +The hardware monitors the average CPU utilization across all
>>>>> cores
>>>>> +in a power domain at regular intervals and decides an uncore
>>>>> frequency.
>>>>> +While this may result in the best performance per watt, workload
>>>>> may be
>>>>> +expecting higher performance at the expense of power. Consider
>>>>> an
>>>>> +application that intermittently wakes up to perform memory reads
>>>>> on an
>>>>> +otherwise idle system. In such cases, if hardware lowers uncore
>>>>> +frequency, then there may be delay in ramp up of frequency to
>>>>> meet
>>>>> +target performance.
>>>>> +
>>>>> +The ELC control defines some parameters which can be changed
>>>>> from
>>>>> SW.
>>>>> +If the average CPU utilization is below a user defined threshold
>>>>> +(elc_low_threshold_percent attribute below), the user defined
>>>>> uncore
>>>>> +frequency floor frequency will be used (elc_floor_freq_khz
>>>>> attribute
>>>>
>>>> Consider the following simplification:
>>>>
>>>> "the user defined uncore frequency floor frequency" ->
>>>> "the user-defined uncore floor frequency"
>>>>
>>>> I think it tells the same even without that first "frequency".
>>>>
>>>> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
>>>>
>>>
>>> Yeah, it looks kind of silly. I think that's just a typo from my
>>> side,
>>> thanks for catching.
>>
>> Do you want me to send a new version of this patch or do you fix it
>> locally? Rest of the patches don't seem to need any changes atm.
> 
> That's up to Hans but that looks trivial change so probably he can fix
> that while applying.
> 
> Hans, v2 of this series seems ready to go (with the small change into
> the documentation patch as discussed above).

Ack, I've applied the series to my review-hans branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

with the suggested improvement to intel_uncore_frequency_scaling.rst
sqaushed in.

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans