Messages in this thread | | | From | "Yu, Fenghua" <> | Subject | RE: [PATCH v2 3/3] x86/umwait: Control umwait maximum time | Date | Fri, 8 Feb 2019 18:51:20 +0000 |
| |
> 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
| |