From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751872AbcEKFBl (ORCPT ); Wed, 11 May 2016 01:01:41 -0400 Received: from mga03.intel.com ([134.134.136.65]:12685 "EHLO mga03.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751699AbcEKFBk (ORCPT ); Wed, 11 May 2016 01:01:40 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,607,1455004800"; d="scan'208";a="803622640" Message-ID: <1462942869.4224.25.camel@linux.intel.com> Subject: Re: [PATCH 1/3] intel_pstate: Clarify average performance computation From: Srinivas Pandruvada To: "Rafael J. Wysocki" Cc: "Rafael J. Wysocki" , Linux PM list , Linux Kernel Mailing List Date: Tue, 10 May 2016 22:01:09 -0700 In-Reply-To: <1703361.r4IIOaCPTn@vostro.rjw.lan> References: <2638272.Bq3MnQEe4U@vostro.rjw.lan> <1462910284.28729.116.camel@linux.intel.com> <1703361.r4IIOaCPTn@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 Tue, 2016-05-10 at 22:57 +0200, Rafael J. Wysocki wrote: > > > > [...] > --- > From: Rafael J. Wysocki > 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 > --- >  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