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

Groups > linux.kernel > #1428834 > unrolled thread

[PATCH] Disable non-ABI-compliant optimisations for live patching

Back to article view | Back to linux.kernel

Contents

  [PATCH] Disable non-ABI-compliant optimisations for live patching Torsten Duwe <duwe@lst.de> - 2016-06-22 16:30 +0200
    Re: [PATCH] Disable non-ABI-compliant optimisations for live patching Josh Poimboeuf <jpoimboe@redhat.com> - 2016-06-22 17:20 +0200
      Re: [PATCH] Disable non-ABI-compliant optimisations for live  patching Miroslav Benes <mbenes@suse.cz> - 2016-06-23 09:50 +0200
        Re: [PATCH] Disable non-ABI-compliant optimisations for live  patching Torsten Duwe <duwe@lst.de> - 2016-06-23 12:10 +0200
          Re: [PATCH] Disable non-ABI-compliant optimisations for live  patching Jiri Kosina <jikos@kernel.org> - 2016-06-23 12:50 +0200
            Re: [PATCH] Disable non-ABI-compliant optimisations for live  patching Jiri Kosina <jikos@kernel.org> - 2016-06-23 14:50 +0200

#1428834 — [PATCH] Disable non-ABI-compliant optimisations for live patching

Live patching, as we use it, deliberately disrupts the fabric of
compile units; thus all assumptions a compiler can make about the
control flow may be invalid. As an example, it could analyse that a
callee does not touch a caller-saved register at all, so why waste
memory bandwidth saving it? The register allocations for the live
patch replacement function may however be quite different.

Starting with this example, disable all compiler optimisations that
do not strictly comply with the established calling conventions.

Signed-off-by: Torsten Duwe <duwe@suse.de>
---

Working on the arm64 ftrace-with-regs/livepatch, it struck me that
this is a general problem: with live patching, certain optimisations
must be switched off for all architectures, the new(?) IPA register
allocator in gcc6 is only one example. We should tackle this
well before it bites us.

	Torsten

---
 Makefile | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Makefile b/Makefile
index b409076..424d2e6 100644
--- a/Makefile
+++ b/Makefile
@@ -743,6 +743,13 @@ KBUILD_CFLAGS 	+= $(call cc-option, -femit-struct-debug-baseonly) \
 		   $(call cc-option,-fno-var-tracking)
 endif
 
+ifdef CONFIG_LIVEPATCH
+# The compiler might generate ABI "shortcuts" to speed up the code,
+# making assumptions which are no longer valid when live patching
+# is enabled. Disable all of them.
+KBUILD_CFLAGS	+= $(call cc-option,-fno-ipa-ra)
+endif
+
 ifdef CONFIG_FUNCTION_TRACER
 ifndef CC_FLAGS_FTRACE
 CC_FLAGS_FTRACE := -pg
-- 
2.6.6

[toc] | [next] | [standalone]

#1428887

On Wed, Jun 22, 2016 at 04:24:41PM +0200, Torsten Duwe wrote:
> Live patching, as we use it, deliberately disrupts the fabric of
> compile units; thus all assumptions a compiler can make about the
> control flow may be invalid. As an example, it could analyse that a
> callee does not touch a caller-saved register at all, so why waste
> memory bandwidth saving it? The register allocations for the live
> patch replacement function may however be quite different.
> 
> Starting with this example, disable all compiler optimisations that
> do not strictly comply with the established calling conventions.
> 
> Signed-off-by: Torsten Duwe <duwe@suse.de>
> ---
> 
> Working on the arm64 ftrace-with-regs/livepatch, it struck me that
> this is a general problem: with live patching, certain optimisations
> must be switched off for all architectures, the new(?) IPA register
> allocator in gcc6 is only one example. We should tackle this
> well before it bites us.
> 
> 	Torsten

I think this is a good idea.  While we're at it, should we also disable
some of the other IPA options?  These sound especially problematic:

-fipa-sra
   Perform interprocedural scalar replacement of aggregates, removal of
   unused parameters and replacement of parameters passed by reference
   by parameters passed by value.

-fipa-cp
   Perform interprocedural constant propagation.  This optimization
   analyzes the program to determine when values passed to functions are
   constants and then optimizes accordingly.  This optimization can
   substantially increase performance if the application has constants
   passed to functions.

-fipa-icf
   Perform Identical Code Folding for functions and read-only variables.
   The optimization reduces code size and may disturb unwind stacks by
   replacing a function by equivalent one with a different name. The
   optimization works more effectively with link time optimization
   enabled.


> 
> ---
>  Makefile | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/Makefile b/Makefile
> index b409076..424d2e6 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -743,6 +743,13 @@ KBUILD_CFLAGS 	+= $(call cc-option, -femit-struct-debug-baseonly) \
>  		   $(call cc-option,-fno-var-tracking)
>  endif
>  
> +ifdef CONFIG_LIVEPATCH
> +# The compiler might generate ABI "shortcuts" to speed up the code,
> +# making assumptions which are no longer valid when live patching
> +# is enabled. Disable all of them.
> +KBUILD_CFLAGS	+= $(call cc-option,-fno-ipa-ra)
> +endif
> +
>  ifdef CONFIG_FUNCTION_TRACER
>  ifndef CC_FLAGS_FTRACE
>  CC_FLAGS_FTRACE := -pg
> -- 
> 2.6.6
> 

-- 
Josh

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

#1429535 — Re: [PATCH] Disable non-ABI-compliant optimisations for live patching

Hi,

On Wed, 22 Jun 2016, Josh Poimboeuf wrote:

> On Wed, Jun 22, 2016 at 04:24:41PM +0200, Torsten Duwe wrote:
> > Live patching, as we use it, deliberately disrupts the fabric of
> > compile units; thus all assumptions a compiler can make about the
> > control flow may be invalid. As an example, it could analyse that a
> > callee does not touch a caller-saved register at all, so why waste
> > memory bandwidth saving it? The register allocations for the live
> > patch replacement function may however be quite different.

But this exact situation should not be possible. Ftrace stub should (and 
it does on x86_64) save the caller-saved registers for us. Otherwise we 
would have seen problems with kGraft already.

> > Starting with this example, disable all compiler optimisations that
> > do not strictly comply with the established calling conventions.

It think it is too rough and I'd like to avoid it if possible.

> > Signed-off-by: Torsten Duwe <duwe@suse.de>
> > ---
> > 
> > Working on the arm64 ftrace-with-regs/livepatch, it struck me that
> > this is a general problem: with live patching, certain optimisations
> > must be switched off for all architectures, the new(?) IPA register
> > allocator in gcc6 is only one example. We should tackle this
> > well before it bites us.
> > 
> > 	Torsten
> 
> I think this is a good idea.  While we're at it, should we also disable
> some of the other IPA options?  These sound especially problematic:
> 
> -fipa-sra
>    Perform interprocedural scalar replacement of aggregates, removal of
>    unused parameters and replacement of parameters passed by reference
>    by parameters passed by value.

Yes, this changes ABI. There are generally two different situations we 
need to solve in kGraft.

1. A to-be-patched function is isra-optimized, then its caller functions 
are patched.

2. As we don't use relocations in kGraft yet we need to call kallsyms on 
every unexported function. If such function is isra-optimized it is easier 
and safer to put it in the very kGraft patch.

This could surely be avoided if one can prove that the new patching 
function is optimized in the same way by gcc.

> -fipa-cp
>    Perform interprocedural constant propagation.  This optimization
>    analyzes the program to determine when values passed to functions are
>    constants and then optimizes accordingly.  This optimization can
>    substantially increase performance if the application has constants
>    passed to functions.

This is the same situation as isra, I think.

> -fipa-icf
>    Perform Identical Code Folding for functions and read-only variables.
>    The optimization reduces code size and may disturb unwind stacks by
>    replacing a function by equivalent one with a different name. The
>    optimization works more effectively with link time optimization
>    enabled.

I haven't met this one yet since we don't have it enabled in SLES as of 
now. Could be problem...

Regards,
Miroslav
 
> > 
> > ---
> >  Makefile | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/Makefile b/Makefile
> > index b409076..424d2e6 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -743,6 +743,13 @@ KBUILD_CFLAGS 	+= $(call cc-option, -femit-struct-debug-baseonly) \
> >  		   $(call cc-option,-fno-var-tracking)
> >  endif
> >  
> > +ifdef CONFIG_LIVEPATCH
> > +# The compiler might generate ABI "shortcuts" to speed up the code,
> > +# making assumptions which are no longer valid when live patching
> > +# is enabled. Disable all of them.
> > +KBUILD_CFLAGS	+= $(call cc-option,-fno-ipa-ra)
> > +endif
> > +
> >  ifdef CONFIG_FUNCTION_TRACER
> >  ifndef CC_FLAGS_FTRACE
> >  CC_FLAGS_FTRACE := -pg
> > -- 
> > 2.6.6
> > 
> 
> -- 
> Josh
> --
> To unsubscribe from this list: send the line "unsubscribe live-patching" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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

#1429655 — Re: [PATCH] Disable non-ABI-compliant optimisations for live patching

On Thu, Jun 23, 2016 at 09:45:48AM +0200, Miroslav Benes wrote:
> 
> Hi,
> 
> On Wed, 22 Jun 2016, Josh Poimboeuf wrote:
> 
> > On Wed, Jun 22, 2016 at 04:24:41PM +0200, Torsten Duwe wrote:
> > > Live patching, as we use it, deliberately disrupts the fabric of
> > > compile units; thus all assumptions a compiler can make about the
> > > control flow may be invalid. As an example, it could analyse that a
> > > callee does not touch a caller-saved register at all, so why waste
> > > memory bandwidth saving it? The register allocations for the live
> > > patch replacement function may however be quite different.
> 
> But this exact situation should not be possible. Ftrace stub should (and 
> it does on x86_64) save the caller-saved registers for us. Otherwise we 

I haven't looked at the fentry solution, but the code I'm involved in saves
the registers so that ftrace, live patch and friends can work freely. But
then it restores all regs and _then_ calls the replacement, so ftrace
saving all regs is no gain at all.

> would have seen problems with kGraft already.

IMHO these problems are rare to trigger. I'm afraid some problems are already
lurking.

> > > Starting with this example, disable all compiler optimisations that
> > > do not strictly comply with the established calling conventions.
> 
> It think it is too rough and I'd like to avoid it if possible.

I'm not too happy either, but function calling conventions are there
for a reason. And if you exchange one function with another version,
the convention is all you can rely on.

[ ... slightly out of context ...:]
> 
> This could surely be avoided if one can prove that the new patching 
> function is optimized in the same way by gcc.

I guess that is what it boils down to in general, and I'd rather avoid that,
as it sometimes requires the whole compile unit to get the same result; and
then we haven't even made the changes we wanted to live patch in the first
place.

My main intention was to create the awareness and offer one possible
solution. I'd be happy if there was a better way.

	Torsten

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

#1429670 — Re: [PATCH] Disable non-ABI-compliant optimisations for live patching

On Thu, 23 Jun 2016, Torsten Duwe wrote:

> I haven't looked at the fentry solution, but the code I'm involved in saves
> the registers so that ftrace, live patch and friends can work freely. But
> then it restores all regs and _then_ calls the replacement, so ftrace
> saving all regs is no gain at all.

You're right, thanks for bringing this up.

In principle we should be able to modify the trampoline so that it 
performs its own register saving (in ftrace_regs_caller) and restoring 
(*), completely shielding the new function from any optimization gcc might 
have done on registers, shouldn't we?

(*) we'll have to piggy-back on ftrace_epilogue on that, i.e. making the 
    return to the original code go through trampoline as well (the same 
    way graph tracer works)

Thanks,

-- 
Jiri Kosina
SUSE Labs

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

#1429781 — Re: [PATCH] Disable non-ABI-compliant optimisations for live patching

On Thu, 23 Jun 2016, Jiri Kosina wrote:

> > I haven't looked at the fentry solution, but the code I'm involved in saves
> > the registers so that ftrace, live patch and friends can work freely. But
> > then it restores all regs and _then_ calls the replacement, so ftrace
> > saving all regs is no gain at all.
> 
> You're right, thanks for bringing this up.
> 
> In principle we should be able to modify the trampoline so that it 
> performs its own register saving (in ftrace_regs_caller) and restoring 
> (*), completely shielding the new function from any optimization gcc might 
> have done on registers, shouldn't we?
> 
> (*) we'll have to piggy-back on ftrace_epilogue on that, i.e. making the 
>     return to the original code go through trampoline as well (the same 
>     way graph tracer works)

Okay, after looking more about how ftrace implements the return 
trampolines for graph caller, it'd be rather difficult to implement in a 
way that we neither interfere with ftrace graph tracer (the 
ftrace_ret_stack in task_struct) nor introduce a serious performance 
overhead or stack usage pressure.

I am pretty sure the overhead we'd be adding would be much worse than just 
really simply turning the IPA-RA off in CONFIG_LIVEPATCH-enabled kernels 
is the easiest way to go.

After talking to Jan Hubicka, I'd actually suggest turning off most/all 
the IPA optimizations; they are supposed to be of questionable benefit for 
kernel anyway, and they might be causing serious issues for us.

I am planning to ask our performance team to measure the impact this'd 
have.

Thanks,

-- 
Jiri Kosina
SUSE Labs

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web