| Message ID | 1554326526-172295-2-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, Apr 03, 2019 at 02:21:47PM -0700, Fenghua Yu wrote: > cpu_caps_cleared and cpu_caps_set may not be aligned to unsigned long. > Atomic operations (i.e. set_bit and clear_bit) on the bitmaps may access Please end function names with parentheses. > two cache lines (a.k.a. split lock) and lock the bus to block all memory not "lock the bus" but "cause the CPU to do a bus lock... " > accesses from other processors to ensure atomicity. > > To avoid the overall performance degradation from the bus locking, align > the two variables to unsigned long. > > Defining the variables as unsigned long may also fix the issue because > they are naturally aligned to unsigned long. But that needs additional > code changes. Adding __aligned(unsigned long) is a simpler fix. > > Signed-off-by: Fenghua Yu <fenghua.yu@intel.com> > --- > arch/x86/kernel/cpu/common.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) With the above nitpicks addressed: Reviewed-by: Borislav Petkov <bp@suse.de>
On Thu, Apr 04, 2019 at 04:39:43PM +0200, Borislav Petkov wrote: > On Wed, Apr 03, 2019 at 02:21:47PM -0700, Fenghua Yu wrote: > > cpu_caps_cleared and cpu_caps_set may not be aligned to unsigned long. > > Atomic operations (i.e. set_bit and clear_bit) on the bitmaps may access > > Please end function names with parentheses. Sure. > > > two cache lines (a.k.a. split lock) and lock the bus to block all memory > > not "lock the bus" but "cause the CPU to do a bus lock... " Sure. > > > accesses from other processors to ensure atomicity. > > > > To avoid the overall performance degradation from the bus locking, align > > the two variables to unsigned long. > > > > Defining the variables as unsigned long may also fix the issue because > > they are naturally aligned to unsigned long. But that needs additional > > code changes. Adding __aligned(unsigned long) is a simpler fix. > > > > Signed-off-by: Fenghua Yu <fenghua.yu@intel.com> > > --- > > arch/x86/kernel/cpu/common.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > With the above nitpicks addressed: > > Reviewed-by: Borislav Petkov <bp@suse.de> Thank you for your review! -Fenghua
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c index cb28e98a0659..3716e2bb028b 100644 --- a/arch/x86/kernel/cpu/common.c +++ b/arch/x86/kernel/cpu/common.c @@ -488,8 +488,9 @@ static const char *table_lookup_model(struct cpuinfo_x86 *c) return NULL; /* Not found */ } -__u32 cpu_caps_cleared[NCAPINTS + NBUGINTS]; -__u32 cpu_caps_set[NCAPINTS + NBUGINTS]; +/* Aligned to unsigned long to avoid split lock in atomic bitmap ops */ +__u32 cpu_caps_cleared[NCAPINTS + NBUGINTS] __aligned(sizeof(unsigned long)); +__u32 cpu_caps_set[NCAPINTS + NBUGINTS] __aligned(sizeof(unsigned long)); void load_percpu_segment(int cpu) {
cpu_caps_cleared and cpu_caps_set may not be aligned to unsigned long. Atomic operations (i.e. set_bit and clear_bit) on the bitmaps may access two cache lines (a.k.a. split lock) and lock the bus to block all memory accesses from other processors to ensure atomicity. To avoid the overall performance degradation from the bus locking, align the two variables to unsigned long. Defining the variables as unsigned long may also fix the issue because they are naturally aligned to unsigned long. But that needs additional code changes. Adding __aligned(unsigned long) is a simpler fix. Signed-off-by: Fenghua Yu <fenghua.yu@intel.com> --- arch/x86/kernel/cpu/common.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)