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
