RE: [PATCH 00/14] cpufreq: intel_pstate: Fixes, cleanups and optimizations

RE: [PATCH 00/14] cpufreq: intel_pstate: Fixes, cleanups and optimizations

[Doug Smythies]

On 2017.03.12 10:12 Rafael J. Wysocki wrote:

> This patch series fixes a couple of bugs in intel_pstate, cleans up the code in
> it somewhat and makes some changes targeted at overhead reductions.
>

If clean up and overhead reductions are being considered, is there any interest
in changing the PID controller to a P controller and eliminating the integral
and derivative code entirely?

Why? The application is not really best suited to a PID
(Proportional Integral Derivative) controller. 

Integral terms are normally used to null out accumulated errors. For example
position errors as a function of integrated velocity, where the overall
position is supposed to be time * nominal velocity, but the actual velocity
at any sample point is not perfect.

In signal processing, derivatives are difficult at the best of times, let alone
with the drastic sample time variations (anywhere from 10 milliseconds to 5 seconds)
experienced here. Myself, I can not think of a need for a derivative term anyhow.

Readers might note the old non-zero integral gain for the old methods used
with Atom processors (being eliminated in this patch set, see patch 2 of 14).
However that was due to the low proportional gain used and was needed to get
target pstates to tick up or down as it settled to some steady state value,
as otherwise and with a setpoint of 97 (which is what was being used at the
time), it would not. I'm saying the integral term was being used in way that
was not intended to overcome another issue. In that scenario, and at the very
least, the error term should have been cleared upon integration to the point
where the pstate ticked up or down as a result.

To be clear, I'm not talking about changing the proportional code at all,
but only about eliminating the integral and derivative code that has never
been used.

If there is interest, I'll prepare and submit a patch.

> Patch [1/14] is a regression fix, patch [2/14] can be regarded as a fix too,
>
> Patches [3-9/14] are cleanups mostly getting rid of unnecessary stuff.
>
> Patches [10-11/14] make changes to reduce the overhead of utilization update
> callbacks used in the active mode.
>
> Patches [12-14/14] make more cleanups on top of that.
>
> Refer to the changelogs for details.

Re: [PATCH 00/14] cpufreq: intel_pstate: Fixes, cleanups and optimizations

[Rafael J. Wysocki]

On Mon, Mar 13, 2017 at 7:29 AM, Doug Smythies <dsmythies@telus.net> wrote:
> On 2017.03.12 10:12 Rafael J. Wysocki wrote:
>
>> This patch series fixes a couple of bugs in intel_pstate, cleans up the code in
>> it somewhat and makes some changes targeted at overhead reductions.
>>
>
> If clean up and overhead reductions are being considered, is there any interest
> in changing the PID controller to a P controller and eliminating the integral
> and derivative code entirely?
>
> Why? The application is not really best suited to a PID
> (Proportional Integral Derivative) controller.

We already have the get_target_pstate_use_cpu_load() P-state selection
routine which is not based on the PID controller and is used for
multiple CPU models already (and for systems with ACPI system profile
set to "mobile", which covers a lot of laptops AFAICS).  Its coverage
may be extended in the future.

> Integral terms are normally used to null out accumulated errors. For example
> position errors as a function of integrated velocity, where the overall
> position is supposed to be time * nominal velocity, but the actual velocity
> at any sample point is not perfect.
>
> In signal processing, derivatives are difficult at the best of times, let alone
> with the drastic sample time variations (anywhere from 10 milliseconds to 5 seconds)
> experienced here. Myself, I can not think of a need for a derivative term anyhow.
>
> Readers might note the old non-zero integral gain for the old methods used
> with Atom processors (being eliminated in this patch set, see patch 2 of 14).
> However that was due to the low proportional gain used and was needed to get
> target pstates to tick up or down as it settled to some steady state value,
> as otherwise and with a setpoint of 97 (which is what was being used at the
> time), it would not. I'm saying the integral term was being used in way that
> was not intended to overcome another issue. In that scenario, and at the very
> least, the error term should have been cleared upon integration to the point
> where the pstate ticked up or down as a result.
>
> To be clear, I'm not talking about changing the proportional code at all,
> but only about eliminating the integral and derivative code that has never
> been used.
>
> If there is interest, I'll prepare and submit a patch.

While it has not been used by default, there is the debugfs interface
for tuning the PID that allows this code to be put into use, in
theory.  It is documented even. :-)

If anyone actively uses it, they won't be happy when it's gone.

Please note that the patches in this series specifically don't change
any user-observable behavior, or at least not intentionally ...

Thanks,
Rafael

RE: [PATCH 00/14] cpufreq: intel_pstate: Fixes, cleanups and optimizations

[Doug Smythies]

Hi Rafael,

Thanks for your quick reply.

On 2017.03.13 04:16 Rafael J. Wysocki wrote:
> On Mon, Mar 13, 2017 at 7:29 AM, Doug Smythies <dsmythies@telus.net> wrote:
>> On 2017.03.12 10:12 Rafael J. Wysocki wrote:
>>
>>> This patch series fixes a couple of bugs in intel_pstate, cleans up the code in
>>> it somewhat and makes some changes targeted at overhead reductions.
>>>
>>
>> If clean up and overhead reductions are being considered, is there any interest
>> in changing the PID controller to a P controller and eliminating the integral
>> and derivative code entirely?
>>
>> Why? The application is not really best suited to a PID
>> (Proportional Integral Derivative) controller.
>
> We already have the get_target_pstate_use_cpu_load() P-state selection
> routine which is not based on the PID controller and is used for
> multiple CPU models already (and for systems with ACPI system profile
> set to "mobile", which covers a lot of laptops AFAICS).

While that is a proportional control type algorithm, I am not suggesting
(at least not this time) changing to it. I am only suggesting to eliminate
the integral and derivative terms for the existing PID controller, but
keeping the existing proportional controller untouched for the
get_target_pstate_use_performance() code path.

>  Its coverage may be extended in the future.

And I will be totally onboard with that, and will help test and such,
when the time comes.

>> Integral terms are normally used to null out accumulated errors. For example
>> position errors as a function of integrated velocity, where the overall
>> position is supposed to be time * nominal velocity, but the actual velocity
>> at any sample point is not perfect.
>>
>> In signal processing, derivatives are difficult at the best of times, let alone
>> with the drastic sample time variations (anywhere from 10 milliseconds to 5 seconds)
>> experienced here. Myself, I can not think of a need for a derivative term anyhow.
>>
>> Readers might note the old non-zero integral gain for the old methods used
>> with Atom processors (being eliminated in this patch set, see patch 2 of 14).
>> However that was due to the low proportional gain used and was needed to get
>> target pstates to tick up or down as it settled to some steady state value,
>> as otherwise and with a setpoint of 97 (which is what was being used at the
>> time), it would not. I'm saying the integral term was being used in way that
>> was not intended to overcome another issue. In that scenario, and at the very
>> least, the error term should have been cleared upon integration to the point
>> where the pstate ticked up or down as a result.
>>
>> To be clear, I'm not talking about changing the proportional code at all,
>> but only about eliminating the integral and derivative code that has never
>> been used.
>>
>> If there is interest, I'll prepare and submit a patch.
>
> While it has not been used by default, there is the debugfs interface
> for tuning the PID that allows this code to be put into use, in
> theory.  It is documented even. :-)

Yes, I understand that.
>
> If anyone actively uses it, they won't be happy when it's gone.
>

But is that a reason not to make a change that makes sense? (Well
it makes sense at least to me.)

I suppose it is possible that someone might be using less p_gain_pct
and compensating with i_gain_pct instead of adjusting setpoint, just
like Atom used to do. I'm saying it is not correct to do it that way,
using an integral term.

> Please note that the patches in this series specifically don't change
> any user-observable behavior, or at least not intentionally ...

I'm not proposing anything that would result in any user-observable
behaviour change either, at least not with the default settings.
However, it is true that i_gain_pct and d_gain_pct would be gone, because
that is the whole point of it.

... Doug

[PATCH 00/14] cpufreq: intel_pstate: Fixes, cleanups and optimizations

[Rafael J. Wysocki]

Hi,

This patch series fixes a couple of bugs in intel_pstate, cleans up the code in
it somewhat and makes some changes targeted at overhead reductions.

Patch [1/14] is a regression fix, patch [2/14] can be regarded as a fix too,

Patches [3-9/14] are cleanups mostly getting rid of unnecessary stuff.

Patches [10-11/14] make changes to reduce the overhead of utilization update
callbacks used in the active mode.

Patches [12-14/14] make more cleanups on top of that.

Refer to the changelogs for details.

Thanks,
Rafael