Messages in this thread Patch in this message | | | Date | Mon, 17 Jun 2019 19:32:22 -0700 | From | Fenghua Yu <> | Subject | Re: [PATCH v4 3/5] x86/umwait: Add sysfs interface to control umwait C0.2 state |
| |
On Mon, Jun 17, 2019 at 05:19:02PM -0700, Andy Lutomirski wrote: > On Mon, Jun 17, 2019 at 5:09 PM Fenghua Yu <fenghua.yu@intel.com> wrote: > But you're already using a mutex and a comment. And you're hoping > that the syscore resume callback reads something sensible despite the > lack of READ_ONCE / WRITE_ONCE. The compiler is unlikely to butcher > this too badly, but still.
You are right, syscore_resume will be wrong if suspend in middle of sysfs writing.
Ok. I change this patch based on your proposed locking. Is this patch right? Should I use WRITE_ONCE/READ_ONCE on each access of umwait_control_cached?
Thanks.
-Fenghua
diff --git a/arch/x86/power/umwait.c b/arch/x86/power/umwait.c index 9594af9f657e..d17572605c1a 100644 --- a/arch/x86/power/umwait.c +++ b/arch/x86/power/umwait.c @@ -11,10 +11,34 @@ */ static u32 umwait_control_cached = 100000 & ~MSR_IA32_UMWAIT_CONTROL_C02_DISABLED; +/* + * Serialize access to umwait_control_cached and IA32_UMWAIT_CONTROL MSR + * in writing sysfs to ensure all CPUs have the same MSR value. + */ +static DEFINE_MUTEX(umwait_lock); + +static void update_this_cpu_umwait_control_msr(void) +{ + unsigned long flags; + + /* + * We need to prevent umwait_control_cached from being changed *and* + * completing its WRMSR between our read and our WRMSR. By turning + * IRQs off here, ensure that no sysfs write happens on this CPU + * and we also make sure that any concurrent sysfs write from a + * different CPU will not finish updating us via IPI until we're done. + */ + local_irq_save(flags); + + wrmsr(MSR_IA32_UMWAIT_CONTROL, READ_ONCE(umwait_control_cached), 0); + + local_irq_restore(flags); +} + /* Set up IA32_UMWAIT_CONTROL MSR on CPU using the current global setting. */ static int umwait_cpu_online(unsigned int cpu) { - wrmsr(MSR_IA32_UMWAIT_CONTROL, umwait_control_cached, 0); + update_this_cpu_umwait_control_msr(); return 0; } @@ -30,24 +54,102 @@ static int umwait_cpu_online(unsigned int cpu) */ static void umwait_syscore_resume(void) { - wrmsr(MSR_IA32_UMWAIT_CONTROL, umwait_control_cached, 0); + update_this_cpu_umwait_control_msr(); } static struct syscore_ops umwait_syscore_ops = { .resume = umwait_syscore_resume, }; +static void umwait_control_msr_update(void *unused) +{ + update_this_cpu_umwait_control_msr(); +} + +static u32 get_umwait_control_c02(void) +{ + return READ_ONCE(umwait_control_cached) & MSR_IA32_UMWAIT_CONTROL_C02_DISABLED; +} + +static u32 get_umwait_control_max_time(void) +{ + return READ_ONCE(umwait_control_cached) & MSR_IA32_UMWAIT_CONTROL_MAX_TIME; +} + +static ssize_t +enable_c02_show(struct device *dev, struct device_attribute *attr, char *buf) +{ + /* + * When bit 0 in IA32_UMWAIT_CONTROL MSR is 1, C0.2 is disabled. + * Otherwise, C0.2 is enabled. Show the opposite of bit 0. + */ + return sprintf(buf, "%d\n", !(bool)get_umwait_control_c02()); +} + +static ssize_t enable_c02_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + u32 umwait_control_c02; + bool c02_enabled; + int ret; + + ret = kstrtobool(buf, &c02_enabled); + if (ret) + return ret; + + mutex_lock(&umwait_lock); + + /* + * The value of bit 0 in IA32_UMWAIT_CONTROL MSR is opposite of + * c02_enabled. + */ + umwait_control_c02 = (u32)!c02_enabled; + if (umwait_control_c02 == get_umwait_control_c02()) + goto out_unlock; + + WRITE_ONCE(umwait_control_cached, umwait_control_c02 | get_umwait_control_max_time()); + /* Enable/disable C0.2 state on all CPUs */ + on_each_cpu(umwait_control_msr_update, NULL, 1); + +out_unlock: + mutex_unlock(&umwait_lock); + + return count; +} +static DEVICE_ATTR_RW(enable_c02); + +static struct attribute *umwait_attrs[] = { + &dev_attr_enable_c02.attr, + NULL +}; + +static struct attribute_group umwait_attr_group = { + .attrs = umwait_attrs, + .name = "umwait_control", +}; + static int __init umwait_init(void) { + struct device *dev; int ret; if (!boot_cpu_has(X86_FEATURE_WAITPKG)) return -ENODEV; + /* Add umwait control interface. */ + dev = cpu_subsys.dev_root; + ret = sysfs_create_group(&dev->kobj, &umwait_attr_group); + if (ret) + return ret; + ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "umwait/intel:online", umwait_cpu_online, NULL); - if (ret < 0) + if (ret < 0) { + sysfs_remove_group(&dev->kobj, &umwait_attr_group); + return ret; + } register_syscore_ops(&umwait_syscore_ops); -- 2.19.1
| |