Questions on the PID controller in the intel_pstate driver

Questions on the PID controller in the intel_pstate driver

[Alexander E. Patrakov]

Hello.

I have noticed that there is a PID controller in the intel_pstate
driver, and thus have questions related to it. Yes, I understand that,
by default, there is only a "proportional" term. I have only read the
code, but have not tried to modify it to see how it breaks.

The goal of this e-mail is to determine whether we can explain
https://bugzilla.kernel.org/show_bug.cgi?id=60727 and to audit the
driver for other bugs. If I worked at Intel, I would have asked the
same questions as a part of normal code review.

1. Usually, the output of the PID controller is directly used to
control the process (in our case, this, naively, would be the
performance MSR). However, in the current driver, the output of
pid_calc is first quantized (by fp_toint(result) at the very end of
the function) and then used as a number of steps to increase or
decrease the MSR value - i.e. essentially integraded. This integration
essentially makes a non-standard integral-doubleintegral-proportional
type of controller instead of PID (and, due to only the P term being
present by default, the result is ian integral-only controller with
the quantization step before the integral).

Why was the decision to quantize and then integrate the PID controller
output made, and not the other way round? Why is the explicit
integration step outside of the controller necessary at all, instead
of letting a normal PI controller output (with different coefficients
- i.e. leaving only the I term would get what we have now, minus
quantization) directly control the MSR value?

2. In intel_pstate_get_scaled_busy(), there is this line:

        busy_scaled = mul_fp(core_busy, div_fp(max_pstate, current_pstate));

What is the physical meaning of the result - i.e. why does core_busy
need to be upscaled by (max_pstate / current_pstate)? Why is this
result chosen as something to stabilize (by comparing to the setpoint
in the PID controller)?

I am asking because I don't quite understand the logic. Is the goal to
ensure (by the PID regulator choosing the P-state) that the CPU is as
close to 97% busy as possible, under the default policy?

3. Why is 97 chosen as a setpoint? What was the reasoning behind
setting the setpoint to 109 before the change 2134ed4d6 (asking
because 109 does not make sense as the "CPU busy percentage" setpoint
in an integral-only controller, see question (1) why I am calling your
controller integral-only)?

4. Was any "well-established" procedure like Ziegler-Nichols tuning
method tried to tune the coefficients of the PID controller? Were
there any failed attempts?

5. Why is a PID controller (a thing that needs tuning, testing for
stability, requires essentially-floating-point calculations and can
provoke "I don't understand" e-mails such as this one) used at all?
What were the arguments for choosing it over, e.g., the simple
heuristic similar to what is in cpufreq_conservative.c?

Note: I am not subscribed to the cpufreq list. Please CC: me on replies.

-- 
Alexander E. Patrakov

Re: Questions on the PID controller in the intel_pstate driver

[Dirk Brandewie]

On 08/11/2013 06:04 AM, Alexander E. Patrakov wrote:
> Hello.
>
> I have noticed that there is a PID controller in the intel_pstate
> driver, and thus have questions related to it. Yes, I understand that,
> by default, there is only a "proportional" term. I have only read the
> code, but have not tried to modify it to see how it breaks.
>
> The goal of this e-mail is to determine whether we can explain
> https://bugzilla.kernel.org/show_bug.cgi?id=60727 and to audit the
> driver for other bugs. If I worked at Intel, I would have asked the
> same questions as a part of normal code review.
>
> 1. Usually, the output of the PID controller is directly used to
> control the process (in our case, this, naively, would be the
> performance MSR). However, in the current driver, the output of
> pid_calc is first quantized (by fp_toint(result) at the very end of
> the function) and then used as a number of steps to increase or
> decrease the MSR value - i.e. essentially integraded. This integration
> essentially makes a non-standard integral-doubleintegral-proportional
> type of controller instead of PID (and, due to only the P term being
> present by default, the result is ian integral-only controller with
> the quantization step before the integral).
>

The fp_toint() call is to return an integer instead of the fixed point
floating point number used in pid_calc().  I am unclear how this is an
integration.

pid_calc returns an error value that the MRS needs to be adjusted by and
not an MSR value since there are MANY SKU's with different pstate ranges.

> Why was the decision to quantize and then integrate the PID controller
> output made, and not the other way round? Why is the explicit
> integration step outside of the controller necessary at all, instead
> of letting a normal PI controller output (with different coefficients
> - i.e. leaving only the I term would get what we have now, minus
> quantization) directly control the MSR value?
>
> 2. In intel_pstate_get_scaled_busy(), there is this line:
>
>          busy_scaled = mul_fp(core_busy, div_fp(max_pstate, current_pstate));
>
> What is the physical meaning of the result - i.e. why does core_busy
> need to be upscaled by (max_pstate / current_pstate)? Why is this
> result chosen as something to stabilize (by comparing to the setpoint
> in the PID controller)?
>

The scaling is done to find out how busy the CPU is at the current P state.
i.e If the core is 60% busy and the P state is 60% of max then it is 100%
     busy in the P state and the P state should be increased

> I am asking because I don't quite understand the logic. Is the goal to
> ensure (by the PID regulator choosing the P-state) that the CPU is as
> close to 97% busy as possible, under the default policy?
>
> 3. Why is 97 chosen as a setpoint? What was the reasoning behind
> setting the setpoint to 109 before the change 2134ed4d6 (asking
> because 109 does not make sense as the "CPU busy percentage" setpoint
> in an integral-only controller, see question (1) why I am calling your
> controller integral-only)?
>

The 97% value goes along with the change to scaling off of max_pstate
instead of turbo_pstate.  The 97 works for the narrow P state bin size approx
2.5% of max_pstate/frequency.

> 4. Was any "well-established" procedure like Ziegler-Nichols tuning
> method tried to tune the coefficients of the PID controller? Were
> there any failed attempts?
>

This algorithm is for tuning linear systems and the changes in load are
anything but linear.  Changing the operating point (MSR) is instantaneous
also not linear.

> 5. Why is a PID controller (a thing that needs tuning, testing for
> stability, requires essentially-floating-point calculations and can
> provoke "I don't understand" e-mails such as this one) used at all?
> What were the arguments for choosing it over, e.g., the simple
> heuristic similar to what is in cpufreq_conservative.c?
>

The PID was chosen after implementation and evaluation of a number of algorithms
the PID had best the best power efficiency.  Is the PID the end all probably
not but in my testing it was the best I could come up with that fit the range
of workloads mobile/desktop/datacenter.

Are the default tuning parameters optimal for all workloads?  Clearly not but
neither are the the tuning parameters for any other governor (that I am aware
of) they were chosen to give good power efficiency for a range of workloads
and processor SKUs

> Note: I am not subscribed to the cpufreq list. Please CC: me on replies.
>

Re: Questions on the PID controller in the intel_pstate driver

[Alexander E. Patrakov]

2013/8/20 Dirk Brandewie <dirk.brandewie@gmail.com>:

(let me reorder the points, so that the answers that I am fully
satisfied with are at the top)
> On 08/11/2013 06:04 AM, Alexander E. Patrakov wrote:

>> 4. Was any "well-established" procedure like Ziegler-Nichols tuning
>> method tried to tune the coefficients of the PID controller? Were
>> there any failed attempts?
>
> This algorithm is for tuning linear systems and the changes in load are
> anything but linear.  Changing the operating point (MSR) is instantaneous
> also not linear.

Thanks for the answer, there is really nothing left to discuss about this point.

About the nonlinearity introduced by the discrete nature of MSR
changes - yes, this is also valid and relevant for point (1), below.

>> 5. Why is a PID controller (a thing that needs tuning, testing for
>> stability, requires essentially-floating-point calculations and can
>> provoke "I don't understand" e-mails such as this one) used at all?
>> What were the arguments for choosing it over, e.g., the simple
>> heuristic similar to what is in cpufreq_conservative.c?
>>
>
> The PID was chosen after implementation and evaluation of a number of
> algorithms
> the PID had best the best power efficiency.  Is the PID the end all probably
> not but in my testing it was the best I could come up with that fit the
> range
> of workloads mobile/desktop/datacenter.

OK.

>> 1. Usually, the output of the PID controller is directly used to
>> control the process (in our case, this, naively, would be the
>> performance MSR). However, in the current driver, the output of
>> pid_calc is first quantized (by fp_toint(result) at the very end of
>> the function) and then used as a number of steps to increase or
>> decrease the MSR value - i.e. essentially integraded. This integration
>> essentially makes a non-standard integral-doubleintegral-proportional
>> type of controller instead of PID (and, due to only the P term being
>> present by default, the result is ian integral-only controller with
>> the quantization step before the integral).
>>
>
> The fp_toint() call is to return an integer instead of the fixed point
> floating point number used in pid_calc().  I am unclear how this is an
> integration.

This is quantization, not integration.

> pid_calc returns an error value that the MRS needs to be adjusted by

Correct, and the "adjust by this value" logic is in
intel_pstate_pstate_increase() and intel_pstate_pstate_decrease().
This "adjust by this value" logic is what I call integration. And BTW,
the current code in intel_pstate_adjust_busy_pstate() after the call
to pid_calc() is just a very elaborate way to write this:

int target;
target = cpu->pstate.current_pstate + ctl;  /* that's what I call
integration of ctl */

intel_pstate_set_pstate(cpu, target);  /* note: it clamps
cpu->pstate.current_pstate, correctly */

so no separate code paths WRT increasing and decreasing the P-state are needed.

> and
> not an MSR value since there are MANY SKU's with different pstate ranges.

and this is what I (so far) disagree with. All your code does
(correctly) to work around many possible SKU's is to interpret the PID
(or, as I and D terms are set to zero by default, P-only) controller
output as a number of steps to increment the P-state by. What I am
saying is that:

1) the PID controller can do it for you, if you replace the
proportional term with the integral one and adjust the
integral-term-clamping logic (replace the hard-coded constant
int_tofp(30) with something related to intel_pstate_get_min_max());

2) doing so would also swap the order of integration and quantization
to the one that represents a simpler type of non-linearity
(quantization at the very end instead of quantization before
integration).

The same ugly stairstep type of non-linearity also applies to the
input of the PID controller, namely, in
intel_pstate_get_scaled_busy(), busy_scaled is a fixed-point number,
that is later converted to integer and then (in pid_calc) again to
fixed point. I think that such roundtrip is not needed, because it
only loses precision unnecessarily.

I think I should provide a series of patches (meant to be rejected,
with an explanation serving as the answer to my question) to better
illustrate my point, will do it later today or tomorrow.

>> 2. In intel_pstate_get_scaled_busy(), there is this line:
>>
>>          busy_scaled = mul_fp(core_busy, div_fp(max_pstate,
>> current_pstate));
>>
>> What is the physical meaning of the result - i.e. why does core_busy
>> need to be upscaled by (max_pstate / current_pstate)? Why is this
>> result chosen as something to stabilize (by comparing to the setpoint
>> in the PID controller)?
>>
>
> The scaling is done to find out how busy the CPU is at the current P state.
> i.e If the core is 60% busy and the P state is 60% of max then it is 100%
>     busy in the P state and the P state should be increased

OK. A patch with comments about the possible ranges of
cpu->samples[cpu->sample_ptr].{aperf, mperf,core_pct_busy} and
busy_scaled in the "60% of the max" P state and in the "110% of the
max" turbo P state would help future readers of this code.

>> I am asking because I don't quite understand the logic. Is the goal to
>> ensure (by the PID regulator choosing the P-state) that the CPU is as
>> close to 97% busy as possible, under the default policy?
>>
>> 3. Why is 97 chosen as a setpoint? What was the reasoning behind
>> setting the setpoint to 109 before the change 2134ed4d6 (asking
>> because 109 does not make sense as the "CPU busy percentage" setpoint
>> in an integral-only controller, see question (1) why I am calling your
>> controller integral-only)?
>>
>
> The 97% value goes along with the change to scaling off of max_pstate
> instead of turbo_pstate.  The 97 works for the narrow P state bin size
> approx
> 2.5% of max_pstate/frequency.

Let me reword to verify my understanding. You are saying that, before
this change, busy_scaled was overestimated by a factor of turbo_pstate
/ max_pstate, and so was the desired setpoint, thus bringing it out of
the 100% range. Is that right?

-- 
Alexander E. Patrakov