| Message ID | 1554326526-172295-14-git-send-email-fenghua.yu@intel.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | x86/split_lock: Enable split locked accesses detection | expand |
On Wed, 3 Apr 2019, Fenghua Yu wrote: > A split locked access locks bus and degrades overall memory access > performance. When split lock detection feature is enumerated, enable > the feature by default to find any split lock issue and then fix > the issue. Enabling the feature allows to find the issues, but does not automagically fix them. Come on. > +#define DISABLE_SPLIT_LOCK_DETECT 0 > +#define ENABLE_SPLIT_LOCK_DETECT 1 If those defines have a value at all, please start with the facility not with functionality, i.e. AC_SPLIT_LOCK_ENABLE.... > + > +static DEFINE_MUTEX(split_lock_detect_mutex); > +static int split_lock_detect_val; detect_val? What value is that? Its supposed to hold those magic defines above. So something like static unsigned int ac_split_lock_enable; > /* > * Just in case our CPU detection goes bad, or you have a weird system, > * allow a way to override the automatic disabling of MPX. > @@ -161,10 +167,45 @@ static bool bad_spectre_microcode(struct cpuinfo_x86 *c) > return false; > } > > +static u32 new_sp_test_ctl_val(u32 test_ctl_val) > +{ > + /* Change the split lock setting. */ > + if (READ_ONCE(split_lock_detect_val) == DISABLE_SPLIT_LOCK_DETECT) That READ_ONCE() is required because? > + test_ctl_val &= ~TEST_CTL_ENABLE_SPLIT_LOCK_DETECT; > + else > + test_ctl_val |= TEST_CTL_ENABLE_SPLIT_LOCK_DETECT; > + > + return test_ctl_val; > +} Aside of that do we really need a misnomed function which replaces the simple inline code at the call site: rdmsr(l, h) l &= ~TEST_CTL_ENABLE_SPLIT_LOCK_DETECT; l |= ac_split_lock_enable << TEST_CTL_ENABLE_SPLIT_LOCK_DETECT_SHIFT; wrmrs(...) or the even more simple if (ac_split_lock_enable) msr_set_bit(...) else msr_clear_nit(...) Hmm? > + > +static inline void show_split_lock_detection_info(void) > +{ > + if (READ_ONCE(split_lock_detect_val)) That READ_ONCE() is required because? > + pr_info_once("x86/split_lock: split lock detection enabled\n"); > + else > + pr_info_once("x86/split_lock: split lock detection disabled\n"); pr_fmt exists for a reason and having 'split lock' repeated several times in the same line is not making it more readable. > +} > + > +static void init_split_lock_detect(struct cpuinfo_x86 *c) > +{ > + if (cpu_has(c, X86_FEATURE_SPLIT_LOCK_DETECT)) { > + u32 l, h; > + > + mutex_lock(&split_lock_detect_mutex); > + rdmsr(MSR_TEST_CTL, l, h); > + l = new_sp_test_ctl_val(l); > + wrmsr(MSR_TEST_CTL, l, h); > + show_split_lock_detection_info(); > + mutex_unlock(&split_lock_detect_mutex); > + } > +} > + > static void early_init_intel(struct cpuinfo_x86 *c) > { > u64 misc_enable; > > + init_split_lock_detect(c); so we have in early boot: early_cpu_init() early_identify_cpu() this_cpu->c_early_init(c) early_init_intel() { init_split_lock_detect(); } .... cpu_set_core_cap_bits(c) set(FEATURE_SPLIT_LOCK) I don't have to understand how init_split_lock_detect() will magically see the feature bit which gets set afterwards, right? > + > /* Unmask CPUID levels if masked: */ > if (c->x86 > 6 || (c->x86 == 6 && c->x86_model >= 0xd)) { > if (msr_clear_bit(MSR_IA32_MISC_ENABLE, > @@ -1032,6 +1073,7 @@ cpu_dev_register(intel_cpu_dev); > static void __init set_split_lock_detect(void) > { > setup_force_cpu_cap(X86_FEATURE_SPLIT_LOCK_DETECT); > + split_lock_detect_val = 1; Oh well. You add defines on top of the file and then you don't use them. Thanks, tglx
On Thu, 4 Apr 2019, Thomas Gleixner wrote: > On Wed, 3 Apr 2019, Fenghua Yu wrote: > > +static void init_split_lock_detect(struct cpuinfo_x86 *c) > > +{ > > + if (cpu_has(c, X86_FEATURE_SPLIT_LOCK_DETECT)) { > > + u32 l, h; > > + > > + mutex_lock(&split_lock_detect_mutex); The mutex protects what exactly in the cpu bringup code? Thanks, tglx
On Thu, Apr 04, 2019 at 08:07:57PM +0200, Thomas Gleixner wrote: > On Wed, 3 Apr 2019, Fenghua Yu wrote: > > > A split locked access locks bus and degrades overall memory access > > performance. When split lock detection feature is enumerated, enable > > the feature by default to find any split lock issue and then fix > > the issue. > > Enabling the feature allows to find the issues, but does not automagically > fix them. Come on. Ok. I will remove the "and then fix the issue". > > > +#define DISABLE_SPLIT_LOCK_DETECT 0 > > +#define ENABLE_SPLIT_LOCK_DETECT 1 > > If those defines have a value at all, please start with the facility not > with functionality, i.e. AC_SPLIT_LOCK_ENABLE.... OK. > > > + > > +static DEFINE_MUTEX(split_lock_detect_mutex); > > +static int split_lock_detect_val; > > detect_val? What value is that? According to previous discussions, I was told to call this split lock feature as "split lock detection" instead of "#AC for split lock". So I use "split_lock_detect..." in variable names or function names, call feature flag as "split_lock_detect", and call the feature as "split lock detection" in descriptions. If you don't agree to name feature as "split lock detection", I can change variable names/function names/feature flag/descriptions etc back to previous names "ac_split_lock...", "#AC for split lock", etc. The variable split_lock_detect_val is either 0 or 1. It stores current enable/disable status of split lock detection feature. By default it's one after the feature is enumerated. Then sysadmin can change it to 0 or 1 to enable or disable the feature during run time. > Its supposed to hold those magic defines > above. So something like > > static unsigned int ac_split_lock_enable; If you agree to name the split lock feature as "split lock detection" feature, can I change this variable to static unsigned int split_lock_detect_enable? > > /* > > * Just in case our CPU detection goes bad, or you have a weird system, > > * allow a way to override the automatic disabling of MPX. > > @@ -161,10 +167,45 @@ static bool bad_spectre_microcode(struct cpuinfo_x86 *c) > > return false; > > } > > > > +static u32 new_sp_test_ctl_val(u32 test_ctl_val) > > +{ > > + /* Change the split lock setting. */ > > + if (READ_ONCE(split_lock_detect_val) == DISABLE_SPLIT_LOCK_DETECT) > > That READ_ONCE() is required because? Ok. Will remove READ_ONCE(). > > > + test_ctl_val &= ~TEST_CTL_ENABLE_SPLIT_LOCK_DETECT; > > + else > > + test_ctl_val |= TEST_CTL_ENABLE_SPLIT_LOCK_DETECT; > > + > > + return test_ctl_val; > > +} > > Aside of that do we really need a misnomed function which replaces the > simple inline code at the call site: > > rdmsr(l, h) > l &= ~TEST_CTL_ENABLE_SPLIT_LOCK_DETECT; > l |= ac_split_lock_enable << TEST_CTL_ENABLE_SPLIT_LOCK_DETECT_SHIFT; > wrmrs(...) > > or the even more simple > > if (ac_split_lock_enable) > msr_set_bit(...) > else > msr_clear_nit(...) > > Hmm? The function new_sp_test_ctrl_val() will be called twice: here when initializing split lock detection and in split_lock_detect_store() when enabling/disabling the feature through the sysfs interface in patch 0014. So can I still keep this function and name it as get_new_test_ctrl_val()? > > > + > > +static inline void show_split_lock_detection_info(void) > > +{ > > + if (READ_ONCE(split_lock_detect_val)) > > That READ_ONCE() is required because? Ok. Will remove READ_ONCE(). > > > + pr_info_once("x86/split_lock: split lock detection enabled\n"); > > + else > > + pr_info_once("x86/split_lock: split lock detection disabled\n"); > > pr_fmt exists for a reason and having 'split lock' repeated several times > in the same line is not making it more readable. Ok. I will change the string to "x86/split_lock_detection: enabled\n", is it ok? > > > > + > > /* Unmask CPUID levels if masked: */ > > if (c->x86 > 6 || (c->x86 == 6 && c->x86_model >= 0xd)) { > > if (msr_clear_bit(MSR_IA32_MISC_ENABLE, > > @@ -1032,6 +1073,7 @@ cpu_dev_register(intel_cpu_dev); > > static void __init set_split_lock_detect(void) > > { > > setup_force_cpu_cap(X86_FEATURE_SPLIT_LOCK_DETECT); > > + split_lock_detect_val = 1; > > Oh well. You add defines on top of the file and then you don't use them. Will fix this. Thanks. -Fenghua
On Thu, 4 Apr 2019, Fenghua Yu wrote: > On Thu, Apr 04, 2019 at 08:07:57PM +0200, Thomas Gleixner wrote: > > On Wed, 3 Apr 2019, Fenghua Yu wrote: > > > +static DEFINE_MUTEX(split_lock_detect_mutex); > > > +static int split_lock_detect_val; > > > > detect_val? What value is that? > > According to previous discussions, I was told to call this split lock feature > as "split lock detection" instead of "#AC for split lock". So I use > "split_lock_detect..." in variable names or function names, call feature flag > as "split_lock_detect", and call the feature as "split lock detection" in > descriptions. > > If you don't agree to name feature as "split lock detection", I can change > variable names/function names/feature flag/descriptions etc back to previous > names "ac_split_lock...", "#AC for split lock", etc. > > The variable split_lock_detect_val is either 0 or 1. It stores current > enable/disable status of split lock detection feature. By default it's > one after the feature is enumerated. Then sysadmin can change it to 0 or 1 > to enable or disable the feature during run time. > > static unsigned int ac_split_lock_enable; > > If you agree to name the split lock feature as "split lock detection" feature, > can I change this variable to static unsigned int split_lock_detect_enable? I don't care much whether it's ac_split_lock or split_lock_detect, but _val is a completely bogus and unintuitive name. The variable tells whether the functionality is enabled or not. Then do not name it $prefix_val, which can mean anything. Name it $prefix_enable, which makes it entirely clear what this is about. And please make it type bool so you don't need any of these defines either. > > > +static u32 new_sp_test_ctl_val(u32 test_ctl_val) > > > +{ > > > + /* Change the split lock setting. */ > > > + if (READ_ONCE(split_lock_detect_val) == DISABLE_SPLIT_LOCK_DETECT) > > > > That READ_ONCE() is required because? > > Ok. Will remove READ_ONCE(). > > > > > > + test_ctl_val &= ~TEST_CTL_ENABLE_SPLIT_LOCK_DETECT; > > > + else > > > + test_ctl_val |= TEST_CTL_ENABLE_SPLIT_LOCK_DETECT; > > > + > > > + return test_ctl_val; > > > +} > > > > Aside of that do we really need a misnomed function which replaces the > > simple inline code at the call site: > > > > rdmsr(l, h) > > l &= ~TEST_CTL_ENABLE_SPLIT_LOCK_DETECT; > > l |= ac_split_lock_enable << TEST_CTL_ENABLE_SPLIT_LOCK_DETECT_SHIFT; > > wrmrs(...) > > > > or the even more simple > > > > if (ac_split_lock_enable) > > msr_set_bit(...) > > else > > msr_clear_nit(...) > > > > Hmm? > > The function new_sp_test_ctrl_val() will be called twice: here when > initializing split lock detection and in split_lock_detect_store() > when enabling/disabling the feature through the sysfs interface in > patch 0014. It's still pointless. > So can I still keep this function and name it as get_new_test_ctrl_val()? No. The function you want to share between init code and sysfs is split_lock_update_msr() { if (split_lock_enable) msr_set_bit(...) else msr_clear_nit(...) } That's all. No duplicated code. No convoluted helper function, nothing. Simple straight forward readable code. > > > +static inline void show_split_lock_detection_info(void) > > > +{ > > > + if (READ_ONCE(split_lock_detect_val)) > > > > That READ_ONCE() is required because? > > Ok. Will remove READ_ONCE(). > > > > > > + pr_info_once("x86/split_lock: split lock detection enabled\n"); > > > + else > > > + pr_info_once("x86/split_lock: split lock detection disabled\n"); > > > > pr_fmt exists for a reason and having 'split lock' repeated several times > > in the same line is not making it more readable. > > Ok. I will change the string to "x86/split_lock_detection: enabled\n", > is it ok? Care to read carefully what I wrote? Hint: pr_fmt > > Oh well. You add defines on top of the file and then you don't use them. > > Will fix this. What about the init / feature detection sequence which you snipped from the reply? Thanks, tglx
On Thu, Apr 04, 2019 at 08:07:57PM +0200, Thomas Gleixner wrote: > On Wed, 3 Apr 2019, Fenghua Yu wrote: > > > +static void init_split_lock_detect(struct cpuinfo_x86 *c) > > +{ > > + if (cpu_has(c, X86_FEATURE_SPLIT_LOCK_DETECT)) { > > + u32 l, h; > > + > > + mutex_lock(&split_lock_detect_mutex); > > + rdmsr(MSR_TEST_CTL, l, h); > > + l = new_sp_test_ctl_val(l); > > + wrmsr(MSR_TEST_CTL, l, h); > > + show_split_lock_detection_info(); > > + mutex_unlock(&split_lock_detect_mutex); > > + } > > +} > > + > > static void early_init_intel(struct cpuinfo_x86 *c) > > { > > u64 misc_enable; > > > > + init_split_lock_detect(c); > > so we have in early boot: > > early_cpu_init() > early_identify_cpu() > this_cpu->c_early_init(c) > early_init_intel() { > init_split_lock_detect(); > } > .... > cpu_set_core_cap_bits(c) > set(FEATURE_SPLIT_LOCK) > > I don't have to understand how init_split_lock_detect() will magically see > the feature bit which gets set afterwards, right? early_init_intel() is called twice on the boot CPU. Besides it's called in earl_cpu_init(), it's also called in: identify_boot_cpu() identify_cpu() init_intel() early_init_intel() init_split_lock_detect(); It's true that init_split_lock_detect() doesn't see the feature bit when it's called for the first time in early_cpu_init(). But it sees the feature bit when it's called for the second time in identify_boot_cpu(). So is init_split_lock_detect() in the right place? Thanks. -Fenghua
On Tue, 9 Apr 2019, Fenghua Yu wrote: > On Thu, Apr 04, 2019 at 08:07:57PM +0200, Thomas Gleixner wrote: > > On Wed, 3 Apr 2019, Fenghua Yu wrote: > > > static void early_init_intel(struct cpuinfo_x86 *c) > > > { > > > u64 misc_enable; > > > > > > + init_split_lock_detect(c); > > > > so we have in early boot: > > > > early_cpu_init() > > early_identify_cpu() > > this_cpu->c_early_init(c) > > early_init_intel() { > > init_split_lock_detect(); > > } > > .... > > cpu_set_core_cap_bits(c) > > set(FEATURE_SPLIT_LOCK) > > > > I don't have to understand how init_split_lock_detect() will magically see > > the feature bit which gets set afterwards, right? > > early_init_intel() is called twice on the boot CPU. Besides it's called > in earl_cpu_init(), it's also called in: > identify_boot_cpu() > identify_cpu() > init_intel() > early_init_intel() > init_split_lock_detect(); > > It's true that init_split_lock_detect() doesn't see the feature bit when > it's called for the first time in early_cpu_init(). But it sees the feature > bit when it's called for the second time in identify_boot_cpu(). That's hideous, really. > So is init_split_lock_detect() in the right place? You're not seriously asking that? It's obviously not the right place. We are not placing calls at random points just because they happen to work by chance. Thanks, tglx
FWIW it took me a while to work out what a 'split lock' was. I suspect because I was thinking of kernel locks, not the instruction lock prefix. It also isn't really obvious that 'split' refers to crossing cache lines. Referring to it as a 'misaligned lock' might be more easily understood. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Wed, Apr 10, 2019 at 08:31:31AM +0200, Thomas Gleixner wrote: > On Tue, 9 Apr 2019, Fenghua Yu wrote: > > On Thu, Apr 04, 2019 at 08:07:57PM +0200, Thomas Gleixner wrote: > > > On Wed, 3 Apr 2019, Fenghua Yu wrote: > > > > static void early_init_intel(struct cpuinfo_x86 *c) > > > > { > > > > u64 misc_enable; > > > > > > > > + init_split_lock_detect(c); > > > > > > so we have in early boot: > > > > > > early_cpu_init() > > > early_identify_cpu() > > > this_cpu->c_early_init(c) > > > early_init_intel() { > > > init_split_lock_detect(); > > > } > > > .... > > > cpu_set_core_cap_bits(c) > > > set(FEATURE_SPLIT_LOCK) > > > > > > I don't have to understand how init_split_lock_detect() will magically see > > > the feature bit which gets set afterwards, right? > > > > early_init_intel() is called twice on the boot CPU. Besides it's called > > in earl_cpu_init(), it's also called in: > > identify_boot_cpu() > > identify_cpu() > > init_intel() > > early_init_intel() > > init_split_lock_detect(); > > > > It's true that init_split_lock_detect() doesn't see the feature bit when > > it's called for the first time in early_cpu_init(). But it sees the feature > > bit when it's called for the second time in identify_boot_cpu(). > > That's hideous, really. > > > So is init_split_lock_detect() in the right place? > > You're not seriously asking that? > > It's obviously not the right place. We are not placing calls at random > points just because they happen to work by chance. Is it OK to put init_split_lock_detect(c) after early_init_intel() in init_intel()? X86_FEATURE_SPLIT_LOCK_DETECT is available now and init_split_lock_detec() is called only once on each CPU. @@ -746,6 +749,8 @@ static void init_intel(struct cpuinfo_x86 *c) { early_init_intel(c); + init_split_lock_detect(c); + intel_workarounds(c); Thanks. -Fenghua
diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c index 7f6943af35dc..ae3e327d5e35 100644 --- a/arch/x86/kernel/cpu/intel.c +++ b/arch/x86/kernel/cpu/intel.c @@ -31,6 +31,12 @@ #include <asm/apic.h> #endif +#define DISABLE_SPLIT_LOCK_DETECT 0 +#define ENABLE_SPLIT_LOCK_DETECT 1 + +static DEFINE_MUTEX(split_lock_detect_mutex); +static int split_lock_detect_val; + /* * Just in case our CPU detection goes bad, or you have a weird system, * allow a way to override the automatic disabling of MPX. @@ -161,10 +167,45 @@ static bool bad_spectre_microcode(struct cpuinfo_x86 *c) return false; } +static u32 new_sp_test_ctl_val(u32 test_ctl_val) +{ + /* Change the split lock setting. */ + if (READ_ONCE(split_lock_detect_val) == DISABLE_SPLIT_LOCK_DETECT) + test_ctl_val &= ~TEST_CTL_ENABLE_SPLIT_LOCK_DETECT; + else + test_ctl_val |= TEST_CTL_ENABLE_SPLIT_LOCK_DETECT; + + return test_ctl_val; +} + +static inline void show_split_lock_detection_info(void) +{ + if (READ_ONCE(split_lock_detect_val)) + pr_info_once("x86/split_lock: split lock detection enabled\n"); + else + pr_info_once("x86/split_lock: split lock detection disabled\n"); +} + +static void init_split_lock_detect(struct cpuinfo_x86 *c) +{ + if (cpu_has(c, X86_FEATURE_SPLIT_LOCK_DETECT)) { + u32 l, h; + + mutex_lock(&split_lock_detect_mutex); + rdmsr(MSR_TEST_CTL, l, h); + l = new_sp_test_ctl_val(l); + wrmsr(MSR_TEST_CTL, l, h); + show_split_lock_detection_info(); + mutex_unlock(&split_lock_detect_mutex); + } +} + static void early_init_intel(struct cpuinfo_x86 *c) { u64 misc_enable; + init_split_lock_detect(c); + /* Unmask CPUID levels if masked: */ if (c->x86 > 6 || (c->x86 == 6 && c->x86_model >= 0xd)) { if (msr_clear_bit(MSR_IA32_MISC_ENABLE, @@ -1032,6 +1073,7 @@ cpu_dev_register(intel_cpu_dev); static void __init set_split_lock_detect(void) { setup_force_cpu_cap(X86_FEATURE_SPLIT_LOCK_DETECT); + split_lock_detect_val = 1; } void __init cpu_set_core_cap_bits(struct cpuinfo_x86 *c)
A split locked access locks bus and degrades overall memory access performance. When split lock detection feature is enumerated, enable the feature by default to find any split lock issue and then fix the issue. Signed-off-by: Fenghua Yu <fenghua.yu@intel.com> --- arch/x86/kernel/cpu/intel.c | 42 +++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+)