Re: [PATCH v4 2/5] x86/umwait: Initialize umwait control values

[Thomas Gleixner]

On Sun, 9 Jun 2019, Fenghua Yu wrote:

> On Sat, Jun 08, 2019 at 03:52:42PM -0700, Andy Lutomirski wrote:
> > On Fri, Jun 7, 2019 at 3:10 PM Fenghua Yu <fenghua.yu@xxxxxxxxx> wrote:
> > >
> > > umwait or tpause allows processor to enter a light-weight
> > > power/performance optimized state (C0.1 state) or an improved
> > > power/performance optimized state (C0.2 state) for a period
> > > specified by the instruction or until the system time limit or until
> > > a store to the monitored address range in umwait.
> > >
> > > IA32_UMWAIT_CONTROL MSR register allows kernel to enable/disable C0.2
> > > on the processor and set maximum time the processor can reside in
> > > C0.1 or C0.2.
> > >
> > > By default C0.2 is enabled so the user wait instructions can enter the
> > > C0.2 state to save more power with slower wakeup time.
> > 
> > Sounds good, but:
> > 
> > > +#define MSR_IA32_UMWAIT_CONTROL_C02            BIT(0)
> > 
> > > +static u32 umwait_control_cached = 100000;
> > 
> > The code seems to disagree.
> 
> The definition of bit[0] is: C0.2 is disabled when bit[0]=1. So
> 100000 means C0.2 is enabled (and max time is 100000).

which is totally non obvious. If you have to encode the control bit, then
please make it explicit, i.e. mask out the disable bit in the initializer.

Thanks,

	tglx