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
