| Message ID | 20210225133808.2188581-1-arnd@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | memblock: fix section mismatch warning | expand |
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 :) )
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
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>
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
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
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 >
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; }