* [PATCH v3 2/2] lib: make a test module with set/clear bit
2020-02-20 23:21 [PATCH v3 1/2] x86: fix bitops.h warning with a moved cast Jesse Brandeburg
@ 2020-02-20 23:21 ` Jesse Brandeburg
2020-02-21 10:25 ` Andy Shevchenko
2020-02-21 10:21 ` [PATCH v3 1/2] x86: fix bitops.h warning with a moved cast Andy Shevchenko
2020-02-21 15:33 ` David Laight
2 siblings, 1 reply; 6+ messages in thread
From: Jesse Brandeburg @ 2020-02-20 23:21 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
Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
---
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 | 55 ++++++++++++++++++++++++++++++
tools/testing/selftests/lib/config | 1 +
4 files changed, 71 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..97339d6455b0
--- /dev/null
+++ b/lib/test_bitops.c
@@ -0,0 +1,55 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#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 compile-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
+};
+
+static DECLARE_BITMAP(g_bitmap, BITOPS_LAST);
+
+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] 6+ messages in thread* Re: [PATCH v3 2/2] lib: make a test module with set/clear bit
2020-02-20 23:21 ` [PATCH v3 2/2] lib: make a test module with set/clear bit Jesse Brandeburg
@ 2020-02-21 10:25 ` Andy Shevchenko
2020-02-21 23:16 ` Jesse Brandeburg
0 siblings, 1 reply; 6+ messages in thread
From: Andy Shevchenko @ 2020-02-21 10:25 UTC (permalink / raw)
To: Jesse Brandeburg
Cc: tglx, mingo, bp, x86, linux-kernel, linux, dan.j.williams, peterz
On Thu, Feb 20, 2020 at 03:21:55PM -0800, Jesse Brandeburg wrote:
> 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
Thanks!
One comments below, after addressing:
Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
> Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
>
> ---
> 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 | 55 ++++++++++++++++++++++++++++++
> tools/testing/selftests/lib/config | 1 +
> 4 files changed, 71 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..97339d6455b0
> --- /dev/null
> +++ b/lib/test_bitops.c
> @@ -0,0 +1,55 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +#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 compile-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_LAST = 255,
BITOPS_LENGTH = 256
and...
> +};
> +
> +static DECLARE_BITMAP(g_bitmap, BITOPS_LAST);
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
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v3 2/2] lib: make a test module with set/clear bit
2020-02-21 10:25 ` Andy Shevchenko
@ 2020-02-21 23:16 ` Jesse Brandeburg
0 siblings, 0 replies; 6+ messages in thread
From: Jesse Brandeburg @ 2020-02-21 23:16 UTC (permalink / raw)
To: Andy Shevchenko
Cc: tglx, mingo, bp, x86, linux-kernel, linux, dan.j.williams, peterz
On Fri, 21 Feb 2020 12:25:18 +0200 Andy wrote:
> On Thu, Feb 20, 2020 at 03:21:55PM -0800, Jesse Brandeburg wrote:
> > 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
>
> Thanks!
> One comments below, after addressing:
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Thanks!
> BITOPS_LAST = 255,
> BITOPS_LENGTH = 256
>
> and...
>
> static DECLARE_BITMAP(g_bitmap, BITOPS_LENGTH);
Fixed in v4
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 1/2] x86: fix bitops.h warning with a moved cast
2020-02-20 23:21 [PATCH v3 1/2] x86: fix bitops.h warning with a moved cast Jesse Brandeburg
2020-02-20 23:21 ` [PATCH v3 2/2] lib: make a test module with set/clear bit Jesse Brandeburg
@ 2020-02-21 10:21 ` Andy Shevchenko
2020-02-21 15:33 ` David Laight
2 siblings, 0 replies; 6+ messages in thread
From: Andy Shevchenko @ 2020-02-21 10:21 UTC (permalink / raw)
To: Jesse Brandeburg
Cc: tglx, mingo, bp, x86, linux-kernel, linux, dan.j.williams, peterz
On Thu, Feb 20, 2020 at 03:21:54PM -0800, Jesse Brandeburg 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.
FWIW,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Thanks for fixing this, I have experienced tons of such messages.
> Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
> ---
> 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..96ef19dcbde6 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" (0xff ^ CONST_MASK(nr)));
> } 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] 6+ messages in thread* RE: [PATCH v3 1/2] x86: fix bitops.h warning with a moved cast
2020-02-20 23:21 [PATCH v3 1/2] x86: fix bitops.h warning with a moved cast Jesse Brandeburg
2020-02-20 23:21 ` [PATCH v3 2/2] lib: make a test module with set/clear bit Jesse Brandeburg
2020-02-21 10:21 ` [PATCH v3 1/2] x86: fix bitops.h warning with a moved cast Andy Shevchenko
@ 2020-02-21 15:33 ` David Laight
2 siblings, 0 replies; 6+ messages in thread
From: David Laight @ 2020-02-21 15:33 UTC (permalink / raw)
To: 'Jesse Brandeburg', tglx@linutronix.de, mingo@redhat.com,
bp@alien8.de
Cc: x86@kernel.org, linux-kernel@vger.kernel.org,
linux@rasmusvillemoes.dk, andriy.shevchenko@intel.com,
dan.j.williams@intel.com, peterz@infradead.org
From: Jesse Brandeburg
> 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.
...
> diff --git a/arch/x86/include/asm/bitops.h b/arch/x86/include/asm/bitops.h
> index 062cdecb2f24..96ef19dcbde6 100644
> --- a/arch/x86/include/asm/bitops.h
> +++ b/arch/x86/include/asm/bitops.h
...
> @@ -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" (0xff ^ CONST_MASK(nr)));
IMHO (CONST_MASK(nr) ^ 0xff) is better.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 6+ messages in thread