From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754113AbcEJBTK (ORCPT ); Mon, 9 May 2016 21:19:10 -0400 Received: from mga02.intel.com ([134.134.136.20]:38028 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752990AbcEJBTI (ORCPT ); Mon, 9 May 2016 21:19:08 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,602,1455004800"; d="scan'208";a="949635718" Message-ID: <1462843116.4224.16.camel@linux.intel.com> Subject: Re: [PATCH 1/3] intel_pstate: Clarify average performance computation From: Srinivas Pandruvada To: "Rafael J. Wysocki" , Linux PM list Cc: Linux Kernel Mailing List Date: Mon, 09 May 2016 18:18:36 -0700 In-Reply-To: <1880504.e9G30eDLBM@vostro.rjw.lan> References: <2638272.Bq3MnQEe4U@vostro.rjw.lan> <1880504.e9G30eDLBM@vostro.rjw.lan> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.18.3 (3.18.3-1.fc23) Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sat, 2016-05-07 at 01:44 +0200, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki > > 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 > --- >  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@vger.kernel.org > More majordomo info at  http://vger.kernel.org/majordomo-info.html