RE: [PATCH v2 3/3] x86/umwait: Control umwait maximum time

["Yu, Fenghua" <fenghua.yu@intel.com>]

> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> On 17/01/2019 00:00, Andy Lutomirski wrote:
> > On Wed, Jan 16, 2019 at 1:24 PM Fenghua Yu <fenghua.yu@intel.com>
> wrote:
> >> IA32_UMWAIT_CONTROL[31:2] determines the maximum time in TSC-
> quanta
> >> that processor can stay in C0.1 or C0.2.
> >>
> >> The maximum time value in IA32_UMWAIT_CONTROL[31-2] is set as zero
> >> which means there is no global time limit for UMWAIT and TPAUSE
> instructions.
> >> Each process sets its own umwait maximum time as the instructions
> operand.
> >>
> >> User can specify global umwait maximum time through interface:
> >> /sys/devices/system/cpu/umwait_control/umwait_max_time
> >> The value in the interface is in decimal in TSC-quanta. Bits[1:0] are
> >> cleared when the value is stored.
> >>
> >> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> >> ---
> >>  arch/x86/include/asm/msr-index.h |  2 ++
> >>  arch/x86/power/umwait.c          | 42
> +++++++++++++++++++++++++++++++-
> >>  2 files changed, 43 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/arch/x86/include/asm/msr-index.h
> >> b/arch/x86/include/asm/msr-index.h
> >> index b56bfecae0de..42b9104fc15b 100644
> >> --- a/arch/x86/include/asm/msr-index.h
> >> +++ b/arch/x86/include/asm/msr-index.h
> >> @@ -62,6 +62,8 @@
> >>  #define MSR_IA32_UMWAIT_CONTROL                0xe1
> >>  #define UMWAIT_CONTROL_C02_BIT         0x0
> >>  #define UMWAIT_CONTROL_C02_MASK                0x00000001
> >> +#define UMWAIT_CONTROL_MAX_TIME_BIT    0x2
> >> +#define UMWAIT_CONTROL_MAX_TIME_MASK   0xfffffffc
> >>
> >>  #define MSR_PKG_CST_CONFIG_CONTROL     0x000000e2
> >>  #define NHM_C3_AUTO_DEMOTE             (1UL << 25)
> >> diff --git a/arch/x86/power/umwait.c b/arch/x86/power/umwait.c index
> >> 95b3867aac1e..4a1a507d3bb7 100644
> >> --- a/arch/x86/power/umwait.c
> >> +++ b/arch/x86/power/umwait.c
> >> @@ -10,6 +10,7 @@
> >>  #include <asm/msr.h>
> >>
> >>  static int umwait_enable_c0_2 = 1; /* 0: disable C0.2. 1: enable
> >> C0.2. */
> >> +static u32 umwait_max_time; /* In TSC-quanta. Only bits [31:2] are
> >> +used. */
> >>  static DEFINE_MUTEX(umwait_lock);
> >>
> >>  /* Return value that will be used to set umwait control MSR */ @@
> >> -20,7 +21,8 @@ static inline u32 umwait_control_val(void)
> >>          * When bit 0 is 1, C0.2 is disabled. Otherwise, C0.2 is enabled.
> >>          * So value in bit 0 is opposite of umwait_enable_c0_2.
> >>          */
> >> -       return ~umwait_enable_c0_2 & UMWAIT_CONTROL_C02_MASK;
> >> +       return (~umwait_enable_c0_2 & UMWAIT_CONTROL_C02_MASK) |
> >> +              umwait_max_time;
> >>  }
> >>
> >>  static ssize_t umwait_enable_c0_2_show(struct device *dev, @@ -61,8
> >> +63,46 @@ static ssize_t umwait_enable_c0_2_store(struct device *dev,
> >>
> >>  static DEVICE_ATTR_RW(umwait_enable_c0_2);
> >>
> >> +static ssize_t umwait_max_time_show(struct device *kobj,
> >> +                                   struct device_attribute *attr,
> >> +char *buf) {
> >> +       return sprintf(buf, "%u\n", umwait_max_time); }
> >> +
> >> +static ssize_t umwait_max_time_store(struct device *kobj,
> >> +                                    struct device_attribute *attr,
> >> +                                    const char *buf, size_t count) {
> >> +       u32 msr_val, max_time;
> >> +       int cpu, ret;
> >> +
> >> +       ret = kstrtou32(buf, 10, &max_time);
> >> +       if (ret)
> >> +               return ret;
> >> +
> >> +       mutex_lock(&umwait_lock);
> >> +
> >> +       /* Only get max time value from bits [31:2] */
> >> +       max_time &= UMWAIT_CONTROL_MAX_TIME_MASK;
> >> +       /* Update the max time value in memory */
> >> +       umwait_max_time = max_time;
> >> +       msr_val = umwait_control_val();
> >> +       get_online_cpus();
> >> +       /* All CPUs have same umwait max time */
> >> +       for_each_online_cpu(cpu)
> >> +               wrmsr_on_cpu(cpu, MSR_IA32_UMWAIT_CONTROL, msr_val,
> 0);
> >> +       put_online_cpus();
> >> +
> >> +       mutex_unlock(&umwait_lock);
> >> +
> >> +       return count;
> >> +}
> >> +
> >> +static DEVICE_ATTR_RW(umwait_max_time);
> >> +
> >>  static struct attribute *umwait_attrs[] = {
> >>         &dev_attr_umwait_enable_c0_2.attr,
> >> +       &dev_attr_umwait_max_time.attr,
> >>         NULL
> >>  };
> > You need something to make sure that newly onlined CPUs get the right
> > value in the MSR.  You also need to make sure you restore it on resume
> > from suspend.  Something like cpu_init() might be the right place.
> >
> > Also, as previously discussed, I think we should set the default to
> > something quite small, maybe 100 microseconds.  IMO the goal is to
> > pick a value that is a high enough multiple of the C0.2 entry+exit
> > latency that we get most of the power and SMT resource savings while
> > being small enough that no one things that UMWAIT is more than a
> > glorified, slightly improved, and far more misleading version of REP
> > NOP.
> >
> > Andrew, would having Linux default to a small value do much to
> > mitigate your concerns that UMWAIT is problematic for hypervisors?
> 
> Sadly no - not really.
> 
> Being an MSR, there is no way the guest kernel is having unfiltered access,
> so the hypervisor can set whatever bound it wishes.
> 
> For any non-trivial wait period, it would be better for the system as a whole
> to switch to a different vcpu, but the semantics don't allow for
> that.  Shortening the timeout just results in userspace taking over again,
> and most likely concluding that there was an early wakeup and going back
> to sleep.
> 
> More useful semantics would be something similar to pause-loop-exiting so
> we can swap contexts while the processor is logically idle in userspace.

So do we still keep the umwait max time out value as 0 which means there is no global time out for umwait?
Sys admin can always change it to different time out based on usage.

BTW, latency exiting from umwait/tpause varies depending on sleeping in C0.1 or C0.2 states.
On machine, it shows a few cycles to hundreds cycles. But I guess it could be different on different machine as well. So I guess it's hard to get a uniform latency value and use it.

Thanks.

-Fenghua