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

[Srinivas Pandruvada]

On Sat, 2016-05-07 at 01:44 +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> 
> 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.
> 
Makes perfect sense.

> Also notice that it would be more useful if it was a "raw" fraction
> rather than percentage, so change its meaning too 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).
Due to the calculation the results from old and new method will be
similar but not same. For example in one scenario the
get_avg_frequencyÂdifference is 4.3KHz (printed side by side using both
old style using pct and new using fraction)
Frequency with old calc: 2996093 Hz
Frequency with old calc: 3000460 Hz

How much do you think the performance gain changing fraction vs pct?

> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> ---
> Âdrivers/cpufreq/intel_pstate.c |ÂÂÂ24 ++++++++++--------------
> Â1 file changed, 10 insertions(+), 14 deletions(-)
> 
> Index: linux-pm/drivers/cpufreq/intel_pstate.c
> ===================================================================
> --- linux-pm.orig/drivers/cpufreq/intel_pstate.c
> +++ linux-pm/drivers/cpufreq/intel_pstate.c
> @@ -72,10 +72,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,7 +90,7 @@ static inline int ceiling_fp(int32_t x)
> Â * data for choosing next P State.
> Â */
> Âstruct sample {
> -	int32_t core_pct_busy;
> +	int32_t core_avg_perf;
> Â	int32_t busy_scaled;
> Â	u64 aperf;
> Â	u64 mperf;
> @@ -1147,15 +1147,11 @@ 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 = div_fp(sample->aperf, sample-
> >mperf);
> Â}
> Â
> Âstatic inline bool intel_pstate_sample(struct cpudata *cpu, u64
> time)
> @@ -1198,9 +1194,9 @@ 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,
> +	return fp_toint(mul_fp(cpu->sample.core_avg_perf,
> Â			ÂÂÂÂÂÂÂint_tofp(cpu-
> >pstate.max_pstate_physical *
> -						cpu->pstate.scaling
> / 100)));
> +						cpu-
> >pstate.scaling)));
> Â}
> Â
> Âstatic inline int32_t get_avg_pstate(struct cpudata *cpu)
> @@ -1260,7 +1256,7 @@ static inline int32_t get_target_pstate_
> Â	Â* period. The result will be a percentage of busy at a
> Â	Â* specified pstate.
> Â	Â*/
> -	core_busy = cpu->sample.core_pct_busy;
> +	core_busy = 100 * cpu->sample.core_avg_perf;
> Â	max_pstate = cpu->pstate.max_pstate_physical;
> Â	current_pstate = cpu->pstate.current_pstate;
> Â	core_busy = mul_fp(core_busy, div_fp(max_pstate,
> current_pstate));
> @@ -1312,7 +1308,7 @@ static inline void intel_pstate_adjust_b
> Â	intel_pstate_update_pstate(cpu, target_pstate);
> Â
> Â	sample = &cpu->sample;
> -	trace_pstate_sample(fp_toint(sample->core_pct_busy),
> +	trace_pstate_sample(fp_toint(100 * sample->core_avg_perf),
> Â		fp_toint(sample->busy_scaled),
> Â		from,
> Â		cpu->pstate.current_pstate,
> @@ -1332,7 +1328,7 @@ static void intel_pstate_update_util(str
> Â		bool sample_taken = intel_pstate_sample(cpu, time);
> Â
> Â		if (sample_taken) {
> -			intel_pstate_calc_busy(cpu);
> +			intel_pstate_calc_avg_perf(cpu);
> Â			if (!hwp_active)
> Â				intel_pstate_adjust_busy_pstate(cpu)
> ;
> Â		}
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm"
> in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info atÂÂhttp://vger.kernel.org/majordomo-info.html