From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755017AbcIKDG3 (ORCPT ); Sat, 10 Sep 2016 23:06:29 -0400 Received: from mga09.intel.com ([134.134.136.24]:4934 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752264AbcIKDG1 (ORCPT ); Sat, 10 Sep 2016 23:06:27 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.30,314,1470726000"; d="scan'208";a="166661515" From: "Pan, Harry" To: "tglx@linutronix.de" CC: "linux-kernel@vger.kernel.org" , "peterz@infradead.org" , "ray.huang@amd.com" , "x86@kernel.org" , "hpa@zytor.com" , "mingo@redhat.com" , "srinivas.pandruvada@linux.intel.com" , "bp@alien8.de" Subject: Re: [PATCH 2/2] perf/x86/rapl: Enable Baytrail/Braswell RAPL support Thread-Topic: [PATCH 2/2] perf/x86/rapl: Enable Baytrail/Braswell RAPL support Thread-Index: AQHSCqwO70QFBqa9eUG61lxaD6zHCaBwvUoAgAAvFACAADhjgIAB8rsA Date: Sun, 11 Sep 2016 03:06:22 +0000 Message-ID: <1473563180.9345.2.camel@intel.com> References: <1473433267-10153-1-git-send-email-harry.pan@intel.com> <1473433267-10153-2-git-send-email-harry.pan@intel.com> <1473443970.3685.20.camel@intel.com> In-Reply-To: Accept-Language: zh-TW, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.252.185.120] Content-Type: text/plain; charset="utf-8" Content-ID: MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id u8B36Y3e007800 Hi Thomas, Appreciate comments, I understood and learned. I just uploaded 3 patches integrated yours, yet there was 'git am' failure thus I did hand work, kindly double check. Sincerely, Harry On Fri, 2016-09-09 at 23:21 +0200, Thomas Gleixner wrote: > On Fri, 9 Sep 2016, Pan, Harry wrote: > > On Fri, 2016-09-09 at 17:11 +0200, Thomas Gleixner wrote: > > > > struct intel_rapl_init_fun { > > > > - bool apply_quirk; > > > > + enum rapl_quirk apply_quirk; > > > > > > This is silly. Make apply_quirk a function pointer and provide functions > > > for the different quirks. > > I read the rapl_check_hw_unit() as: read MSR_RAPL_POWER_UNIT, apply > > quirk if need, then estimate timer rate. > > > > In case to refine struct intel_rapl_init_fun adding callback, then > > either the quirk moving outside the rapl_check_hw_unit(), or replace > > input parameter as whole rapl_init in order to assess quirk callback, by > > far it looks to me centralize these two quirks inside this function more > > easily to maintain. > > If you have more than 3 quirks then the function becomes completely > unreadable while with a function pointer nobody has to touch it when adding > a new quirk. Neither do you have to update enums. > > > > Zero ininitalization has no real value other than consuming state space. > > To enable more than one quirk I extended bool to enum, I thought the > > __initconst space would be freed after kernel initialized, is there more > > detail concern I missed? > > I meant screen space. What's the point of zero initialization other than > consuming code lines and providing zero information? > > But instead of arguing with you in circles I took the 5 minutes to make it > function pointer based. Patch below. > > Thanks, > > tglx > > 8<------------------- > > Subject: x86/perf/rapl: Make quirk a function pointer > From: Thomas Gleixner > Date: Fri, 09 Sep 2016 22:46:17 +0200 > > There are more model specific quirks required. So we need to change the > single purpose boolean quirk flag to an easy extensible mechanism. > > Make the quirk a function pointer and move the existing quirk into its own > function. > > While at it make the init struct initializers readable and rename the > misnomed intel_rapl_hw_init_fun struct to intel_rapl_model_desc because > that's what it is a cpu model descriptor for the rapl features specific to > a particular model. > > Signed-off-by: Thomas Gleixner > --- > arch/x86/events/intel/rapl.c | 92 +++++++++++++++++++++---------------------- > 1 file changed, 46 insertions(+), 46 deletions(-) > > --- a/arch/x86/events/intel/rapl.c > +++ b/arch/x86/events/intel/rapl.c > @@ -152,6 +152,12 @@ struct rapl_pmus { > struct rapl_pmu *pmus[]; > }; > > +struct intel_rapl_model_desc { > + void (*quirk)(void);; > + int cntr_mask; > + struct attribute **attrs; > +}; > + > /* 1/2^hw_unit Joule */ > static int rapl_hw_unit[NR_RAPL_DOMAINS] __read_mostly; > static struct rapl_pmus *rapl_pmus; > @@ -617,7 +623,18 @@ static int rapl_cpu_prepare(unsigned int > return 0; > } > > -static int rapl_check_hw_unit(bool apply_quirk) > +static void rapl_hsx_quirk(void) > +{ > + /* > + * DRAM domain on HSW server and KNL has fixed energy unit which can be > + * different than the unit from power unit MSR. See > + * "Intel Xeon Processor E5-1600 and E5-2600 v3 Product Families, V2 > + * of 2. Datasheet, September 2014, Reference Number: 330784-001 " > + */ > + rapl_hw_unit[RAPL_IDX_RAM_NRG_STAT] = 16; > +} > + > +static int rapl_check_hw_unit(const struct intel_rapl_model_desc *model) > { > u64 msr_rapl_power_unit_bits; > int i; > @@ -628,14 +645,9 @@ static int rapl_check_hw_unit(bool apply > for (i = 0; i < NR_RAPL_DOMAINS; i++) > rapl_hw_unit[i] = (msr_rapl_power_unit_bits >> 8) & 0x1FULL; > > - /* > - * DRAM domain on HSW server and KNL has fixed energy unit which can be > - * different than the unit from power unit MSR. See > - * "Intel Xeon Processor E5-1600 and E5-2600 v3 Product Families, V2 > - * of 2. Datasheet, September 2014, Reference Number: 330784-001 " > - */ > - if (apply_quirk) > - rapl_hw_unit[RAPL_IDX_RAM_NRG_STAT] = 16; > + /* Apply quirk before initializing the timer rate */ > + if (model->quirk) > + model->quirk(); > > /* > * Calculate the timer rate: > @@ -701,46 +713,36 @@ static int __init init_rapl_pmus(void) > #define X86_RAPL_MODEL_MATCH(model, init) \ > { X86_VENDOR_INTEL, 6, model, X86_FEATURE_ANY, (unsigned long)&init } > > -struct intel_rapl_init_fun { > - bool apply_quirk; > - int cntr_mask; > - struct attribute **attrs; > -}; > - > -static const struct intel_rapl_init_fun snb_rapl_init __initconst = { > - .apply_quirk = false, > - .cntr_mask = RAPL_IDX_CLN, > - .attrs = rapl_events_cln_attr, > +static const struct intel_rapl_model_desc snb_rapl_init __initconst = { > + .cntr_mask = RAPL_IDX_CLN, > + .attrs = rapl_events_cln_attr, > }; > > -static const struct intel_rapl_init_fun hsx_rapl_init __initconst = { > - .apply_quirk = true, > - .cntr_mask = RAPL_IDX_SRV, > - .attrs = rapl_events_srv_attr, > +static const struct intel_rapl_model_desc hsx_rapl_init __initconst = { > + .quirk = rapl_hsx_quirk, > + .cntr_mask = RAPL_IDX_SRV, > + .attrs = rapl_events_srv_attr, > }; > > -static const struct intel_rapl_init_fun hsw_rapl_init __initconst = { > - .apply_quirk = false, > - .cntr_mask = RAPL_IDX_HSW, > - .attrs = rapl_events_hsw_attr, > +static const struct intel_rapl_model_desc hsw_rapl_init __initconst = { > + .cntr_mask = RAPL_IDX_HSW, > + .attrs = rapl_events_hsw_attr, > }; > > -static const struct intel_rapl_init_fun snbep_rapl_init __initconst = { > - .apply_quirk = false, > - .cntr_mask = RAPL_IDX_SRV, > - .attrs = rapl_events_srv_attr, > +static const struct intel_rapl_model_desc snbep_rapl_init __initconst = { > + .cntr_mask = RAPL_IDX_SRV, > + .attrs = rapl_events_srv_attr, > }; > > -static const struct intel_rapl_init_fun knl_rapl_init __initconst = { > - .apply_quirk = true, > - .cntr_mask = RAPL_IDX_KNL, > - .attrs = rapl_events_knl_attr, > +static const struct intel_rapl_model_desc knl_rapl_init __initconst = { > + .quirk = rapl_hsx_quirk, > + .cntr_mask = RAPL_IDX_KNL, > + .attrs = rapl_events_knl_attr, > }; > > -static const struct intel_rapl_init_fun skl_rapl_init __initconst = { > - .apply_quirk = false, > - .cntr_mask = RAPL_IDX_SKL_CLN, > - .attrs = rapl_events_skl_attr, > +static const struct intel_rapl_model_desc skl_rapl_init __initconst = { > + .cntr_mask = RAPL_IDX_SKL_CLN, > + .attrs = rapl_events_skl_attr, > }; > > static const struct x86_cpu_id rapl_cpu_match[] __initconst = { > @@ -772,21 +774,19 @@ MODULE_DEVICE_TABLE(x86cpu, rapl_cpu_mat > > static int __init rapl_pmu_init(void) > { > + const struct intel_rapl_model_desc *model; > const struct x86_cpu_id *id; > - struct intel_rapl_init_fun *rapl_init; > - bool apply_quirk; > int ret; > > id = x86_match_cpu(rapl_cpu_match); > if (!id) > return -ENODEV; > > - rapl_init = (struct intel_rapl_init_fun *)id->driver_data; > - apply_quirk = rapl_init->apply_quirk; > - rapl_cntr_mask = rapl_init->cntr_mask; > - rapl_pmu_events_group.attrs = rapl_init->attrs; > + model = (struct intel_rapl_model_desc*)id->driver_data; > + rapl_cntr_mask = model->cntr_mask; > + rapl_pmu_events_group.attrs = model->attrs; > > - ret = rapl_check_hw_unit(apply_quirk); > + ret = rapl_check_hw_unit(model); > if (ret) > return ret; >