[PATCH] memblock: fix section mismatch warning

[PATCH] memblock: fix section mismatch warning

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

The inlining logic in clang-13 is rewritten to often not inline
some functions that were inlined by all earlier compilers.

In case of the memblock interfaces, this exposed a harmless bug
of a missing __init annotation:

WARNING: modpost: vmlinux.o(.text+0x507c0a): Section mismatch in reference from the function memblock_bottom_up() to the variable .meminit.data:memblock
The function memblock_bottom_up() references
the variable __meminitdata memblock.
This is often because memblock_bottom_up lacks a __meminitdata
annotation or the annotation of memblock is wrong.

Interestingly, these annotations were present originally, but got removed
with the explanation that the __init annotation prevents the function
from getting inlined. I checked this again and found that while this
is the case with clang, gcc (version 7 through 10, did not test others)
does inline the functions regardless.

As the previous change was apparently intended to help the clang
builds, reverting it to help the newer clang versions seems appropriate
as well. gcc builds don't seem to care either way.

Fixes: 5bdba520c1b3 ("mm: memblock: drop __init from memblock functions to make it inline")
Reference: 2cfb3665e864 ("include/linux/memblock.h: add __init to memblock_set_bottom_up()")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 include/linux/memblock.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/memblock.h b/include/linux/memblock.h
index c88bc24e31aa..d13e3cd938b4 100644
--- a/include/linux/memblock.h
+++ b/include/linux/memblock.h
@@ -460,7 +460,7 @@ static inline void memblock_free_late(phys_addr_t base, phys_addr_t size)
 /*
  * Set the allocation direction to bottom-up or top-down.
  */
-static inline void memblock_set_bottom_up(bool enable)
+static inline __init void memblock_set_bottom_up(bool enable)
 {
 	memblock.bottom_up = enable;
 }
@@ -470,7 +470,7 @@ static inline void memblock_set_bottom_up(bool enable)
  * if this is true, that said, memblock will allocate memory
  * in bottom-up direction.
  */
-static inline bool memblock_bottom_up(void)
+static inline __init bool memblock_bottom_up(void)
 {
 	return memblock.bottom_up;
 }
-- 
2.29.2

Re: [PATCH] memblock: fix section mismatch warning

[David Hildenbrand]

On 25.02.21 14:38, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> The inlining logic in clang-13 is rewritten to often not inline
> some functions that were inlined by all earlier compilers.
> 
> In case of the memblock interfaces, this exposed a harmless bug
> of a missing __init annotation:
> 
> WARNING: modpost: vmlinux.o(.text+0x507c0a): Section mismatch in reference from the function memblock_bottom_up() to the variable .meminit.data:memblock
> The function memblock_bottom_up() references
> the variable __meminitdata memblock.
> This is often because memblock_bottom_up lacks a __meminitdata
> annotation or the annotation of memblock is wrong.
> 
> Interestingly, these annotations were present originally, but got removed
> with the explanation that the __init annotation prevents the function
> from getting inlined. I checked this again and found that while this
> is the case with clang, gcc (version 7 through 10, did not test others)
> does inline the functions regardless.

Did I understand correctly, that with this change it will not get 
inlined with any version of clang? Maybe __always_inline is more 
appropriate then.

(I don't see why to not inline that function, but I am obviously not a 
compiler person :) )

-- 
Thanks,

David / dhildenb

Re: [PATCH] memblock: fix section mismatch warning

[Arnd Bergmann]

On Thu, Feb 25, 2021 at 2:47 PM David Hildenbrand <david@redhat.com> wrote:
>
> On 25.02.21 14:38, Arnd Bergmann wrote:
> > From: Arnd Bergmann <arnd@arndb.de>
> >
> > The inlining logic in clang-13 is rewritten to often not inline
> > some functions that were inlined by all earlier compilers.
> >
> > In case of the memblock interfaces, this exposed a harmless bug
> > of a missing __init annotation:
> >
> > WARNING: modpost: vmlinux.o(.text+0x507c0a): Section mismatch in reference from the function memblock_bottom_up() to the variable .meminit.data:memblock
> > The function memblock_bottom_up() references
> > the variable __meminitdata memblock.
> > This is often because memblock_bottom_up lacks a __meminitdata
> > annotation or the annotation of memblock is wrong.
> >
> > Interestingly, these annotations were present originally, but got removed
> > with the explanation that the __init annotation prevents the function
> > from getting inlined. I checked this again and found that while this
> > is the case with clang, gcc (version 7 through 10, did not test others)
> > does inline the functions regardless.
>
> Did I understand correctly, that with this change it will not get
> inlined with any version of clang? Maybe __always_inline is more
> appropriate then.
>
> (I don't see why to not inline that function, but I am obviously not a
> compiler person :) )

Looking at the assembler output in the arm64 build that triggered the
warning, I see this code:

0000000000000a40 <memblock_bottom_up>:
 a40:   55                      push   %rbp
 a41:   48 89 e5                mov    %rsp,%rbp
 a44:   41 56                   push   %r14
 a46:   53                      push   %rbx
 a47:   e8 00 00 00 00          call   a4c <memblock_bottom_up+0xc>
                        a48: R_X86_64_PLT32     __sanitizer_cov_trace_pc-0x4
 a4c:   48 c7 c7 00 00 00 00    mov    $0x0,%rdi
                        a4f: R_X86_64_32S       memblock
 a53:   e8 00 00 00 00          call   a58 <memblock_bottom_up+0x18>
                        a54: R_X86_64_PLT32     __asan_load1_noabort-0x4
 a58:   44 0f b6 35 00 00 00    movzbl 0x0(%rip),%r14d        # a60
<memblock_bottom_up+0x20>
 a5f:   00
                        a5c: R_X86_64_PC32      memblock-0x4
 a60:   bf 02 00 00 00          mov    $0x2,%edi
 a65:   44 89 f6                mov    %r14d,%esi
 a68:   e8 00 00 00 00          call   a6d <memblock_bottom_up+0x2d>
                        a69: R_X86_64_PLT32
__sanitizer_cov_trace_const_cmp1-0x4
 a6d:   41 83 fe 01             cmp    $0x1,%r14d
 a71:   77 20                   ja     a93 <memblock_bottom_up+0x53>
 a73:   e8 00 00 00 00          call   a78 <memblock_bottom_up+0x38>
                        a74: R_X86_64_PLT32     __sanitizer_cov_trace_pc-0x4
 a78:   44 89 f3                mov    %r14d,%ebx
 a7b:   80 e3 01                and    $0x1,%bl
 a7e:   41 83 e6 01             and    $0x1,%r14d
 a82:   31 ff                   xor    %edi,%edi
 a84:   44 89 f6                mov    %r14d,%esi
 a87:   e8 00 00 00 00          call   a8c <memblock_bottom_up+0x4c>
                        a88: R_X86_64_PLT32
__sanitizer_cov_trace_const_cmp1-0x4
 a8c:   89 d8                   mov    %ebx,%eax
 a8e:   5b                      pop    %rbx
 a8f:   41 5e                   pop    %r14
 a91:   5d                      pop    %rbp
 a92:   c3                      ret
 a93:   e8 00 00 00 00          call   a98 <memblock_bottom_up+0x58>
                        a94: R_X86_64_PLT32     __sanitizer_cov_trace_pc-0x4
 a98:   48 c7 c7 00 00 00 00    mov    $0x0,%rdi
                        a9b: R_X86_64_32S       .data+0x3c0
 a9f:   4c 89 f6                mov    %r14,%rsi
 aa2:   e8 00 00 00 00          call   aa7 <memblock_bottom_up+0x67>
                        aa3: R_X86_64_PLT32
__ubsan_handle_load_invalid_value-0x4
 aa7:   eb cf                   jmp    a78 <memblock_bottom_up+0x38>
 aa9:   66 2e 0f 1f 84 00 00    cs nopw 0x0(%rax,%rax,1)
 ab0:   00 00 00
 ab3:   66 2e 0f 1f 84 00 00    cs nopw 0x0(%rax,%rax,1)
 aba:   00 00 00
 abd:   0f 1f 00                nopl   (%rax)

This means that the sanitiers added a lot of extra checking around what
would have been a trivial global variable access otherwise. In this case,
not inlining would be a reasonable decision.

      Arnd

Re: [PATCH] memblock: fix section mismatch warning

[David Hildenbrand]

On 25.02.21 15:06, Arnd Bergmann wrote:
> On Thu, Feb 25, 2021 at 2:47 PM David Hildenbrand <david@redhat.com> wrote:
>>
>> On 25.02.21 14:38, Arnd Bergmann wrote:
>>> From: Arnd Bergmann <arnd@arndb.de>
>>>
>>> The inlining logic in clang-13 is rewritten to often not inline
>>> some functions that were inlined by all earlier compilers.
>>>
>>> In case of the memblock interfaces, this exposed a harmless bug
>>> of a missing __init annotation:
>>>
>>> WARNING: modpost: vmlinux.o(.text+0x507c0a): Section mismatch in reference from the function memblock_bottom_up() to the variable .meminit.data:memblock
>>> The function memblock_bottom_up() references
>>> the variable __meminitdata memblock.
>>> This is often because memblock_bottom_up lacks a __meminitdata
>>> annotation or the annotation of memblock is wrong.
>>>
>>> Interestingly, these annotations were present originally, but got removed
>>> with the explanation that the __init annotation prevents the function
>>> from getting inlined. I checked this again and found that while this
>>> is the case with clang, gcc (version 7 through 10, did not test others)
>>> does inline the functions regardless.
>>
>> Did I understand correctly, that with this change it will not get
>> inlined with any version of clang? Maybe __always_inline is more
>> appropriate then.
>>
>> (I don't see why to not inline that function, but I am obviously not a
>> compiler person :) )
> 
> Looking at the assembler output in the arm64 build that triggered the
> warning, I see this code:
> 
> 0000000000000a40 <memblock_bottom_up>:
>   a40:   55                      push   %rbp
>   a41:   48 89 e5                mov    %rsp,%rbp
>   a44:   41 56                   push   %r14
>   a46:   53                      push   %rbx
>   a47:   e8 00 00 00 00          call   a4c <memblock_bottom_up+0xc>
>                          a48: R_X86_64_PLT32     __sanitizer_cov_trace_pc-0x4
>   a4c:   48 c7 c7 00 00 00 00    mov    $0x0,%rdi
>                          a4f: R_X86_64_32S       memblock
>   a53:   e8 00 00 00 00          call   a58 <memblock_bottom_up+0x18>
>                          a54: R_X86_64_PLT32     __asan_load1_noabort-0x4
>   a58:   44 0f b6 35 00 00 00    movzbl 0x0(%rip),%r14d        # a60
> <memblock_bottom_up+0x20>
>   a5f:   00
>                          a5c: R_X86_64_PC32      memblock-0x4
>   a60:   bf 02 00 00 00          mov    $0x2,%edi
>   a65:   44 89 f6                mov    %r14d,%esi
>   a68:   e8 00 00 00 00          call   a6d <memblock_bottom_up+0x2d>
>                          a69: R_X86_64_PLT32
> __sanitizer_cov_trace_const_cmp1-0x4
>   a6d:   41 83 fe 01             cmp    $0x1,%r14d
>   a71:   77 20                   ja     a93 <memblock_bottom_up+0x53>
>   a73:   e8 00 00 00 00          call   a78 <memblock_bottom_up+0x38>
>                          a74: R_X86_64_PLT32     __sanitizer_cov_trace_pc-0x4
>   a78:   44 89 f3                mov    %r14d,%ebx
>   a7b:   80 e3 01                and    $0x1,%bl
>   a7e:   41 83 e6 01             and    $0x1,%r14d
>   a82:   31 ff                   xor    %edi,%edi
>   a84:   44 89 f6                mov    %r14d,%esi
>   a87:   e8 00 00 00 00          call   a8c <memblock_bottom_up+0x4c>
>                          a88: R_X86_64_PLT32
> __sanitizer_cov_trace_const_cmp1-0x4
>   a8c:   89 d8                   mov    %ebx,%eax
>   a8e:   5b                      pop    %rbx
>   a8f:   41 5e                   pop    %r14
>   a91:   5d                      pop    %rbp
>   a92:   c3                      ret
>   a93:   e8 00 00 00 00          call   a98 <memblock_bottom_up+0x58>
>                          a94: R_X86_64_PLT32     __sanitizer_cov_trace_pc-0x4
>   a98:   48 c7 c7 00 00 00 00    mov    $0x0,%rdi
>                          a9b: R_X86_64_32S       .data+0x3c0
>   a9f:   4c 89 f6                mov    %r14,%rsi
>   aa2:   e8 00 00 00 00          call   aa7 <memblock_bottom_up+0x67>
>                          aa3: R_X86_64_PLT32
> __ubsan_handle_load_invalid_value-0x4
>   aa7:   eb cf                   jmp    a78 <memblock_bottom_up+0x38>
>   aa9:   66 2e 0f 1f 84 00 00    cs nopw 0x0(%rax,%rax,1)
>   ab0:   00 00 00
>   ab3:   66 2e 0f 1f 84 00 00    cs nopw 0x0(%rax,%rax,1)
>   aba:   00 00 00
>   abd:   0f 1f 00                nopl   (%rax)
> 
> This means that the sanitiers added a lot of extra checking around what
> would have been a trivial global variable access otherwise. In this case,
> not inlining would be a reasonable decision.

It's not like if there are a lot of call sites:

  $ git grep memblock_bottom_up
arch/x86/mm/init.c:     if (memblock_bottom_up()) {
include/linux/memblock.h:static inline bool memblock_bottom_up(void)
mm/cma.c:               if (!memblock_bottom_up() && memblock_end >= SZ_4G + size) {
mm/memblock.c:  if (memblock_bottom_up())


Similarly for memblock_set_bottom_up() within a kernel image.

But it's not like this is performance-sensitive code :)

Thanks!

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

Re: [PATCH] memblock: fix section mismatch warning

[Mike Rapoport]

On Thu, Feb 25, 2021 at 03:06:27PM +0100, Arnd Bergmann wrote:
> On Thu, Feb 25, 2021 at 2:47 PM David Hildenbrand <david@redhat.com> wrote:
> >
> > On 25.02.21 14:38, Arnd Bergmann wrote:
> > > From: Arnd Bergmann <arnd@arndb.de>
> > >
> > > The inlining logic in clang-13 is rewritten to often not inline
> > > some functions that were inlined by all earlier compilers.
> > >
> > > In case of the memblock interfaces, this exposed a harmless bug
> > > of a missing __init annotation:
> > >
> > > WARNING: modpost: vmlinux.o(.text+0x507c0a): Section mismatch in reference from the function memblock_bottom_up() to the variable .meminit.data:memblock
> > > The function memblock_bottom_up() references
> > > the variable __meminitdata memblock.
> > > This is often because memblock_bottom_up lacks a __meminitdata
> > > annotation or the annotation of memblock is wrong.
> > >
> > > Interestingly, these annotations were present originally, but got removed
> > > with the explanation that the __init annotation prevents the function
> > > from getting inlined. I checked this again and found that while this
> > > is the case with clang, gcc (version 7 through 10, did not test others)
> > > does inline the functions regardless.
> >
> > Did I understand correctly, that with this change it will not get
> > inlined with any version of clang? Maybe __always_inline is more
> > appropriate then.
> >
> > (I don't see why to not inline that function, but I am obviously not a
> > compiler person :) )
> 
> Looking at the assembler output in the arm64 build that triggered the
> warning, I see this code:

"push %rbp" seems more x86 for me, but that's not really important :)

I wonder what happens with other memblock inline APIs, particularly with
alloc wrappers. Do they still get inlined?

> 0000000000000a40 <memblock_bottom_up>:
>  a40:   55                      push   %rbp
>  a41:   48 89 e5                mov    %rsp,%rbp
>  a44:   41 56                   push   %r14
>  a46:   53                      push   %rbx
>  a47:   e8 00 00 00 00          call   a4c <memblock_bottom_up+0xc>
>                         a48: R_X86_64_PLT32     __sanitizer_cov_trace_pc-0x4
>  a4c:   48 c7 c7 00 00 00 00    mov    $0x0,%rdi
>                         a4f: R_X86_64_32S       memblock
>  a53:   e8 00 00 00 00          call   a58 <memblock_bottom_up+0x18>
>                         a54: R_X86_64_PLT32     __asan_load1_noabort-0x4
>  a58:   44 0f b6 35 00 00 00    movzbl 0x0(%rip),%r14d        # a60
> <memblock_bottom_up+0x20>
>  a5f:   00
>                         a5c: R_X86_64_PC32      memblock-0x4
>  a60:   bf 02 00 00 00          mov    $0x2,%edi
>  a65:   44 89 f6                mov    %r14d,%esi
>  a68:   e8 00 00 00 00          call   a6d <memblock_bottom_up+0x2d>
>                         a69: R_X86_64_PLT32
> __sanitizer_cov_trace_const_cmp1-0x4
>  a6d:   41 83 fe 01             cmp    $0x1,%r14d
>  a71:   77 20                   ja     a93 <memblock_bottom_up+0x53>
>  a73:   e8 00 00 00 00          call   a78 <memblock_bottom_up+0x38>
>                         a74: R_X86_64_PLT32     __sanitizer_cov_trace_pc-0x4
>  a78:   44 89 f3                mov    %r14d,%ebx
>  a7b:   80 e3 01                and    $0x1,%bl
>  a7e:   41 83 e6 01             and    $0x1,%r14d
>  a82:   31 ff                   xor    %edi,%edi
>  a84:   44 89 f6                mov    %r14d,%esi
>  a87:   e8 00 00 00 00          call   a8c <memblock_bottom_up+0x4c>
>                         a88: R_X86_64_PLT32
> __sanitizer_cov_trace_const_cmp1-0x4
>  a8c:   89 d8                   mov    %ebx,%eax
>  a8e:   5b                      pop    %rbx
>  a8f:   41 5e                   pop    %r14
>  a91:   5d                      pop    %rbp
>  a92:   c3                      ret
>  a93:   e8 00 00 00 00          call   a98 <memblock_bottom_up+0x58>
>                         a94: R_X86_64_PLT32     __sanitizer_cov_trace_pc-0x4
>  a98:   48 c7 c7 00 00 00 00    mov    $0x0,%rdi
>                         a9b: R_X86_64_32S       .data+0x3c0
>  a9f:   4c 89 f6                mov    %r14,%rsi
>  aa2:   e8 00 00 00 00          call   aa7 <memblock_bottom_up+0x67>
>                         aa3: R_X86_64_PLT32
> __ubsan_handle_load_invalid_value-0x4
>  aa7:   eb cf                   jmp    a78 <memblock_bottom_up+0x38>
>  aa9:   66 2e 0f 1f 84 00 00    cs nopw 0x0(%rax,%rax,1)
>  ab0:   00 00 00
>  ab3:   66 2e 0f 1f 84 00 00    cs nopw 0x0(%rax,%rax,1)
>  aba:   00 00 00
>  abd:   0f 1f 00                nopl   (%rax)
> 
> This means that the sanitiers added a lot of extra checking around what
> would have been a trivial global variable access otherwise. In this case,
> not inlining would be a reasonable decision.
> 
>       Arnd

-- 
Sincerely yours,
Mike.

Re: [PATCH] memblock: fix section mismatch warning

[Arnd Bergmann]

On Thu, Feb 25, 2021 at 4:08 PM Mike Rapoport <rppt@kernel.org> wrote:
> On Thu, Feb 25, 2021 at 03:06:27PM +0100, Arnd Bergmann wrote:
> > On Thu, Feb 25, 2021 at 2:47 PM David Hildenbrand <david@redhat.com> wrote:
> > >
> > > (I don't see why to not inline that function, but I am obviously not a
> > > compiler person :) )
> >
> > Looking at the assembler output in the arm64 build that triggered the
> > warning, I see this code:
>
> "push %rbp" seems more x86 for me, but that's not really important :)

I suppose the relocation names like "R_X86_64_32S" and the command
line I used should could have told me the same ;-)

> I wonder what happens with other memblock inline APIs, particularly with
> alloc wrappers. Do they still get inlined?

Trying the same configuration here, with all the allocation functions
marked __init again, they all get inlined by clang, regardless of the
'__init' and 'inline' and '__always_inline' tags.

With gcc-7 and gcc-10 one instance of the plain 'memblock_alloc' does not
get fully inlined if I revert the __always_inline back to plain inline:

        .type   memblock_alloc.constprop.0, @function
memblock_alloc.constprop.0:
.LASANPC4090:
        pushq   %rbp    #
# include/linux/memblock.h:407: static inline __init void
*memblock_alloc(phys_addr_t size, phys_addr_t align)
        movq    %rdi, %rbp      # tmp84, size
# include/linux/memblock.h:409:    return memblock_alloc_try_nid(size,
align, MEMBLOCK_LOW_LIMIT,
        call    __sanitizer_cov_trace_pc        #
        movq    %rbp, %rdi      # size,
        orl     $-1, %r8d       #,
        xorl    %ecx, %ecx      #
        xorl    %edx, %edx      #
        movl    $4096, %esi     #,
# include/linux/memblock.h:411: }
        popq    %rbp    #
# include/linux/memblock.h:409:    return memblock_alloc_try_nid(size,
align, MEMBLOCK_LOW_LIMIT,
        jmp     memblock_alloc_try_nid  #
        .size   memblock_alloc.constprop.0, .-memblock_alloc.constprop.0

Apparently, this is an optimization for code size, as there are
multiple callers in
kernel/dma/swiotlb.c and it can move the call to __sanitizer_cov_trace_pc into
a single place here.

       Arnd

Re: [PATCH] memblock: fix section mismatch warning

[Mike Rapoport]

On Thu, Feb 25, 2021 at 02:38:00PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> The inlining logic in clang-13 is rewritten to often not inline
> some functions that were inlined by all earlier compilers.
> 
> In case of the memblock interfaces, this exposed a harmless bug
> of a missing __init annotation:
> 
> WARNING: modpost: vmlinux.o(.text+0x507c0a): Section mismatch in reference from the function memblock_bottom_up() to the variable .meminit.data:memblock
> The function memblock_bottom_up() references
> the variable __meminitdata memblock.
> This is often because memblock_bottom_up lacks a __meminitdata
> annotation or the annotation of memblock is wrong.
> 
> Interestingly, these annotations were present originally, but got removed
> with the explanation that the __init annotation prevents the function
> from getting inlined. I checked this again and found that while this
> is the case with clang, gcc (version 7 through 10, did not test others)
> does inline the functions regardless.
> 
> As the previous change was apparently intended to help the clang
> builds, reverting it to help the newer clang versions seems appropriate
> as well. gcc builds don't seem to care either way.
> 
> Fixes: 5bdba520c1b3 ("mm: memblock: drop __init from memblock functions to make it inline")
> Reference: 2cfb3665e864 ("include/linux/memblock.h: add __init to memblock_set_bottom_up()")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

I thought it'll go via memblock tree but since Andrew has already took it

Reviewed-by: Mike Rapoport <rppt@linux.ibm.com>

> ---
>  include/linux/memblock.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/memblock.h b/include/linux/memblock.h
> index c88bc24e31aa..d13e3cd938b4 100644
> --- a/include/linux/memblock.h
> +++ b/include/linux/memblock.h
> @@ -460,7 +460,7 @@ static inline void memblock_free_late(phys_addr_t base, phys_addr_t size)
>  /*
>   * Set the allocation direction to bottom-up or top-down.
>   */
> -static inline void memblock_set_bottom_up(bool enable)
> +static inline __init void memblock_set_bottom_up(bool enable)
>  {
>  	memblock.bottom_up = enable;
>  }
> @@ -470,7 +470,7 @@ static inline void memblock_set_bottom_up(bool enable)
>   * if this is true, that said, memblock will allocate memory
>   * in bottom-up direction.
>   */
> -static inline bool memblock_bottom_up(void)
> +static inline __init bool memblock_bottom_up(void)
>  {
>  	return memblock.bottom_up;
>  }
> -- 
> 2.29.2
> 

-- 
Sincerely yours,
Mike.