Re: [PATCH] lib: bitmap: Mute some odd section mismatch warning in xtensa kernel build

From: Andy Shevchenko
Date: Sun Aug 15 2021 - 06:31:20 EST


On Sun, Aug 15, 2021 at 6:23 AM Barry Song <21cnbao@xxxxxxxxx> wrote:
>
> From: Barry Song <song.bao.hua@xxxxxxxxxxxxx>
>
> 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