* [PATCH] lib: bitmap: Mute some odd section mismatch warning in xtensa kernel build
@ 2021-08-15 3:21 Barry Song
2021-08-15 10:30 ` Andy Shevchenko
2021-08-15 15:55 ` Yury Norov
0 siblings, 2 replies; 4+ messages in thread
From: Barry Song @ 2021-08-15 3:21 UTC (permalink / raw)
To: gregkh, yury.norov, andriy.shevchenko, linux, linux-kernel
Cc: linuxarm, Barry Song, kernel test robot, Max Filippov, Andrew Pinski
From: Barry Song <song.bao.hua@hisilicon.com>
Constanly there are some section mismatch issues reported in test_bitmap
for xtensa platform such as:
Section mismatch in reference from the function bitmap_equal() to the
variable .init.data:initcall_level_names
The function bitmap_equal() references the variable __initconst
__setup_str_initcall_blacklist. This is often because bitmap_equal
lacks a __initconst annotation or the annotation of
__setup_str_initcall_blacklist is wrong.
Section mismatch in reference from the function bitmap_copy_clear_tail()
to the variable .init.rodata:__setup_str_initcall_blacklist
The function bitmap_copy_clear_tail() references the variable __initconst
__setup_str_initcall_blacklist.
This is often because bitmap_copy_clear_tail lacks a __initconst
annotation or the annotation of __setup_str_initcall_blacklist is wrong.
To be honest, hardly to believe kernel code is wrong since bitmap_equal is
always called in __init function in test_bitmap.c just like __bitmap_equal.
But gcc doesn't report any issue for __bitmap_equal even when bitmap_equal
and __bitmap_equal show in the same function such as:
static void noinline __init test_mem_optimisations(void)
{
...
for (start = 0; start < 1024; start += 8) {
for (nbits = 0; nbits < 1024 - start; nbits += 8) {
if (!bitmap_equal(bmap1, bmap2, 1024)) {
failed_tests++;
}
if (!__bitmap_equal(bmap1, bmap2, 1024)) {
failed_tests++;
}
...
}
}
}
The different between __bitmap_equal() and bitmap_equal() is that the
former is extern and a EXPORT_SYMBOL. So noinline, and probably in fact
noclone. But the later is static and unfortunately not inlined at this
time though it has a "inline" flag.
bitmap_copy_clear_tail(), on the other hand, seems more innocent as it is
accessing stack only by its wrapper bitmap_from_arr32() in function
test_bitmap_arr32():
static void __init test_bitmap_arr32(void)
{
unsigned int nbits, next_bit;
u32 arr[EXP1_IN_BITS / 32];
DECLARE_BITMAP(bmap2, EXP1_IN_BITS);
memset(arr, 0xa5, sizeof(arr));
for (nbits = 0; nbits < EXP1_IN_BITS; ++nbits) {
bitmap_to_arr32(arr, exp1, nbits);
bitmap_from_arr32(bmap2, arr, nbits);
expect_eq_bitmap(bmap2, exp1, nbits);
...
}
}
Looks like gcc optimized arr, bmap2 things to .init.data but it seems
nothing is wrong in kernel since test_bitmap_arr32() is __init.
Max Filippov reported a bug to gcc but gcc people don't ack. So here
this patch removes the involved symbols by forcing inline. It might
not be that elegant but I don't see any harm as bitmap_equal() and
bitmap_copy_clear_tail() are both quite small. In addition, kernel
doc also backs this modification "We don't use the 'inline' keyword
because it's broken": www.kernel.org/doc/local/inline.html
Another possible way to "fix" the warning is moving the involved
symboms to lib/bitmap.c:
+int bitmap_equal(const unsigned long *src1,
+ const unsigned long *src2, unsigned int nbits)
+{
+ if (small_const_nbits(nbits))
+ return !((*src1 ^ *src2) & BITMAP_LAST_WORD_MASK(nbits));
+ if (__builtin_constant_p(nbits & BITMAP_MEM_MASK) &&
+ IS_ALIGNED(nbits, BITMAP_MEM_ALIGNMENT))
+ return !memcmp(src1, src2, nbits / 8);
+ return __bitmap_equal(src1, src2, nbits);
+}
+EXPORT_SYMBOL(bitmap_equal);
This is harmful to the performance.
Reported-by: kernel test robot <lkp@intel.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Max Filippov <jcmvbkbc@gmail.com>
Cc: Andrew Pinski <pinskia@gmail.com>
Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92938
Signed-off-by: Barry Song <song.bao.hua@hisilicon.com>
---
include/linux/bitmap.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
index 37f36dad18bd..3eec9f68a0b6 100644
--- a/include/linux/bitmap.h
+++ b/include/linux/bitmap.h
@@ -258,7 +258,7 @@ static inline void bitmap_copy(unsigned long *dst, const unsigned long *src,
/*
* Copy bitmap and clear tail bits in last word.
*/
-static inline void bitmap_copy_clear_tail(unsigned long *dst,
+static __always_inline void bitmap_copy_clear_tail(unsigned long *dst,
const unsigned long *src, unsigned int nbits)
{
bitmap_copy(dst, src, nbits);
@@ -334,7 +334,7 @@ static inline void bitmap_complement(unsigned long *dst, const unsigned long *sr
#endif
#define BITMAP_MEM_MASK (BITMAP_MEM_ALIGNMENT - 1)
-static inline int bitmap_equal(const unsigned long *src1,
+static __always_inline int bitmap_equal(const unsigned long *src1,
const unsigned long *src2, unsigned int nbits)
{
if (small_const_nbits(nbits))
--
2.25.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] lib: bitmap: Mute some odd section mismatch warning in xtensa kernel build
2021-08-15 3:21 [PATCH] lib: bitmap: Mute some odd section mismatch warning in xtensa kernel build Barry Song
@ 2021-08-15 10:30 ` Andy Shevchenko
2021-08-15 11:25 ` Barry Song
2021-08-15 15:55 ` Yury Norov
1 sibling, 1 reply; 4+ messages in thread
From: Andy Shevchenko @ 2021-08-15 10:30 UTC (permalink / raw)
To: Barry Song
Cc: Greg Kroah-Hartman, Yury Norov, Andy Shevchenko,
Rasmus Villemoes, Linux Kernel Mailing List, Linuxarm,
Barry Song, kernel test robot, Max Filippov, Andrew Pinski
On Sun, Aug 15, 2021 at 6:23 AM Barry Song <21cnbao@gmail.com> wrote:
>
> From: Barry Song <song.bao.hua@hisilicon.com>
>
> Constanly there are some section mismatch issues reported in test_bitmap
Constantly
> for xtensa platform such as:
>
> Section mismatch in reference from the function bitmap_equal() to the
> variable .init.data:initcall_level_names
> The function bitmap_equal() references the variable __initconst
> __setup_str_initcall_blacklist. This is often because bitmap_equal
> lacks a __initconst annotation or the annotation of
> __setup_str_initcall_blacklist is wrong.
>
> Section mismatch in reference from the function bitmap_copy_clear_tail()
> to the variable .init.rodata:__setup_str_initcall_blacklist
> The function bitmap_copy_clear_tail() references the variable __initconst
> __setup_str_initcall_blacklist.
> This is often because bitmap_copy_clear_tail lacks a __initconst
> annotation or the annotation of __setup_str_initcall_blacklist is wrong.
>
> To be honest, hardly to believe kernel code is wrong since bitmap_equal is
bitmap_equal()
> always called in __init function in test_bitmap.c just like __bitmap_equal.
__bitmap_equal()
> But gcc doesn't report any issue for __bitmap_equal even when bitmap_equal
> and __bitmap_equal show in the same function such as:
Ditto as above in both lines.
> static void noinline __init test_mem_optimisations(void)
> {
> ...
> for (start = 0; start < 1024; start += 8) {
> for (nbits = 0; nbits < 1024 - start; nbits += 8) {
> if (!bitmap_equal(bmap1, bmap2, 1024)) {
> failed_tests++;
> }
> if (!__bitmap_equal(bmap1, bmap2, 1024)) {
> failed_tests++;
> }
> ...
> }
> }
> }
>
> The different between __bitmap_equal() and bitmap_equal() is that the
> former is extern and a EXPORT_SYMBOL. So noinline, and probably in fact
and an EXPORT_SYMBOL
> noclone. But the later is static and unfortunately not inlined at this
latter
> time though it has a "inline" flag.
has an "inline"
> bitmap_copy_clear_tail(), on the other hand, seems more innocent as it is
> accessing stack only by its wrapper bitmap_from_arr32() in function
> test_bitmap_arr32():
> static void __init test_bitmap_arr32(void)
> {
> unsigned int nbits, next_bit;
> u32 arr[EXP1_IN_BITS / 32];
> DECLARE_BITMAP(bmap2, EXP1_IN_BITS);
>
> memset(arr, 0xa5, sizeof(arr));
>
> for (nbits = 0; nbits < EXP1_IN_BITS; ++nbits) {
> bitmap_to_arr32(arr, exp1, nbits);
> bitmap_from_arr32(bmap2, arr, nbits);
> expect_eq_bitmap(bmap2, exp1, nbits);
> ...
> }
> }
> Looks like gcc optimized arr, bmap2 things to .init.data but it seems
> nothing is wrong in kernel since test_bitmap_arr32() is __init.
in the kernel
> Max Filippov reported a bug to gcc but gcc people don't ack. So here
> this patch removes the involved symbols by forcing inline. It might
> not be that elegant but I don't see any harm as bitmap_equal() and
> bitmap_copy_clear_tail() are both quite small. In addition, kernel
> doc also backs this modification "We don't use the 'inline' keyword
> because it's broken": www.kernel.org/doc/local/inline.html
>
> Another possible way to "fix" the warning is moving the involved
> symboms to lib/bitmap.c:
symbols
>
> +int bitmap_equal(const unsigned long *src1,
> + const unsigned long *src2, unsigned int nbits)
> +{
> + if (small_const_nbits(nbits))
> + return !((*src1 ^ *src2) & BITMAP_LAST_WORD_MASK(nbits));
> + if (__builtin_constant_p(nbits & BITMAP_MEM_MASK) &&
> + IS_ALIGNED(nbits, BITMAP_MEM_ALIGNMENT))
> + return !memcmp(src1, src2, nbits / 8);
> + return __bitmap_equal(src1, src2, nbits);
> +}
> +EXPORT_SYMBOL(bitmap_equal);
>
> This is harmful to the performance.
I'm afraid it's a bit of a slippery road. These two are currently
being used in tests, what if somebody extends tests with something
else similar? Will we need to __always_inline more symbols because of
that? What about non-bitmap APIs?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] lib: bitmap: Mute some odd section mismatch warning in xtensa kernel build
2021-08-15 10:30 ` Andy Shevchenko
@ 2021-08-15 11:25 ` Barry Song
0 siblings, 0 replies; 4+ messages in thread
From: Barry Song @ 2021-08-15 11:25 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Greg Kroah-Hartman, Yury Norov, Andy Shevchenko,
Rasmus Villemoes, Linux Kernel Mailing List, Linuxarm,
Barry Song, kernel test robot, Max Filippov, Andrew Pinski
> On Sun, Aug 15, 2021 at 6:23 AM Barry Song <21cnbao@gmail.com> wrote:
> >
> > From: Barry Song <song.bao.hua@hisilicon.com>
> >
> > Constanly there are some section mismatch issues reported in test_bitmap
>
> Constantly
>
> > for xtensa platform such as:
> >
> > Section mismatch in reference from the function bitmap_equal() to the
> > variable .init.data:initcall_level_names
> > The function bitmap_equal() references the variable __initconst
> > __setup_str_initcall_blacklist. This is often because bitmap_equal
> > lacks a __initconst annotation or the annotation of
> > __setup_str_initcall_blacklist is wrong.
> >
> > Section mismatch in reference from the function
> bitmap_copy_clear_tail()
> > to the variable .init.rodata:__setup_str_initcall_blacklist
> > The function bitmap_copy_clear_tail() references the variable
> __initconst
> > __setup_str_initcall_blacklist.
> > This is often because bitmap_copy_clear_tail lacks a __initconst
> > annotation or the annotation of __setup_str_initcall_blacklist is
> wrong.
> >
> > To be honest, hardly to believe kernel code is wrong since bitmap_equal
> is
>
> bitmap_equal()
>
> > always called in __init function in test_bitmap.c just like
> __bitmap_equal.
>
> __bitmap_equal()
>
> > But gcc doesn't report any issue for __bitmap_equal even when
> bitmap_equal
> > and __bitmap_equal show in the same function such as:
>
> Ditto as above in both lines.
>
> > static void noinline __init test_mem_optimisations(void)
> > {
> > ...
> > for (start = 0; start < 1024; start += 8) {
> > for (nbits = 0; nbits < 1024 - start; nbits += 8) {
> > if (!bitmap_equal(bmap1, bmap2, 1024)) {
> > failed_tests++;
> > }
> > if (!__bitmap_equal(bmap1, bmap2, 1024)) {
> > failed_tests++;
> > }
> > ...
> > }
> > }
> > }
> >
> > The different between __bitmap_equal() and bitmap_equal() is that the
> > former is extern and a EXPORT_SYMBOL. So noinline, and probably in fact
>
> and an EXPORT_SYMBOL
>
> > noclone. But the later is static and unfortunately not inlined at this
>
> latter
>
> > time though it has a "inline" flag.
>
> has an "inline"
>
> > bitmap_copy_clear_tail(), on the other hand, seems more innocent as it is
> > accessing stack only by its wrapper bitmap_from_arr32() in function
> > test_bitmap_arr32():
> > static void __init test_bitmap_arr32(void)
> > {
> > unsigned int nbits, next_bit;
> > u32 arr[EXP1_IN_BITS / 32];
> > DECLARE_BITMAP(bmap2, EXP1_IN_BITS);
> >
> > memset(arr, 0xa5, sizeof(arr));
> >
> > for (nbits = 0; nbits < EXP1_IN_BITS; ++nbits) {
> > bitmap_to_arr32(arr, exp1, nbits);
> > bitmap_from_arr32(bmap2, arr, nbits);
> > expect_eq_bitmap(bmap2, exp1, nbits);
> > ...
> > }
> > }
> > Looks like gcc optimized arr, bmap2 things to .init.data but it seems
> > nothing is wrong in kernel since test_bitmap_arr32() is __init.
>
> in the kernel
>
> > Max Filippov reported a bug to gcc but gcc people don't ack. So here
> > this patch removes the involved symbols by forcing inline. It might
> > not be that elegant but I don't see any harm as bitmap_equal() and
> > bitmap_copy_clear_tail() are both quite small. In addition, kernel
> > doc also backs this modification "We don't use the 'inline' keyword
> > because it's broken": www.kernel.org/doc/local/inline.html
> >
> > Another possible way to "fix" the warning is moving the involved
> > symboms to lib/bitmap.c:
>
> symbols
>
> >
> > +int bitmap_equal(const unsigned long *src1,
> > + const unsigned long *src2, unsigned int nbits)
> > +{
> > + if (small_const_nbits(nbits))
> > + return !((*src1 ^ *src2) &
> BITMAP_LAST_WORD_MASK(nbits));
> > + if (__builtin_constant_p(nbits & BITMAP_MEM_MASK) &&
> > + IS_ALIGNED(nbits, BITMAP_MEM_ALIGNMENT))
> > + return !memcmp(src1, src2, nbits / 8);
> > + return __bitmap_equal(src1, src2, nbits);
> > +}
> > +EXPORT_SYMBOL(bitmap_equal);
> >
> > This is harmful to the performance.
>
> I'm afraid it's a bit of a slippery road. These two are currently
> being used in tests, what if somebody extends tests with something
> else similar? Will we need to __always_inline more symbols because of
> that? What about non-bitmap APIs?
>
you are right. Andy, we will have to mark those functions one by one
if gcc doesn't want to change. actually i am seeing the same issue in
lib/find_bit_benchmark.c whose __init functions are also calling
static inline bitmap api.
not quite issue if it is specific to xentas. i'd welcome a global gcc
option to disable this kind of clone for xentas or i would be happy to
see gcc people ack this is a gcc issue.
> --
> With Best Regards,
> Andy Shevchenko
>
Thanks
Barry
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] lib: bitmap: Mute some odd section mismatch warning in xtensa kernel build
2021-08-15 3:21 [PATCH] lib: bitmap: Mute some odd section mismatch warning in xtensa kernel build Barry Song
2021-08-15 10:30 ` Andy Shevchenko
@ 2021-08-15 15:55 ` Yury Norov
1 sibling, 0 replies; 4+ messages in thread
From: Yury Norov @ 2021-08-15 15:55 UTC (permalink / raw)
To: Barry Song
Cc: gregkh, andriy.shevchenko, linux, linux-kernel, linuxarm,
Barry Song, kernel test robot, Max Filippov, Andrew Pinski,
linux-xtensa, gcc, gcc-bugs
On Sun, Aug 15, 2021 at 03:21:32PM +1200, Barry Song wrote:
> From: Barry Song <song.bao.hua@hisilicon.com>
>
> Constanly there are some section mismatch issues reported in test_bitmap
> for xtensa platform such as:
>
> Section mismatch in reference from the function bitmap_equal() to the
> variable .init.data:initcall_level_names
> The function bitmap_equal() references the variable __initconst
> __setup_str_initcall_blacklist. This is often because bitmap_equal
> lacks a __initconst annotation or the annotation of
> __setup_str_initcall_blacklist is wrong.
>
> Section mismatch in reference from the function bitmap_copy_clear_tail()
> to the variable .init.rodata:__setup_str_initcall_blacklist
> The function bitmap_copy_clear_tail() references the variable __initconst
> __setup_str_initcall_blacklist.
> This is often because bitmap_copy_clear_tail lacks a __initconst
> annotation or the annotation of __setup_str_initcall_blacklist is wrong.
>
> To be honest, hardly to believe kernel code is wrong since bitmap_equal is
> always called in __init function in test_bitmap.c just like __bitmap_equal.
> But gcc doesn't report any issue for __bitmap_equal even when bitmap_equal
> and __bitmap_equal show in the same function such as:
>
> static void noinline __init test_mem_optimisations(void)
> {
> ...
> for (start = 0; start < 1024; start += 8) {
> for (nbits = 0; nbits < 1024 - start; nbits += 8) {
> if (!bitmap_equal(bmap1, bmap2, 1024)) {
> failed_tests++;
> }
> if (!__bitmap_equal(bmap1, bmap2, 1024)) {
> failed_tests++;
> }
> ...
> }
> }
> }
>
> The different between __bitmap_equal() and bitmap_equal() is that the
> former is extern and a EXPORT_SYMBOL. So noinline, and probably in fact
> noclone. But the later is static and unfortunately not inlined at this
> time though it has a "inline" flag.
>
> bitmap_copy_clear_tail(), on the other hand, seems more innocent as it is
> accessing stack only by its wrapper bitmap_from_arr32() in function
> test_bitmap_arr32():
> static void __init test_bitmap_arr32(void)
> {
> unsigned int nbits, next_bit;
> u32 arr[EXP1_IN_BITS / 32];
> DECLARE_BITMAP(bmap2, EXP1_IN_BITS);
>
> memset(arr, 0xa5, sizeof(arr));
>
> for (nbits = 0; nbits < EXP1_IN_BITS; ++nbits) {
> bitmap_to_arr32(arr, exp1, nbits);
> bitmap_from_arr32(bmap2, arr, nbits);
> expect_eq_bitmap(bmap2, exp1, nbits);
> ...
> }
> }
> Looks like gcc optimized arr, bmap2 things to .init.data but it seems
> nothing is wrong in kernel since test_bitmap_arr32() is __init.
>
> Max Filippov reported a bug to gcc but gcc people don't ack. So here
> this patch removes the involved symbols by forcing inline. It might
> not be that elegant but I don't see any harm as bitmap_equal() and
> bitmap_copy_clear_tail() are both quite small. In addition, kernel
> doc also backs this modification "We don't use the 'inline' keyword
> because it's broken": www.kernel.org/doc/local/inline.html
This is a 2006 article. Are you sure nothing has been changed over the
last 15 years?
> Another possible way to "fix" the warning is moving the involved
> symboms to lib/bitmap.c:
So, it's a GCC issue already reported to GCC? For me it sounds like
nothing to fix in kernel. If I was a GCC developer, I'd prefer to have
all bugs clearly reproducible.
Let's wait for GCC and xtensa people comments. (CC xtensa and GCC
lists)
Yury
> +int bitmap_equal(const unsigned long *src1,
> + const unsigned long *src2, unsigned int nbits)
> +{
> + if (small_const_nbits(nbits))
> + return !((*src1 ^ *src2) & BITMAP_LAST_WORD_MASK(nbits));
> + if (__builtin_constant_p(nbits & BITMAP_MEM_MASK) &&
> + IS_ALIGNED(nbits, BITMAP_MEM_ALIGNMENT))
> + return !memcmp(src1, src2, nbits / 8);
> + return __bitmap_equal(src1, src2, nbits);
> +}
> +EXPORT_SYMBOL(bitmap_equal);
>
> This is harmful to the performance.
>
> Reported-by: kernel test robot <lkp@intel.com>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Max Filippov <jcmvbkbc@gmail.com>
> Cc: Andrew Pinski <pinskia@gmail.com>
> Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92938
> Signed-off-by: Barry Song <song.bao.hua@hisilicon.com>
> ---
> include/linux/bitmap.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
> index 37f36dad18bd..3eec9f68a0b6 100644
> --- a/include/linux/bitmap.h
> +++ b/include/linux/bitmap.h
> @@ -258,7 +258,7 @@ static inline void bitmap_copy(unsigned long *dst, const unsigned long *src,
> /*
> * Copy bitmap and clear tail bits in last word.
> */
> -static inline void bitmap_copy_clear_tail(unsigned long *dst,
> +static __always_inline void bitmap_copy_clear_tail(unsigned long *dst,
> const unsigned long *src, unsigned int nbits)
> {
> bitmap_copy(dst, src, nbits);
> @@ -334,7 +334,7 @@ static inline void bitmap_complement(unsigned long *dst, const unsigned long *sr
> #endif
> #define BITMAP_MEM_MASK (BITMAP_MEM_ALIGNMENT - 1)
>
> -static inline int bitmap_equal(const unsigned long *src1,
> +static __always_inline int bitmap_equal(const unsigned long *src1,
> const unsigned long *src2, unsigned int nbits)
> {
> if (small_const_nbits(nbits))
> --
> 2.25.1
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-08-15 15:55 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-15 3:21 [PATCH] lib: bitmap: Mute some odd section mismatch warning in xtensa kernel build Barry Song
2021-08-15 10:30 ` Andy Shevchenko
2021-08-15 11:25 ` Barry Song
2021-08-15 15:55 ` Yury Norov
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).