[1/2] futex: mark futex_detect_cmpxchg() as 'noinline'

Message ID 20190307091514.2489338-1-arnd@arndb.de
State New
Headers show
Series
  • [1/2] futex: mark futex_detect_cmpxchg() as 'noinline'
Related show

Commit Message

Arnd Bergmann March 7, 2019, 9:14 a.m.
On 32-bit ARM, I got a link failure in futex_init() when building
with clang in some random configurations:

kernel/futex.o:(.text.fixup+0x5c): relocation truncated to fit: R_ARM_JUMP24 against `.init.text'

As far as I can tell, the problem is that a branch is over 16MB
apart in those configurations, but only if it branches back to
the init text.

Marking the futex_detect_cmpxchg() function as noinline and
not __init avoids the problem for me.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

---
 kernel/futex.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.20.0

Comments

Joe Perches March 7, 2019, 5:19 p.m. | #1
On Thu, 2019-03-07 at 10:14 +0100, Arnd Bergmann wrote:
> On 32-bit ARM, I got a link failure in futex_init() when building

> with clang in some random configurations:

> 

> kernel/futex.o:(.text.fixup+0x5c): relocation truncated to fit: R_ARM_JUMP24 against `.init.text'

> 

> As far as I can tell, the problem is that a branch is over 16MB

> apart in those configurations, but only if it branches back to

> the init text.

> 

> Marking the futex_detect_cmpxchg() function as noinline and

> not __init avoids the problem for me.


Perhaps the __init and __exit #defines should be noinline
to allow discarding of the code.

These defines are already marked with __section so I'd've
expected these to be noinline anyway.

---
 include/linux/init.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/init.h b/include/linux/init.h
index 5255069f5a9f..806215a74064 100644
--- a/include/linux/init.h
+++ b/include/linux/init.h
@@ -47,7 +47,7 @@
 
 /* These are for everybody (although not all archs will actually
    discard it in modules) */
-#define __init		__section(.init.text) __cold  __latent_entropy __noinitretpoline
+#define __init		__section(.init.text) __cold  __latent_entropy __noinitretpoline noinline
 #define __initdata	__section(.init.data)
 #define __initconst	__section(.init.rodata)
 #define __exitdata	__section(.exit.data)
@@ -80,7 +80,7 @@
 #define __exitused  __used
 #endif
 
-#define __exit          __section(.exit.text) __exitused __cold notrace
+#define __exit          __section(.exit.text) __exitused __cold notrace noinline
 
 /* Used for MEMORY_HOTPLUG */
 #define __meminit        __section(.meminit.text) __cold notrace \
Russell King - ARM Linux admin March 7, 2019, 5:25 p.m. | #2
On Thu, Mar 07, 2019 at 09:19:04AM -0800, Joe Perches wrote:
> On Thu, 2019-03-07 at 10:14 +0100, Arnd Bergmann wrote:

> > On 32-bit ARM, I got a link failure in futex_init() when building

> > with clang in some random configurations:

> > 

> > kernel/futex.o:(.text.fixup+0x5c): relocation truncated to fit: R_ARM_JUMP24 against `.init.text'

> > 

> > As far as I can tell, the problem is that a branch is over 16MB

> > apart in those configurations, but only if it branches back to

> > the init text.

> > 

> > Marking the futex_detect_cmpxchg() function as noinline and

> > not __init avoids the problem for me.

> 

> Perhaps the __init and __exit #defines should be noinline

> to allow discarding of the code.


How does that help this case?  The problem is futex_detect_cmpxchg()
being placed in .init.text which is too far from .text.fixup.
Whether it is inlined or not is immaterial since both
futex_detect_cmpxchg() and its only caller futex_init() are both __init
functions.

It seems to me to be completely sane to have:

static void __init foo(...)
{
}

static int __init foo_init(...)
{
	foo();
}

and have the expectation that the compiler _can_, if it desires, inline
foo() into foo_init().

Let me re-iterate: the inlining of futex_detect_cmpxchg() into
futex_init() is not the issue here.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
Joe Perches March 7, 2019, 5:42 p.m. | #3
On Thu, 2019-03-07 at 17:25 +0000, Russell King - ARM Linux admin wrote:
> On Thu, Mar 07, 2019 at 09:19:04AM -0800, Joe Perches wrote:

> > On Thu, 2019-03-07 at 10:14 +0100, Arnd Bergmann wrote:

> > > On 32-bit ARM, I got a link failure in futex_init() when building

> > > with clang in some random configurations:

> > > 

> > > kernel/futex.o:(.text.fixup+0x5c): relocation truncated to fit: R_ARM_JUMP24 against `.init.text'

> > > 

> > > As far as I can tell, the problem is that a branch is over 16MB

> > > apart in those configurations, but only if it branches back to

> > > the init text.

> > > 

> > > Marking the futex_detect_cmpxchg() function as noinline and

> > > not __init avoids the problem for me.

> > 

> > Perhaps the __init and __exit #defines should be noinline

> > to allow discarding of the code.

> 

> How does that help this case?


It doesn't particularly.
It does help any other case that might arise.

> It seems to me to be completely sane to have:

> 

> static void __init foo(...)

> {

> }

> 

> static int __init foo_init(...)

> {

> 	foo();

> }

> 

> and have the expectation that the compiler _can_, if it desires, inline

> foo() into foo_init().


True, but init function performance requirements are
generally trivial and isolating the possibility of
defects like this seems a reasonable trade-off to me.
Russell King - ARM Linux admin March 7, 2019, 6:07 p.m. | #4
On Thu, Mar 07, 2019 at 09:42:40AM -0800, Joe Perches wrote:
> On Thu, 2019-03-07 at 17:25 +0000, Russell King - ARM Linux admin wrote:

> > On Thu, Mar 07, 2019 at 09:19:04AM -0800, Joe Perches wrote:

> > > On Thu, 2019-03-07 at 10:14 +0100, Arnd Bergmann wrote:

> > > > On 32-bit ARM, I got a link failure in futex_init() when building

> > > > with clang in some random configurations:

> > > > 

> > > > kernel/futex.o:(.text.fixup+0x5c): relocation truncated to fit: R_ARM_JUMP24 against `.init.text'

> > > > 

> > > > As far as I can tell, the problem is that a branch is over 16MB

> > > > apart in those configurations, but only if it branches back to

> > > > the init text.

> > > > 

> > > > Marking the futex_detect_cmpxchg() function as noinline and

> > > > not __init avoids the problem for me.

> > > 

> > > Perhaps the __init and __exit #defines should be noinline

> > > to allow discarding of the code.

> > 

> > How does that help this case?

> 

> It doesn't particularly.

> It does help any other case that might arise.


Please describe what "any other case" you are thinking of - as already
stated, your patch would have no beneficial effect for the observed
issue.

In fact, it _could_ do exactly the opposite.  Where functions can be
inlined, they may become smaller due to no need for function prologue
and epilogue, and other optimisations.  Forcing the compiler to avoid
inlining them could result in larger code, which means a higher chance
of triggering the "relocation truncated to fit" problem.

The converse is also a possibility too - it all depends on the inlining
behaviour of the compiler towards static functions, and the resulting
size of the code.

What may be right for one architecture and compiler may not be right
for another.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
Nick Desaulniers March 7, 2019, 6:12 p.m. | #5
On Thu, Mar 7, 2019 at 1:15 AM Arnd Bergmann <arnd@arndb.de> wrote:
>

> On 32-bit ARM, I got a link failure in futex_init() when building

> with clang in some random configurations:

>

> kernel/futex.o:(.text.fixup+0x5c): relocation truncated to fit: R_ARM_JUMP24 against `.init.text'


Do we know what function from the fixup text section is calling
futex_detect_cmpxchg?  I'm curious if this is maybe another case of
-Wsection where some function may be in the wrong section?

>

> As far as I can tell, the problem is that a branch is over 16MB

> apart in those configurations, but only if it branches back to

> the init text.

>

> Marking the futex_detect_cmpxchg() function as noinline and

> not __init avoids the problem for me.

>

> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

> ---

>  kernel/futex.c | 2 +-

>  1 file changed, 1 insertion(+), 1 deletion(-)

>

> diff --git a/kernel/futex.c b/kernel/futex.c

> index c3b73b0311bc..dda77ed9f445 100644

> --- a/kernel/futex.c

> +++ b/kernel/futex.c

> @@ -3849,7 +3849,7 @@ SYSCALL_DEFINE6(futex_time32, u32 __user *, uaddr, int, op, u32, val,

>  }

>  #endif /* CONFIG_COMPAT_32BIT_TIME */

>

> -static void __init futex_detect_cmpxchg(void)

> +static noinline void futex_detect_cmpxchg(void)

>  {

>  #ifndef CONFIG_HAVE_FUTEX_CMPXCHG

>         u32 curval;

> --

> 2.20.0

>



-- 
Thanks,
~Nick Desaulniers
Nathan Chancellor March 7, 2019, 6:21 p.m. | #6
On Thu, Mar 07, 2019 at 10:12:11AM -0800, Nick Desaulniers wrote:
> On Thu, Mar 7, 2019 at 1:15 AM Arnd Bergmann <arnd@arndb.de> wrote:

> >

> > On 32-bit ARM, I got a link failure in futex_init() when building

> > with clang in some random configurations:

> >

> > kernel/futex.o:(.text.fixup+0x5c): relocation truncated to fit: R_ARM_JUMP24 against `.init.text'

> 

> Do we know what function from the fixup text section is calling

> futex_detect_cmpxchg?  I'm curious if this is maybe another case of

> -Wsection where some function may be in the wrong section?

> 


Looks like this is the call stack:

futex_init ->
    futex_detect_cmpxchg ->
        cmpxchg_futex_value_locked ->
            futex_atomic_cmpxchg_inatomic

This is the same issue I reported: https://github.com/ClangBuiltLinux/linux/issues/325

Marking arm's futex_atomic_cmpxchg_inatomic as noinline also fixes this
so maybe that's it?

Cheers,
Nathan

> >

> > As far as I can tell, the problem is that a branch is over 16MB

> > apart in those configurations, but only if it branches back to

> > the init text.

> >

> > Marking the futex_detect_cmpxchg() function as noinline and

> > not __init avoids the problem for me.

> >

> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>

> > ---

> >  kernel/futex.c | 2 +-

> >  1 file changed, 1 insertion(+), 1 deletion(-)

> >

> > diff --git a/kernel/futex.c b/kernel/futex.c

> > index c3b73b0311bc..dda77ed9f445 100644

> > --- a/kernel/futex.c

> > +++ b/kernel/futex.c

> > @@ -3849,7 +3849,7 @@ SYSCALL_DEFINE6(futex_time32, u32 __user *, uaddr, int, op, u32, val,

> >  }

> >  #endif /* CONFIG_COMPAT_32BIT_TIME */

> >

> > -static void __init futex_detect_cmpxchg(void)

> > +static noinline void futex_detect_cmpxchg(void)

> >  {

> >  #ifndef CONFIG_HAVE_FUTEX_CMPXCHG

> >         u32 curval;

> > --

> > 2.20.0

> >

> 

> 

> -- 

> Thanks,

> ~Nick Desaulniers
Arnd Bergmann March 7, 2019, 10:24 p.m. | #7
On Thu, Mar 7, 2019 at 7:21 PM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>

> On Thu, Mar 07, 2019 at 10:12:11AM -0800, Nick Desaulniers wrote:

> > On Thu, Mar 7, 2019 at 1:15 AM Arnd Bergmann <arnd@arndb.de> wrote:

> > >

> > > On 32-bit ARM, I got a link failure in futex_init() when building

> > > with clang in some random configurations:

> > >

> > > kernel/futex.o:(.text.fixup+0x5c): relocation truncated to fit: R_ARM_JUMP24 against `.init.text'

> >

> > Do we know what function from the fixup text section is calling

> > futex_detect_cmpxchg?  I'm curious if this is maybe another case of

> > -Wsection where some function may be in the wrong section?

> >

>

> Looks like this is the call stack:

>

> futex_init ->

>     futex_detect_cmpxchg ->

>         cmpxchg_futex_value_locked ->

>             futex_atomic_cmpxchg_inatomic

>

> This is the same issue I reported: https://github.com/ClangBuiltLinux/linux/issues/325

>

> Marking arm's futex_atomic_cmpxchg_inatomic as noinline also fixes this

> so maybe that's it?


I think part of the problem is the linker sometimes failing to generate
veneers for long calls. I've seen this in the past with relocations in
non-executable sections, but it's possible that the problem here is
having a relocation to a section that doesn't start with ".text".

I have not analyzed the linker behavior in more detail, but have
experimentally shown that the problem doesn't happen if the relocation
is from the .text.fixup segment to .text rather than .init.text. It's
also possible that this is simply a result of the relative distance of
the locations in memory as Russell said, so it could come back
if the kernel grows further.

        Arnd

Patch

diff --git a/kernel/futex.c b/kernel/futex.c
index c3b73b0311bc..dda77ed9f445 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -3849,7 +3849,7 @@  SYSCALL_DEFINE6(futex_time32, u32 __user *, uaddr, int, op, u32, val,
 }
 #endif /* CONFIG_COMPAT_32BIT_TIME */
 
-static void __init futex_detect_cmpxchg(void)
+static noinline void futex_detect_cmpxchg(void)
 {
 #ifndef CONFIG_HAVE_FUTEX_CMPXCHG
 	u32 curval;