* [PATCH v4 1/2] x86: fix bitops.h warning with a moved cast @ 2020-02-22 0:02 Jesse Brandeburg 2020-02-22 0:02 ` [PATCH v4 2/2] lib: make a test module with set/clear bit Jesse Brandeburg 2020-02-22 9:39 ` [PATCH v4 1/2] x86: fix bitops.h warning with a moved cast Andy Shevchenko 0 siblings, 2 replies; 5+ messages in thread From: Jesse Brandeburg @ 2020-02-22 0:02 UTC (permalink / raw) To: tglx, mingo, bp Cc: Jesse Brandeburg, x86, linux-kernel, linux, andriy.shevchenko, dan.j.williams, peterz Fix many sparse warnings when building with C=1. When the kernel is compiled with C=1, there are lots of messages like: arch/x86/include/asm/bitops.h:77:37: warning: cast truncates bits from constant value (ffffff7f becomes 7f) CONST_MASK() is using a signed integer "1" to create the mask which is later cast to (u8) when used. Move the cast to the definition and clean up the calling sites to prevent sparse from warning. The reason the warning was occurring is because certain bitmasks that end with a mask next to a natural boundary like 7, 15, 23, 31, end up with a mask like 0x7f, which then results in sign extension when doing an invert (but I'm not a compiler expert). It was really only "clear_bit" that was having problems, and it was only on bit checks next to a byte boundary (top bit). Verified with a test module (see next patch) and assembly inspection that the patch doesn't introduce any change in generated code. Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com> Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com> --- v4: reverse argument order as suggested by David Laight, added reviewed-by v3: Clean up the header file changes as per peterz. v2: use correct CC: list --- arch/x86/include/asm/bitops.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/arch/x86/include/asm/bitops.h b/arch/x86/include/asm/bitops.h index 062cdecb2f24..fed152434ed0 100644 --- a/arch/x86/include/asm/bitops.h +++ b/arch/x86/include/asm/bitops.h @@ -46,7 +46,7 @@ * a mask operation on a byte. */ #define CONST_MASK_ADDR(nr, addr) WBYTE_ADDR((void *)(addr) + ((nr)>>3)) -#define CONST_MASK(nr) (1 << ((nr) & 7)) +#define CONST_MASK(nr) ((u8)1 << ((nr) & 7)) static __always_inline void arch_set_bit(long nr, volatile unsigned long *addr) @@ -54,7 +54,7 @@ arch_set_bit(long nr, volatile unsigned long *addr) if (__builtin_constant_p(nr)) { asm volatile(LOCK_PREFIX "orb %1,%0" : CONST_MASK_ADDR(nr, addr) - : "iq" ((u8)CONST_MASK(nr)) + : "iq" (CONST_MASK(nr)) : "memory"); } else { asm volatile(LOCK_PREFIX __ASM_SIZE(bts) " %1,%0" @@ -74,7 +74,7 @@ arch_clear_bit(long nr, volatile unsigned long *addr) if (__builtin_constant_p(nr)) { asm volatile(LOCK_PREFIX "andb %1,%0" : CONST_MASK_ADDR(nr, addr) - : "iq" ((u8)~CONST_MASK(nr))); + : "iq" (CONST_MASK(nr) ^ 0xff)); } else { asm volatile(LOCK_PREFIX __ASM_SIZE(btr) " %1,%0" : : RLONG_ADDR(addr), "Ir" (nr) : "memory"); base-commit: ca7e1fd1026c5af6a533b4b5447e1d2f153e28f2 -- 2.24.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v4 2/2] lib: make a test module with set/clear bit 2020-02-22 0:02 [PATCH v4 1/2] x86: fix bitops.h warning with a moved cast Jesse Brandeburg @ 2020-02-22 0:02 ` Jesse Brandeburg 2020-02-22 9:39 ` [PATCH v4 1/2] x86: fix bitops.h warning with a moved cast Andy Shevchenko 1 sibling, 0 replies; 5+ messages in thread From: Jesse Brandeburg @ 2020-02-22 0:02 UTC (permalink / raw) To: tglx, mingo, bp Cc: Jesse Brandeburg, x86, linux-kernel, linux, andriy.shevchenko, dan.j.williams, peterz Test some bit clears/sets to make sure assembly doesn't change, and that the set_bit and clear_bit functions work and don't cause sparse warnings. Instruct Kbuild to build this file with extra warning level -Wextra, to catch new issues, and also doesn't hurt to build with C=1. This was used to test changes to arch/x86/include/asm/bitops.h. In particular, sparse (C=1) was very concerned when the last bit before a natural boundary, like 7, or 31, was being tested, as this causes sign extension (0xffffff7f) for instance when clearing bit 7. Recommended usage: make defconfig scripts/config -m CONFIG_TEST_BITOPS make modules_prepare make C=1 W=1 lib/test_bitops.ko objdump -S -d lib/test_bitops.ko insmod lib/test_bitops.ko rmmod lib/test_bitops.ko <check dmesg>, there should be no compiler/sparse warnings and no error messages in log. Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com> Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com> --- v4: Slight change to bitops_fun last member, as suggested by Andy, added his reviewed-by too. Added module load/unload to usage. Added copyright. v3: Update the test to fail if bits aren't cleared, and make the test reproduce the original issue without patch 1/2, showing that the issue is fixed in patch 1/2. Thanks PeterZ! v2: Correct CC: list --- lib/Kconfig.debug | 13 +++++++ lib/Makefile | 2 + lib/test_bitops.c | 60 ++++++++++++++++++++++++++++++ tools/testing/selftests/lib/config | 1 + 4 files changed, 76 insertions(+) create mode 100644 lib/test_bitops.c diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index 69def4a9df00..61a5d00ea064 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -1947,6 +1947,19 @@ config TEST_LKM If unsure, say N. +config TEST_BITOPS + tristate "Test module for compilation of clear_bit/set_bit operations" + depends on m + help + This builds the "test_bitops" module that is much like the + TEST_LKM module except that it does a basic exercise of the + clear_bit and set_bit macros to make sure there are no compiler + warnings from C=1 sparse checker or -Wextra compilations. It has + no dependencies and doesn't run or load unless explicitly requested + by name. for example: modprobe test_bitops. + + If unsure, say N. + config TEST_VMALLOC tristate "Test module for stress/performance analysis of vmalloc allocator" default n diff --git a/lib/Makefile b/lib/Makefile index 611872c06926..b18db565b355 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -89,6 +89,8 @@ obj-$(CONFIG_TEST_OBJAGG) += test_objagg.o obj-$(CONFIG_TEST_STACKINIT) += test_stackinit.o obj-$(CONFIG_TEST_BLACKHOLE_DEV) += test_blackhole_dev.o obj-$(CONFIG_TEST_MEMINIT) += test_meminit.o +obj-$(CONFIG_TEST_BITOPS) += test_bitops.o +CFLAGS_test_bitops.o += -Werror obj-$(CONFIG_TEST_LIVEPATCH) += livepatch/ diff --git a/lib/test_bitops.c b/lib/test_bitops.c new file mode 100644 index 000000000000..fd50b3ae4a14 --- /dev/null +++ b/lib/test_bitops.c @@ -0,0 +1,60 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (C) 2020 Intel Corporation + */ + +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt + +#include <linux/init.h> +#include <linux/module.h> +#include <linux/printk.h> + +/* a tiny module only meant to test set/clear_bit */ + +/* use an enum because thats the most common BITMAP usage */ +enum bitops_fun { + BITOPS_4 = 4, + BITOPS_7 = 7, + BITOPS_11 = 11, + BITOPS_31 = 31, + BITOPS_88 = 88, + BITOPS_LAST = 255, + BITOPS_LENGTH = 256 +}; + +static DECLARE_BITMAP(g_bitmap, BITOPS_LENGTH); + +static int __init test_bitops_startup(void) +{ + pr_warn("Loaded test module\n"); + set_bit(BITOPS_4, g_bitmap); + set_bit(BITOPS_7, g_bitmap); + set_bit(BITOPS_11, g_bitmap); + set_bit(BITOPS_31, g_bitmap); + set_bit(BITOPS_88, g_bitmap); + return 0; +} + +static void __exit test_bitops_unstartup(void) +{ + int bit_set; + + clear_bit(BITOPS_4, g_bitmap); + clear_bit(BITOPS_7, g_bitmap); + clear_bit(BITOPS_11, g_bitmap); + clear_bit(BITOPS_31, g_bitmap); + clear_bit(BITOPS_88, g_bitmap); + + bit_set = find_first_bit(g_bitmap, BITOPS_LAST); + if (bit_set != BITOPS_LAST) + pr_err("ERROR: FOUND SET BIT %d\n", bit_set); + + pr_warn("Unloaded test module\n"); +} + +module_init(test_bitops_startup); +module_exit(test_bitops_unstartup); + +MODULE_AUTHOR("Jesse Brandeburg <jesse.brandeburg@intel.com>"); +MODULE_LICENSE("GPL"); +MODULE_DESCRIPTION("Bit testing module"); diff --git a/tools/testing/selftests/lib/config b/tools/testing/selftests/lib/config index 14a77ea4a8da..b80ee3f6e265 100644 --- a/tools/testing/selftests/lib/config +++ b/tools/testing/selftests/lib/config @@ -2,3 +2,4 @@ CONFIG_TEST_PRINTF=m CONFIG_TEST_BITMAP=m CONFIG_PRIME_NUMBERS=m CONFIG_TEST_STRSCPY=m +CONFIG_TEST_BITOPS=m -- 2.24.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v4 1/2] x86: fix bitops.h warning with a moved cast 2020-02-22 0:02 [PATCH v4 1/2] x86: fix bitops.h warning with a moved cast Jesse Brandeburg 2020-02-22 0:02 ` [PATCH v4 2/2] lib: make a test module with set/clear bit Jesse Brandeburg @ 2020-02-22 9:39 ` Andy Shevchenko 2020-02-24 9:54 ` Peter Zijlstra 1 sibling, 1 reply; 5+ messages in thread From: Andy Shevchenko @ 2020-02-22 9:39 UTC (permalink / raw) To: Jesse Brandeburg Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT), Linux Kernel Mailing List, Rasmus Villemoes, Andriy Shevchenko, Dan Williams, Peter Zijlstra (Intel) On Sat, Feb 22, 2020 at 2:04 AM Jesse Brandeburg <jesse.brandeburg@intel.com> wrote: > > Fix many sparse warnings when building with C=1. > > When the kernel is compiled with C=1, there are lots of messages like: > arch/x86/include/asm/bitops.h:77:37: warning: cast truncates bits from constant value (ffffff7f becomes 7f) > > CONST_MASK() is using a signed integer "1" to create the mask which > is later cast to (u8) when used. Move the cast to the definition and > clean up the calling sites to prevent sparse from warning. > > The reason the warning was occurring is because certain bitmasks that > end with a mask next to a natural boundary like 7, 15, 23, 31, end up > with a mask like 0x7f, which then results in sign extension when doing > an invert (but I'm not a compiler expert). It was really only > "clear_bit" that was having problems, and it was only on bit checks next > to a byte boundary (top bit). > > Verified with a test module (see next patch) and assembly inspection > that the patch doesn't introduce any change in generated code. > > Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com> > --- > v4: reverse argument order as suggested by David Laight, added reviewed-by > v3: Clean up the header file changes as per peterz. > v2: use correct CC: list > --- > arch/x86/include/asm/bitops.h | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/arch/x86/include/asm/bitops.h b/arch/x86/include/asm/bitops.h > index 062cdecb2f24..fed152434ed0 100644 > --- a/arch/x86/include/asm/bitops.h > +++ b/arch/x86/include/asm/bitops.h > @@ -46,7 +46,7 @@ > * a mask operation on a byte. > */ > #define CONST_MASK_ADDR(nr, addr) WBYTE_ADDR((void *)(addr) + ((nr)>>3)) > -#define CONST_MASK(nr) (1 << ((nr) & 7)) > +#define CONST_MASK(nr) ((u8)1 << ((nr) & 7)) > > static __always_inline void > arch_set_bit(long nr, volatile unsigned long *addr) > @@ -54,7 +54,7 @@ arch_set_bit(long nr, volatile unsigned long *addr) > if (__builtin_constant_p(nr)) { > asm volatile(LOCK_PREFIX "orb %1,%0" > : CONST_MASK_ADDR(nr, addr) > - : "iq" ((u8)CONST_MASK(nr)) > + : "iq" (CONST_MASK(nr)) > : "memory"); > } else { > asm volatile(LOCK_PREFIX __ASM_SIZE(bts) " %1,%0" > @@ -74,7 +74,7 @@ arch_clear_bit(long nr, volatile unsigned long *addr) > if (__builtin_constant_p(nr)) { > asm volatile(LOCK_PREFIX "andb %1,%0" > : CONST_MASK_ADDR(nr, addr) > - : "iq" ((u8)~CONST_MASK(nr))); > + : "iq" (CONST_MASK(nr) ^ 0xff)); I'm wondering if the original, by Peter Z, order allows us to drop (u8) casting in the CONST_MASK completely. > } else { > asm volatile(LOCK_PREFIX __ASM_SIZE(btr) " %1,%0" > : : RLONG_ADDR(addr), "Ir" (nr) : "memory"); > > base-commit: ca7e1fd1026c5af6a533b4b5447e1d2f153e28f2 > -- > 2.24.1 > -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v4 1/2] x86: fix bitops.h warning with a moved cast 2020-02-22 9:39 ` [PATCH v4 1/2] x86: fix bitops.h warning with a moved cast Andy Shevchenko @ 2020-02-24 9:54 ` Peter Zijlstra 2020-02-24 17:00 ` Jesse Brandeburg 0 siblings, 1 reply; 5+ messages in thread From: Peter Zijlstra @ 2020-02-24 9:54 UTC (permalink / raw) To: Andy Shevchenko Cc: Jesse Brandeburg, Thomas Gleixner, Ingo Molnar, Borislav Petkov, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT), Linux Kernel Mailing List, Rasmus Villemoes, Andriy Shevchenko, Dan Williams On Sat, Feb 22, 2020 at 11:39:57AM +0200, Andy Shevchenko wrote: > On Sat, Feb 22, 2020 at 2:04 AM Jesse Brandeburg > > -#define CONST_MASK(nr) (1 << ((nr) & 7)) > > +#define CONST_MASK(nr) ((u8)1 << ((nr) & 7)) > > > > static __always_inline void > > arch_set_bit(long nr, volatile unsigned long *addr) > > @@ -54,7 +54,7 @@ arch_set_bit(long nr, volatile unsigned long *addr) > > if (__builtin_constant_p(nr)) { > > asm volatile(LOCK_PREFIX "orb %1,%0" > > : CONST_MASK_ADDR(nr, addr) > > - : "iq" ((u8)CONST_MASK(nr)) > > + : "iq" (CONST_MASK(nr)) Note how this is not equivalent, the old code actually handed in a u8 while the new code hands int. By moving the (u8) cast into the parens, you casl 1 to u8, which then instantly gets promoted to 'int' due to the '<<' operator. > > : "memory"); > > } else { > > asm volatile(LOCK_PREFIX __ASM_SIZE(bts) " %1,%0" > > @@ -74,7 +74,7 @@ arch_clear_bit(long nr, volatile unsigned long *addr) > > if (__builtin_constant_p(nr)) { > > asm volatile(LOCK_PREFIX "andb %1,%0" > > : CONST_MASK_ADDR(nr, addr) > > - : "iq" ((u8)~CONST_MASK(nr))); > > + : "iq" (CONST_MASK(nr) ^ 0xff)); > > I'm wondering if the original, by Peter Z, order allows us to drop > (u8) casting in the CONST_MASK completely. I'm thinking it's all nonsense anyway :-), the result of either << or ^ is always promoted to int anyway. The sparse complaint was that ~CONST_MASK(nr) had high bits set which were lost, which is true, but a copmletely stupid warning IMO. By using 0xff ^ CONST_MASK(nr), those bits will not be set and will not be lost. None of that has anything to do with where we place a pointless cast more or less. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v4 1/2] x86: fix bitops.h warning with a moved cast 2020-02-24 9:54 ` Peter Zijlstra @ 2020-02-24 17:00 ` Jesse Brandeburg 0 siblings, 0 replies; 5+ messages in thread From: Jesse Brandeburg @ 2020-02-24 17:00 UTC (permalink / raw) To: Peter Zijlstra Cc: Andy Shevchenko, Thomas Gleixner, Ingo Molnar, Borislav Petkov, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT), Linux Kernel Mailing List, Rasmus Villemoes, Andriy Shevchenko, Dan Williams On Mon, 24 Feb 2020 10:54:02 +0100 Peter wrote: > On Sat, Feb 22, 2020 at 11:39:57AM +0200, Andy Shevchenko wrote: > > On Sat, Feb 22, 2020 at 2:04 AM Jesse Brandeburg > > > > -#define CONST_MASK(nr) (1 << ((nr) & 7)) > > > +#define CONST_MASK(nr) ((u8)1 << ((nr) & 7)) > > > > > > static __always_inline void > > > arch_set_bit(long nr, volatile unsigned long *addr) > > > @@ -54,7 +54,7 @@ arch_set_bit(long nr, volatile unsigned long *addr) > > > if (__builtin_constant_p(nr)) { > > > asm volatile(LOCK_PREFIX "orb %1,%0" > > > : CONST_MASK_ADDR(nr, addr) > > > - : "iq" ((u8)CONST_MASK(nr)) > > > + : "iq" (CONST_MASK(nr)) > > Note how this is not equivalent, the old code actually handed in a u8 > while the new code hands int. By moving the (u8) cast into the parens, > you casl 1 to u8, which then instantly gets promoted to 'int' due to the > '<<' operator. True. Which is why I had decided to use the strongly typed local variables, which as I recall you mentioned were "sad", so I had tried to fix it with the simpler changes. Everything is a negotiation with the compiler and tools here, about how clearly you can communicate the code's intent and functionality while still keeping it performant. > > > : "memory"); > > > } else { > > > asm volatile(LOCK_PREFIX __ASM_SIZE(bts) " %1,%0" > > > @@ -74,7 +74,7 @@ arch_clear_bit(long nr, volatile unsigned long *addr) > > > if (__builtin_constant_p(nr)) { > > > asm volatile(LOCK_PREFIX "andb %1,%0" > > > : CONST_MASK_ADDR(nr, addr) > > > - : "iq" ((u8)~CONST_MASK(nr))); > > > + : "iq" (CONST_MASK(nr) ^ 0xff)); > > > > I'm wondering if the original, by Peter Z, order allows us to drop > > (u8) casting in the CONST_MASK completely. > > I'm thinking it's all nonsense anyway :-), the result of either << or ^ > is always promoted to int anyway. Yeah, I realize this is *all* nonsense, but I *do* see value in making the code not generate sparse warnings, as long as the end result doesn't generate code change. It allows you to run more tools to find bugs with less false positives. > The sparse complaint was that ~CONST_MASK(nr) had high bits set which > were lost, which is true, but a copmletely stupid warning IMO. > > By using 0xff ^ CONST_MASK(nr), those bits will not be set and will not > be lost. > > None of that has anything to do with where we place a pointless cast > more or less. Well now that we have the test module, I'll check that the simplest possible patch of just changing the one line for ~CONST_MASK(nr) to 0xFF ^ CONST_MASK(nr) will fix the issue. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-02-24 17:00 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-02-22 0:02 [PATCH v4 1/2] x86: fix bitops.h warning with a moved cast Jesse Brandeburg 2020-02-22 0:02 ` [PATCH v4 2/2] lib: make a test module with set/clear bit Jesse Brandeburg 2020-02-22 9:39 ` [PATCH v4 1/2] x86: fix bitops.h warning with a moved cast Andy Shevchenko 2020-02-24 9:54 ` Peter Zijlstra 2020-02-24 17:00 ` Jesse Brandeburg
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).