Messages in this thread | | | Date | Mon, 24 Jun 2019 00:40:33 +0200 (CEST) | From | Thomas Gleixner <> | Subject | Re: [PATCH v5 4/5] x86/umwait: Add sysfs interface to control umwait maximum time |
| |
On Wed, 19 Jun 2019, Fenghua Yu wrote:
> Users can write an unsigned 32-bit number to > /sys/devices/system/cpu/umwait_control/max_time to change the default
Users? Administrators can. Users NOT.
> value. Note that a value of zero means there is no limit. Low order > two bits must be zero.
...
> +static ssize_t max_time_store(struct device *kobj, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + u32 max_time; > + int ret; > + > + ret = kstrtou32(buf, 0, &max_time); > + if (ret) > + return ret; > + > + /* bits[1:0] must be zero */ > + if (max_time & ~MSR_IA32_UMWAIT_CONTROL_MAX_TIME) > + return -EINVAL; > + > + mutex_lock(&umwait_lock); > + > + if (max_time == get_umwait_ctrl_max_time()) > + goto out_unlock; > + > + WRITE_ONCE(umwait_control_cached, > + UMWAIT_CTRL_VAL(max_time, get_umwait_ctrl_c02()));
Same convoluted logic with reading the cached value twice to confuse the reader.
ctrl = READ_ONCE(umwait_control_cached); if (max_time == umwait_ctrl_max_time(ctrl)) goto out_unlock;
ctrl = (ctrl & ~MSR_IA32_UMWAIT_CONTROL_TIME_MASK) | max_time; WRITE_ONCE(umwait_control_cached, ctrl);
Simple, right?
But this can be done even simpler with a shared update function:
static void umwait_update_control(u32 maxtime, bool c02_enable) { u32 ctrl = maxtime & MSR_IA32_UMWAIT_CONTROL_TIME_MASK;
if (!c02_enable) ctrl |= MSR_IA32_UMWAIT_CONTROL_C02_DISABLE;
WRITE_ONCE(umwait_control_cached, ctrl); /* Propagate to all CPUs */ on_each_cpu(umwait_update_control_msr, NULL, 1); }
With that both functions become trivial and do not have duplicated code.
Thanks,
tglx
| |