Re: [PATCH 1/3] intel_pstate: Clarify average performance computation

[Rafael J. Wysocki]

On Wed, May 11, 2016 at 7:01 AM, Srinivas Pandruvada
<srinivas.pandruvada@xxxxxxxxxxxxxxx> wrote:
> On Tue, 2016-05-10 at 22:57 +0200, Rafael J. Wysocki wrote:
>> > > >
>
> [...]
>
>> ---
>> From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
>> Subject: [PATCH] intel_pstate: Clarify average performance
>> computation
>>
>> The core_pct_busy field of struct sample actually contains the
>> average performace during the last sampling period (in percent)
>> and not the utilization of the core as suggested by its name
>> which is confusing.
>>
>> For this reason, change the name of that field to core_avg_perf
>> and rename the function that computes its value accordingly.
>>
>> Also notice that storing this value as percentage requires a costly
>> integer multiplication to be carried out in a hot path, so instead
>> store it as an "extended fixed point" value with more fraction bits
>> and update the code using it accordingly (it is better to change the
>> name of the field along with its meaning in one go than to make those
>> two changes separately, as that would likely lead to more
>> confusion).
>>
>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
>> ---
>>  drivers/cpufreq/intel_pstate.c |   31 +++++++++++++++---------------
>> -
>>  1 file changed, 15 insertions(+), 16 deletions(-)
>>
>> Index: linux-pm/drivers/cpufreq/intel_pstate.c
>> ===================================================================
>> --- linux-pm.orig/drivers/cpufreq/intel_pstate.c
>> +++ linux-pm/drivers/cpufreq/intel_pstate.c
>> @@ -49,6 +49,9 @@
>>  #define int_tofp(X) ((int64_t)(X) << FRAC_BITS)
>>  #define fp_toint(X) ((X) >> FRAC_BITS)
>>
>> +#define EXT_BITS 6
>> +#define EXT_FRAC_BITS (EXT_BITS + FRAC_BITS)
>> +
>>  static inline int32_t mul_fp(int32_t x, int32_t y)
>>  {
>>       return ((int64_t)x * (int64_t)y) >> FRAC_BITS;
>> @@ -72,10 +75,10 @@ static inline int ceiling_fp(int32_t x)
>>
>>  /**
>>   * struct sample -   Store performance sample
>> - * @core_pct_busy:   Ratio of APERF/MPERF in percent, which is
>> actual
>> + * @core_avg_perf:   Ratio of APERF/MPERF which is the actual
>> average
>>   *                   performance during last sample period
>>   * @busy_scaled:     Scaled busy value which is used to calculate
>> next
>> - *                   P state. This can be different than
>> core_pct_busy
>> + *                   P state. This can be different than
>> core_avg_perf
>>   *                   to account for cpu idle period
>>   * @aperf:           Difference of actual performance frequency
>> clock count
>>   *                   read from APERF MSR between last and
>> current sample
>> @@ -90,8 +93,8 @@ static inline int ceiling_fp(int32_t x)
>>   * data for choosing next P State.
>>   */
>>  struct sample {
>> -     int32_t core_pct_busy;
>>       int32_t busy_scaled;
>> +     u64 core_avg_perf;
>>       u64 aperf;
>>       u64 mperf;
>>       u64 tsc;
>> @@ -1147,15 +1150,12 @@ static void intel_pstate_get_cpu_pstates
>>       intel_pstate_set_min_pstate(cpu);
>>  }
>>
>> -static inline void intel_pstate_calc_busy(struct cpudata *cpu)
>> +static inline void intel_pstate_calc_avg_perf(struct cpudata *cpu)
>>  {
>>       struct sample *sample = &cpu->sample;
>> -     int64_t core_pct;
>> -
>> -     core_pct = sample->aperf * int_tofp(100);
>> -     core_pct = div64_u64(core_pct, sample->mperf);
>>
>> -     sample->core_pct_busy = (int32_t)core_pct;
>> +     sample->core_avg_perf = div64_u64(sample->aperf <<
>> EXT_FRAC_BITS,
>> +                                       sample->mperf);
>>  }
>>
>>  static inline bool intel_pstate_sample(struct cpudata *cpu, u64
>> time)
>> @@ -1198,9 +1198,8 @@ static inline bool intel_pstate_sample(s
>>
>>  static inline int32_t get_avg_frequency(struct cpudata *cpu)
>>  {
>> -     return fp_toint(mul_fp(cpu->sample.core_pct_busy,
>> -                            int_tofp(cpu-
>> >pstate.max_pstate_physical *
>> -                                             cpu->pstate.scaling
>> / 100)));
>> +     return (cpu->sample.core_avg_perf * cpu-
>> >pstate.max_pstate_physical *
>> +                     cpu->pstate.scaling) >> EXT_FRAC_BITS;
>
> This breaks frequency display. Needs cast
> return ((u64)cpu->sample.core_avg_perf * cpu->
>         pstate.max_pstate_physical * cpu->pstate.scaling) >>
> EXT_FRAC_BITS;

Well, that's strange, because sample.core_avg_perf is a u64 after this
patch already.

But if we are to make explicit type conversions, I'd rather store
sample.core_avg_perf in 32 bit.

> Otherwise results are very close with the version without this change.

OK, let me resend the series with this patch reworked once again.