Date:   Sun, 15 Aug 2021 23:25:35 +1200
From:   Barry Song <21cnbao@...il.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Yury Norov <yury.norov@...il.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linuxarm <linuxarm@...wei.com>,
        Barry Song <song.bao.hua@...ilicon.com>,
        kernel test robot <lkp@...el.com>,
        Max Filippov <jcmvbkbc@...il.com>,
        Andrew Pinski <pinskia@...il.com>
Subject: Re: [PATCH] lib: bitmap: Mute some odd section mismatch warning in
 xtensa kernel build

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

Powered by blists - more mailing lists