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

[Fenghua Yu]

On Thu, Apr 25, 2019 at 08:31:15AM +0200, Ingo Molnar wrote:
> 
> * Fenghua Yu <fenghua.yu@xxxxxxxxx> wrote:
> 
> > +		disabled if split lock operation in kernel code happens on
> > +		the CPU. The interface doesn't show or control split lock
> > +		detection on individual CPU.
> 
> I.e. implementation and possible actual state are out of sync. Why?
> 
> Also, if it's a global flag, why waste memory on putting a sysfs knob 
> into every CPU's sysfs file?
> 
> Finally, why is a debugging facility in sysfs, why not a debugfs knob? 
> Using a sysctl would solve the percpu vs. global confusion as well ...

Can I put the interface in /sys/kernel/debug/x86/split_lock_detect?

> 
> > --- a/arch/x86/kernel/cpu/intel.c
> > +++ b/arch/x86/kernel/cpu/intel.c
> > @@ -35,6 +35,7 @@
> >  DEFINE_PER_CPU(u64, msr_test_ctl_cache);
> >  EXPORT_PER_CPU_SYMBOL_GPL(msr_test_ctl_cache);
> >  
> > +static DEFINE_MUTEX(split_lock_detect_mutex);
> >  static bool split_lock_detect_enable;
> 
> 'enable' is a verb in plain form - which we use for function names.
> 
> For variable names that denotes current state we typically use past 
> tense, i.e. 'enabled'.
> 
> (The only case where we'd us the split_lock_detect_enable name for a flag 
> if it's a flag to trigger some sort of enabling action - which this 
> isn't.)
> 
> Please review the whole series for various naming mishaps.
OK.

> 
> > +	mutex_lock(&split_lock_detect_mutex);
> > +
> > +	split_lock_detect_enable = val;
> > +
> > +	/* Update the split lock detection setting in MSR on all online CPUs. */
> > +	on_each_cpu(split_lock_update_msr, NULL, 1);
> > +
> > +	if (split_lock_detect_enable)
> > +		pr_info("enabled\n");
> > +	else
> > +		pr_info("disabled\n");
> > +
> > +	mutex_unlock(&split_lock_detect_mutex);
> 
> Instead of a mutex, please just use the global atomic debug flag which 
> controls the warning printout. By using that flag both for the WARN()ing 
> and for controlling MSR state all the races are solved and the code is 
> simplified.

So is it OK to define split_lock_debug and use it in #AC handler and in
here?

static atomic_t split_lock_debug;

in #AC handler:

+       if (atomic_cmpxchg(&split_lock_debug, 0, 1) == 0) {
+               /* Only warn split lock once */
+               WARN_ONCE(1, "split lock operation detected\n");
+               atomic_set(&split_lock_debug, 0);
+       }

And in split_lock_detect_store(), replace the mutex with split_lock_debug
like this:
 
-       mutex_lock(&split_lock_detect_mutex);
+       while (atomic_cmpxchg(&split_lock_debug, 1, 0))
+              cpu_relax();
.... 
-       mutex_unlock(&split_lock_detect_mutex);
+       atomic_set(&split_lock_debug, 0);
 
Is this right code for sync in both #AC handler and in
split_lock_detect_store()?

Thanks.

-Fenghua