Re: [PATCH v6 14/20] x86/split_lock: Add a sysfs interface to enable/disable split lock detection during run time

[Thomas Gleixner]

On Wed, 3 Apr 2019, Fenghua Yu wrote:
> +
> +static ssize_t
> +split_lock_detect_show(struct device *dev, struct device_attribute *attr,
> +		       char *buf)
> +{
> +	return sprintf(buf, "%u\n", READ_ONCE(split_lock_detect_val));

Please stop sprinkling READ_ONCE all over the place or can you explain why
this is in any way useful? You know what READ/WRITE_ONCE() is for, right?

> +}
> +
> +static ssize_t
> +split_lock_detect_store(struct device *dev, struct device_attribute *attr,
> +			const char *buf, size_t count)
> +{
> +	u32 val, l, h;
> +	int cpu, ret;
> +
> +	ret = kstrtou32(buf, 10, &val);
> +	if (ret)
> +		return ret;
> +
> +	if (val != DISABLE_SPLIT_LOCK_DETECT && val != ENABLE_SPLIT_LOCK_DETECT)
> +		return -EINVAL;

As this is really a simple boolean you can just use strtobool() and be done
with it.

> +
> +	/*
> +	 * Since split lock could be disabled by kernel #AC handler or user
> +	 * may directly change bit 29 in MSR_TEST_CTL, split lock setting on

The user can change bit 29 in that MSR? If you talk about /dev/msr then I
really do not care. That interface should die.

Aside of that your usage of the term 'user' is really misleading and
inconsistent all over the place.

> +	 * each CPU may be different from global setting split_lock_detect_val
> +	 * by now. Update MSR on each CPU, so all of CPUs will have same split
> +	 * lock setting.

That helps in which way? If #AC was detected in the kernel then

     1) It's likely to be switched off again right away

     2) The WARN_ONCE() already triggered and will not warn again.

So what's the point here, really? If the kernel triggers #AC, game
over. Fix the kernel first. If your kernel is clean, then why do you need
that knob at all?

> +	 */
> +	mutex_lock(&split_lock_detect_mutex);
> +
> +	WRITE_ONCE(split_lock_detect_val, val);

Oh well.

> +	/*
> +	 * Get MSR_TEST_CTL on this CPU, assuming all CPUs have same value
> +	 * in the MSR except split lock detection bit (bit 29).

And some day in the future this breaks because MRS_TEST_CTL has some other
shiny bits.

> +	 */
> +	rdmsr(MSR_TEST_CTL, l, h);
> +	l = new_sp_test_ctl_val(l);
> +	/* Update the split lock detection setting on all online CPUs. */
> +	for_each_online_cpu(cpu)

And what exactly protects the online cpu mask?

> +		wrmsr_on_cpu(cpu, MSR_TEST_CTL, l, h);

Oh well. Instead of just having a function which does:

fun() 
   	 if (ac_...enabled)
	 	msr_set_bit()
	 else
	 	msr_clear_bit()

and invoke that from cpu init code and from here via on_each_cpu() or such?

> +	mutex_unlock(&split_lock_detect_mutex);
> +
> +	return count;
> +}
> +
> +static DEVICE_ATTR_RW(split_lock_detect);
> +
> +static int __init split_lock_init(void)
> +{
> +	int ret;
> +
> +	if (!boot_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT))
> +		return -ENODEV;
> +
> +	ret = device_create_file(cpu_subsys.dev_root,
> +				 &dev_attr_split_lock_detect);
> +	if (ret)
> +		return ret;
> +
> +	return 0;

What's wrong with:

       return device_create_file();

??? Not hard enough to read, right?

> +}
> +

Pointless empty line.

> +subsys_initcall(split_lock_init);

Thanks,

	tglx