Re: [PATCH v5 4/5] x86/umwait: Add sysfs interface to control umwait maximum time

[Thomas Gleixner <tglx@linutronix.de>]

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