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,
> > };
> >
