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@xxxxxxxxx> 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