Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1732322 > unrolled thread

Re: [RFC PATCH 3/4] x86/asm: Make alternative macro interfaces more clear and consistent

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC PATCH 3/4] x86/asm: Make alternative macro interfaces more  clear and consistent Josh Poimboeuf <jpoimboe@redhat.com> - 2017-09-14 16:50 +0200
    Re: [RFC PATCH 3/4] x86/asm: Make alternative macro interfaces more  clear and consistent Linus Torvalds <torvalds@linux-foundation.org> - 2017-09-14 19:20 +0200
      Re: [RFC PATCH 3/4] x86/asm: Make alternative macro interfaces more  clear and consistent Josh Poimboeuf <jpoimboe@redhat.com> - 2017-09-14 19:30 +0200
        Re: [RFC PATCH 3/4] x86/asm: Make alternative macro interfaces more  clear and consistent Josh Poimboeuf <jpoimboe@redhat.com> - 2017-09-14 19:40 +0200
          Re: [RFC PATCH 3/4] x86/asm: Make alternative macro interfaces more  clear and consistent Linus Torvalds <torvalds@linux-foundation.org> - 2017-09-14 20:30 +0200
            Re: [RFC PATCH 3/4] x86/asm: Make alternative macro interfaces more  clear and consistent Josh Poimboeuf <jpoimboe@redhat.com> - 2017-09-14 20:50 +0200
              Re: [RFC PATCH 3/4] x86/asm: Make alternative macro interfaces more  clear and consistent Josh Poimboeuf <jpoimboe@redhat.com> - 2017-09-15 18:20 +0200

#1732322 — Re: [RFC PATCH 3/4] x86/asm: Make alternative macro interfaces more clear and consistent

On Sat, Sep 02, 2017 at 12:32:21PM +0200, Ingo Molnar wrote:
> 
> * Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> 
> > On Thu, Aug 31, 2017 at 12:25:42PM -0500, Josh Poimboeuf wrote:
> > > 2) Put "sp" in the clobbers list instead of as an i/o constraint.  This
> > >    mostly works for GCC, and doesn't break clang.  However, it causes
> > >    GCC to insert a "lea -0x10(%rbp),%rsp" in the epilogue of every
> > >    affected function.
> > 
> > And maybe this extra instruction is negligible for performance and not a
> > big deal?  I might look at this one after the holiday too.
> 
> Please do statistics of how many functions are affected, on a defconfig-ish 
> kernel.

As it turns out, the real problem with this option is that it imposes a
penalty for CONFIG_FRAME_POINTER=n: even with frame pointers disabled,
it forces the frame pointer to be saved for each function which uses the
inline asm "call" statements.  Our current solution doesn't do that.

- On a defconfig-based kernel, this adds +6k of .text (+0.06%).

- On a Fedora distro-based config, it adds +27k of .text (+0.3%).
  (I think the difference from defconfig is mostly caused by
  CONFIG_PARAVIRT.)

I'll try a few more experiments, but I'll probably end up engaging the
compiler people as Linus suggested.

-- 
Josh

[toc] | [next] | [standalone]

#1732471

On Thu, Sep 14, 2017 at 7:48 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> As it turns out, the real problem with this option is that it imposes a
> penalty for CONFIG_FRAME_POINTER=n: even with frame pointers disabled,
> it forces the frame pointer to be saved for each function which uses the
> inline asm "call" statements.  Our current solution doesn't do that.

But couldn't we make the whole stack pointer clobber be dependent on
CONFIG_FRAME_POINTER?

The only reason we do it is to make sure the frame pointer is set up
before the inline asm is emitted, but with frame pointers disabled we
don't need to.

Or was there some other compiler issue?

But yes, talking to the compiler people is a good idea anyway. Both
the clang ones (marking rsp as an in/out register *really* shouldn't
cause them to generate wrong code) and the gcc people (to see if there
are other alternatives).

                      Linus

[toc] | [prev] | [next] | [standalone]

#1732476

On Thu, Sep 14, 2017 at 10:16:08AM -0700, Linus Torvalds wrote:
> On Thu, Sep 14, 2017 at 7:48 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >
> > As it turns out, the real problem with this option is that it imposes a
> > penalty for CONFIG_FRAME_POINTER=n: even with frame pointers disabled,
> > it forces the frame pointer to be saved for each function which uses the
> > inline asm "call" statements.  Our current solution doesn't do that.
> 
> But couldn't we make the whole stack pointer clobber be dependent on
> CONFIG_FRAME_POINTER?
> 
> The only reason we do it is to make sure the frame pointer is set up
> before the inline asm is emitted, but with frame pointers disabled we
> don't need to.

We could, but then that would mean either:

 a) uglifying the 15 or so relevant inline asm locations with ifdefs; or

 b) using my ASM_CALL macro, which I think you frowned upon?

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1732482

On Thu, Sep 14, 2017 at 12:26:27PM -0500, Josh Poimboeuf wrote:
> On Thu, Sep 14, 2017 at 10:16:08AM -0700, Linus Torvalds wrote:
> > On Thu, Sep 14, 2017 at 7:48 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > >
> > > As it turns out, the real problem with this option is that it imposes a
> > > penalty for CONFIG_FRAME_POINTER=n: even with frame pointers disabled,
> > > it forces the frame pointer to be saved for each function which uses the
> > > inline asm "call" statements.  Our current solution doesn't do that.
> > 
> > But couldn't we make the whole stack pointer clobber be dependent on
> > CONFIG_FRAME_POINTER?
> > 
> > The only reason we do it is to make sure the frame pointer is set up
> > before the inline asm is emitted, but with frame pointers disabled we
> > don't need to.
> 
> We could, but then that would mean either:
> 
>  a) uglifying the 15 or so relevant inline asm locations with ifdefs; or

Actually I guess we could put the "sp" in a macro...  I'll try it.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1732497

On Thu, Sep 14, 2017 at 10:33 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>>
>>  a) uglifying the 15 or so relevant inline asm locations with ifdefs; or
>
> Actually I guess we could put the "sp" in a macro...  I'll try it.

Exactly. Do something like

   #ifdef CONFIG_FRAME_POINTER
   # define EXTRA_ASM_CLOBBERS "rsp"
   #else
   # define EXTRA_ASM_CLOBBERS
   #endif

and then replace the nasty

        register void *__sp asm(_ASM_SP);
        ..
        "+r" (__sp)

games with just that EXTRA_ASM_CLOBBERS thing at the end of the clobbers.

Yes, you'd probably have to document that the alternative_call_2()
thing doesn't take a "input" argument, but a input_and_clobbers, but
all users do that anyway.

I dunno.

               Linus

[toc] | [prev] | [next] | [standalone]

#1732511

On Thu, Sep 14, 2017 at 11:28:30AM -0700, Linus Torvalds wrote:
> On Thu, Sep 14, 2017 at 10:33 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >>
> >>  a) uglifying the 15 or so relevant inline asm locations with ifdefs; or
> >
> > Actually I guess we could put the "sp" in a macro...  I'll try it.
> 
> Exactly. Do something like
> 
>    #ifdef CONFIG_FRAME_POINTER
>    # define EXTRA_ASM_CLOBBERS "rsp"
>    #else
>    # define EXTRA_ASM_CLOBBERS
>    #endif
> 
> and then replace the nasty
> 
>         register void *__sp asm(_ASM_SP);
>         ..
>         "+r" (__sp)
> 
> games with just that EXTRA_ASM_CLOBBERS thing at the end of the clobbers.
> 
> Yes, you'd probably have to document that the alternative_call_2()
> thing doesn't take a "input" argument, but a input_and_clobbers, but
> all users do that anyway.
> 
> I dunno.

There's also alternative_call(), which doesn't yet have the '__rsp'
annotation, but it probably should.  It has some callers which pass
clobbers and some which don't, so its conversion would be trickier.

So my plan is to keep patch 3 of this series, which clarifies those
alternative macro interfaces, and also separates the inputs from the
clobbers.  That'll make it really easy to add something like
EXTRA_ASM_CLOBBERS above.

In fact I'll probably keep patches 1-3, because they're all
improvements.  Then I'll replace the original patch 4 (ASM_CALL) with
the "sp" clobbers thing.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1732940

On Thu, Sep 14, 2017 at 01:45:29PM -0500, Josh Poimboeuf wrote:
> On Thu, Sep 14, 2017 at 11:28:30AM -0700, Linus Torvalds wrote:
> > On Thu, Sep 14, 2017 at 10:33 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > >>
> > >>  a) uglifying the 15 or so relevant inline asm locations with ifdefs; or
> > >
> > > Actually I guess we could put the "sp" in a macro...  I'll try it.
> > 
> > Exactly. Do something like
> > 
> >    #ifdef CONFIG_FRAME_POINTER
> >    # define EXTRA_ASM_CLOBBERS "rsp"
> >    #else
> >    # define EXTRA_ASM_CLOBBERS
> >    #endif
> > 
> > and then replace the nasty
> > 
> >         register void *__sp asm(_ASM_SP);
> >         ..
> >         "+r" (__sp)
> > 
> > games with just that EXTRA_ASM_CLOBBERS thing at the end of the clobbers.
> > 
> > Yes, you'd probably have to document that the alternative_call_2()
> > thing doesn't take a "input" argument, but a input_and_clobbers, but
> > all users do that anyway.
> > 
> > I dunno.
> 
> There's also alternative_call(), which doesn't yet have the '__rsp'
> annotation, but it probably should.  It has some callers which pass
> clobbers and some which don't, so its conversion would be trickier.
> 
> So my plan is to keep patch 3 of this series, which clarifies those
> alternative macro interfaces, and also separates the inputs from the
> clobbers.  That'll make it really easy to add something like
> EXTRA_ASM_CLOBBERS above.
> 
> In fact I'll probably keep patches 1-3, because they're all
> improvements.  Then I'll replace the original patch 4 (ASM_CALL) with
> the "sp" clobbers thing.

So I couldn't figure out how to make it any simpler than this:

#ifdef CONFIG_FRAME_POINTER
# define ASM_CALL_CLOBBERS "sp"
# define ASM_CALL_CLOBBERS_APPEND , ASM_CALL_CLOBBERS
# define ASM_CALL_CLOBBERS_ARGS(args...) ASM_CALL_CLOBBERS, ## args
#else
# define ASM_CALL_CLOBBERS
# define ASM_CALL_CLOBBERS_APPEND
# define ASM_CALL_CLOBBERS_ARGS(args...) args
#endif


ASM_CALL_CLOBBERS is the normal one:

  asm volatile("call foo" : : : ASM_CALL_CLOBBERS);


ASM_CALL_CLOBBERS_APPEND is needed when combining the option with other
clobbers options, like:

  asm volatile("call foo" : : : "memory" ASM_CALL_CLOBBERS_APPEND);


ASM_CALL_CLOBBERS_ARGS is needed for the pesky alternative_call() macro
so it can work with the variadic argument:

  #define alternative_call(oldfunc, newfunc, feature, outputs, inputs,	\
  			 clobbers...)					\
  	asm volatile (ALTERNATIVE("call %P[old]", "call %P[new]",	\
  				  feature),				\
  		      : outputs						\
  		      : [old] "i" (oldfunc), [new] "i" (newfunc)	\
  		        ARGS_APPEND(inputs)				\
  		      : ASM_CALL_CLOBBERS_ARGS(clobbers))


So I *was* about ready to post something like the above.  But, of
course, the kbuild robot found that the new version of my patches
manages to crash GCC with a certain randconfig.

  https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82221

So the saga continues.  Now I've managed to break clang, sparse, and GCC
with each consecutive iteration of these patches...  I'm starting to
lose my faith in compilers.

-- 
Josh

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web