Re: [PATCH 2/2] perf/x86/rapl: Enable Baytrail/Braswell RAPL support

[Pan, Harry]

Hi Peter,

Totally agreed and uploaded patchset again.
https://lkml.org/lkml/2016/9/9/467
https://lkml.org/lkml/2016/9/9/468
One more thing, I did not refine rapl_advertise() description.
Advice is welcome.

Sincerely,
Harry

On Fri, 2016-09-09 at 11:29 +0200, Peter Zijlstra wrote:
> On Thu, Sep 08, 2016 at 05:08:58PM +0800, Harry Pan wrote:
> 
> > @@ -177,6 +187,16 @@ static inline u64 rapl_scale(u64 v, int cfg)
> >  		pr_warn("Invalid domain %d, failed to scale data\n", cfg);
> >  		return v;
> >  	}
> > +
> > +	/*
> > +	 * Some Atom series processors (BYT/BSW) use 2^ESU microjoules.
> > +	 *
> > +	 * TODO: this looks hacky, it's better to refactor scale-up mechanism
> > +	 * to compromise the main stream processors and Atom ones.
> > +	 */
> > +	if (is_baytrail)
> > +		return v << rapl_hw_unit[cfg - 1];
> > +
> 
> Can't you simply set rapl_hw_unit[] such that 32 - rapl_hw_unit[] ends
> up at the right number? Then you only get to much with values in
> rapl_check_hw_unit without runtime overhead later.
> 
> > +static int rapl_check_hw_unit(enum rapl_quirk apply_quirk)
> >  {
> >  	u64 msr_rapl_power_unit_bits;
> >  	int i;
> > @@ -634,10 +674,20 @@ static int rapl_check_hw_unit(bool apply_quirk)
> >  	 * "Intel Xeon Processor E5-1600 and E5-2600 v3 Product Families, V2
> >  	 * of 2. Datasheet, September 2014, Reference Number: 330784-001 "
> >  	 */
> > -	if (apply_quirk)
> > +	if (apply_quirk == RAPL_HSX_QUIRK)
> >  		rapl_hw_unit[RAPL_IDX_RAM_NRG_STAT] = 16;
> >  
> >  	/*
> > +	 * Some Atom processors (BYT/BSW) have 2^ESU microjoules increment,
> > +	 * refer to Software Developers' Manual, Vol. 3C, Order No. 325384,
> > +	 * Table 35-8 of MSR_RAPL_POWER_UNIT
> > +	 */
> > +	if (apply_quirk == RAPL_BYT_QUIRK)
> > +		is_baytrail = true;
> > +	else
> > +		is_baytrail = false;
> 
> it was already false...
> 
> 	/*
> 	 * comment explaining quirk goes here...
> 	 */
> 	if (apply_quirk = RAPL_BYT_QUIRK) {
> 		for (i = 0; i < NR_RAPL_DOMAINS; i++)
> 			rapl_hw_unit[i] = 32 - rapl_hw_unit[i];
> 	}
> 
> and then you get to verify what to do with rapl_timer_ms.
> 
> 
> 
> >  static const struct intel_rapl_init_fun snb_rapl_init __initconst = {
> > -	.apply_quirk = false,
> > +	.apply_quirk = 0,
> 
> Either leave it out (unmentioned members get initialized to 0) or add
> RAPL_NO_QUIRK or so.
> 
> >  	.cntr_mask = RAPL_IDX_CLN,
> >  	.attrs = rapl_events_cln_attr,
> >  };
> >