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

[Srinivas Pandruvada]

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;

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

Thanks,
Srinivas