Disable non-ABI-compliant optimisations for live patching
PATCH 1/1 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.
---
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(+)
Makefile +7 -0
@@ -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
Signature · 1 line
2.6.6
Quoted 13 lines · Torsten Duwe
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 <[email protected]>---
Quoted 7 lines
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.
Quoted 26 lines
--- Makefile | 7 +++++++ 1 file changed, 7 insertions(+)Makefile +7
-0@@ -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 := -pgSignature · 1 line
2.6.6
Signature · 1 line
Josh
Hi,
Quoted 7 lines · Josh Poimboeuf
Quoted 6 lines · Torsten Duwe
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.
Quoted 2 lines
Quoted 2 lines
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.
Quoted 11 lines
Quoted 10 lines
Signed-off-by Torsten Duwe <[email protected]>---
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
Quoted 7 lines
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.
Quoted 6 lines
-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.
Quoted 6 lines
-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
Quoted 34 lines
Quoted 26 lines
--- Makefile | 7 +++++++ 1 file changed, 7 insertions(+)Makefile +7
-0@@ -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 := -pgSignature · 1 line
2.6.6Signature · 5 lines
Josh -- To unsubscribe from this list: send the line "unsubscribe live-patching" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 13 lines · Miroslav Benes
Hi,
Quoted 7 lines · Josh Poimboeuf
Quoted 6 lines · Torsten Duwe
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.
Quoted 2 lines
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.
Quoted 1 line
would have seen problems with kGraft already.
IMHO these problems are rare to trigger. I'm afraid some problems are already lurking.
Quoted 4 lines
Quoted 2 lines
Quoted 2 lines
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 ...:]
Quoted 3 lines
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
Quoted 4 lines · Torsten Duwe
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,
Signature · 2 lines
Jiri Kosina SUSE Labs
Quoted 7 lines · Jiri Kosina
Quoted 4 lines
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.
Quoted 8 lines
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_epilogueon 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,
Signature · 2 lines
Jiri Kosina SUSE Labs
Quoted 24 lines · Jiri Kosina
Quoted 7 lines · Jiri Kosina
Quoted 4 lines
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.
Quoted 8 lines
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_epilogueon 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_stackintask_struct) nor introduce a serious performance overhead or stack usage pressure.
Quoted 7 lines
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.
Lets compile kernel with -O0, and sacrifice few lambs....?
Would it be possible to document which kind of guarantees live patching needs from compiler?
I always assumed that whoever is preparing the patch does manual investigation to see what needs to be changed and how, but apparantly that's not the case, so documentation would be good.
Best regards,
Pavel
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Quoted 2 lines · Pavel Machek
Would it be possible to document which kind of guarantees live patching needs from compiler?
Sure, here you go, all required guarantees:
Stick to the ABI.
Quoted 3 lines
I always assumed that whoever is preparing the patch does manual investigation to see what needs to be changed and how, but apparantly that's not the case, so documentation would be good.
See above for the documentation. Imagine each architecture had a whole hand-optimised assembler kernel. That would give some performance improvement, right? Same issue here, only much smaller. It's ultimate performance vs. maintainability.
HTH,
Torsten
Quoted 9 lines · Torsten Duwe
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.
I thought that in such case, person creating the live patch should notice and adjust patch appropriately, at assembly level if neccessary..?
If this is not true, and we want gcc to help us, what other optimalizations do we need to disable? Even changes inside one compiler unit can be "interesting"...
Pavel
Quoted 33 lines
Signed-off-by Torsten Duwe <[email protected]>---
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(+)Makefile +7
-0@@ -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
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Quoted 7 lines · Pavel Machek
Quoted 7 lines
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.
Quoted 6 lines
Quoted 2 lines
Starting with this example, disable all compiler optimisations that do not strictly comply with the established calling conventions.
I thought that in such case, person creating the live patch should notice and adjust patch appropriately, at assembly level if neccessary..?
Yes, that still holds; a lot of things could be automated though, and creating the automation tools is one of the big TODO items.
Quoted 3 lines
If this is not true, and we want gcc to help us, what other optimalizations do we need to disable? Even changes inside one compiler unit can be "interesting"...
What would actually be helpful is gcc providing us with a list of functions where it performed this ABI-violating optimization (similarly, we're already obtaining list of "what got inlined where"). Unfortunately, -fdump-ipa-ra is currently missing; I'm talking to gcc guys now to have it implemented.
Signature · 2 lines
Jiri Kosina SUSE Labs
Quoted 12 lines · Jiri Kosina
Quoted 10 lines · Pavel Machek
Quoted 9 lines
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.
Quoted 6 lines
Quoted 3 lines
I thought that in such case, person creating the live patch should notice and adjust patch appropriately, at assembly level if neccessary..?
Yes, that still holds; a lot of things could be automated though, and creating the automation tools is one of the big TODO items.
So the patch is not a bugfix, it is just something that slows down kernel to make stuff easier for the person doing the live patching...?
Quoted 9 lines
Quoted 3 lines
If this is not true, and we want gcc to help us, what other optimalizations do we need to disable? Even changes inside one compiler unit can be "interesting"...
What would actually be helpful is gcc providing us with a list of functions where it performed this ABI-violating optimization (similarly, we're already obtaining list of "what got inlined where"). Unfortunately, -fdump-ipa-ra is currently missing; I'm talking to gcc guys now to have it implemented.
What you actually want is "whenever source of function A influenced code in function B, I want to be notified", right?
If gcc can eliminate an if() brach in function B, because it can tell reading function A it can not happen, you need to know. Maybe that's limited to ABI today, but...
Pavel
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Quoted 9 lines · Pavel Machek
Quoted 6 lines
Quoted 3 lines
I thought that in such case, person creating the live patch should notice and adjust patch appropriately, at assembly level if neccessary..?
Yes, that still holds; a lot of things could be automated though, and creating the automation tools is one of the big TODO items.
So the patch is not a bugfix, it is just something that slows down kernel to make stuff easier for the person doing the live patching...?
Well, up to the last week noone realized the implications IPA-RA has for live patches. Now that we know about this, we have to deal with it somehow; as currently gcc doesn't provide easy way for us to obtain the information (non-existing -fdump-ipa-ra), disabling the optimization on CONFIG_LIVEPATCH-enabled kernels is a sensible workaround before we're able to get the information from gcc.
Quoted 6 lines
What you actually want is "whenever source of function A influenced code in function B, I want to be notified", right?
If gcc can eliminate an
if()brach in function B, because it can tell reading function A it can not happen, you need to know. Maybe that's limited to ABI today, but...
Yeah; dead code elimination is also a thing to watch for.
Signature · 2 lines
Jiri Kosina SUSE Labs
Quoted 12 lines · Jiri Kosina
Quoted 9 lines · Pavel Machek
Quoted 6 lines
Quoted 3 lines
I thought that in such case, person creating the live patch should notice and adjust patch appropriately, at assembly level if neccessary..?
Yes, that still holds; a lot of things could be automated though, and creating the automation tools is one of the big TODO items.
So the patch is not a bugfix, it is just something that slows down kernel to make stuff easier for the person doing the live patching...?
Quoted 6 lines
Well, up to the last week noone realized the implications IPA-RA has for live patches. Now that we know about this, we have to deal with it somehow; as currently gcc doesn't provide easy way for us to obtain the information (non-existing -fdump-ipa-ra), disabling the optimization on CONFIG_LIVEPATCH-enabled kernels is a sensible workaround before we're able to get the information from gcc.
You can still build the whole kernel with the patch applied, and look for code differences in all the functions, then analyzing them... no?
Quoted 8 lines
Quoted 6 lines
What you actually want is "whenever source of function A influenced code in function B, I want to be notified", right?
If gcc can eliminate an
if()brach in function B, because it can tell reading function A it can not happen, you need to know. Maybe that's limited to ABI today, but...Yeah; dead code elimination is also a thing to watch for.
That was supposed to be just an example.
I believe you want "whenever source of function A influenced code in function B, I want to be notified", and I believe it should be documented as such.
gcc might produce new and interesting optimalizations in future. I believe you want --dont-let-function-influence-function switch to gcc, not growing list of --no-optimalization-A, --no-optimalization-B...
Thanks,
Pavel
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Quoted 3 lines · Pavel Machek
I believe you want "whenever source of function A influenced code in function B, I want to be notified", and I believe it should be documented as such.
Well, exactly. IPA is a group of optimizations that are, by definition, intra-prodcedural.
Signature · 2 lines
Jiri Kosina SUSE Labs