Immedate Values Optimized Jump Fix

PATCH In review Mathieu Desnoyers 3 participants 10 messages Jul 17, 2008 – Jul 19, 2008 x86

lore

Share & tools

lore

Mathieu Desnoyers

author

2008-07-17 03:17 UTC #0 lore raw

PATCH 1/1 Immedate Values Optimized Jump Fix

Hi Ingo,

This fix would apply to the ftrace tree which contains the jump-based immediate values. I don't know if this tree is still under active development, but I guess that it wouldn't hurt to fix it.

Caused a kernel OOPS when loading the tracepoint probes once in a while on a 8-way x86_64.

Immedate Values Optimized Jump Fix

Fix the immediate values optimized jump fallback, which parameters were wrong following the last changes. It should be a 5 bytes instruction (not 2) with a 4 bytes operand.

Signed-off-by Mathieu Desnoyers <[email protected]>

---
 arch/x86/kernel/immediate.c |    2 +-
 include/asm-x86/immediate.h |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

arch/x86/kernel/immediate.c +1 -1

Index: linux-2.6-lttng/arch/x86/kernel/immediate.c
===================================================================
@@ -500,7 +500,7 @@ __kprobes int arch_imv_update(struct __i
 				"Jump target fallback at %lX, nr fail %d\n",
 				imv->imv, ++nr_fail);
 #endif
-			imv->size = 1;
+			imv->size = 4;	/* Fallback on movl */
 		} else {
 #ifdef DEBUG_IMMEDIATE
 			static int nr_success;

include/asm-x86/immediate.h +1 -1

Index: linux-2.6-lttng/include/asm-x86/immediate.h
===================================================================
@@ -132,7 +132,7 @@ struct __imv {
 		BUILD_BUG_ON(sizeof(__typeof__(name##__imv)) > 1);	\
 		asm (".section __imv,\"aw\",@progbits\n\t"		\
 			_ASM_PTR "%c1, (3f)-4\n\t"			\
-			".byte 0, 2\n\t"				\
+			".byte 0, 5\n\t"				\
 			".previous\n\t"					\
 			"mov $0,%0\n\t"					\
 			"3:\n\t"					\

Signature · 6 lines

Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Ingo Molnar

2008-07-18 16:15 UTC #1 lore raw

Quoted 8 lines · Mathieu Desnoyers

Hi Ingo,
This fix would apply to the ftrace tree which contains the jump-based immediate values. I don't know if this tree is still under active development, but I guess that it wouldn't hurt to fix it.
Caused a kernel OOPS when loading the tracepoint probes once in a while on a 8-way x86_64.

We've still got tip/tracing/immediates, with these commits from you:

 $ git-log-line linus.. arch/x86/kernel/immediate.c
 ee563d6: immediate values: jump liveliness
 e26875a: Immediate Values - Jump
 3fc8d03: Immediate Values - x86 Optimization NMI and MCE support

... but the topic is stalled right now, due to hpa having had objections. Have those concerns been resolved? (Peter Cc:-ed)

i'd have applied this fix, but it does not apply. The first chunk seems already be present (in a different form), the second chunk looks much different.

Ingo

Mathieu Desnoyers

author

2008-07-18 18:31 UTC #2 lore raw

Quoted 19 lines · * Ingo Molnar ([email protected]) wrote:

Quoted 8 lines · Mathieu Desnoyers
Hi Ingo,
This fix would apply to the ftrace tree which contains the jump-based immediate values. I don't know if this tree is still under active development, but I guess that it wouldn't hurt to fix it.
Caused a kernel OOPS when loading the tracepoint probes once in a while on a 8-way x86_64.
We've still got tip/tracing/immediates, with these commits from you:
 $ git-log-line linus.. arch/x86/kernel/immediate.c
 ee563d6: immediate values: jump liveliness
 e26875a: Immediate Values - Jump
 3fc8d03: Immediate Values - x86 Optimization NMI and MCE support

Quoted 9 lines

... but the topic is stalled right now, due to hpa having had objections. Have those concerns been resolved? (Peter Cc:-ed)
i'd have applied this fix, but it does not apply. The first chunk seems already be present (in a different form), the second chunk looks much different.
Ingo

Hrm, I've edited directly the immediate values: jump liveliness patches, which explains why it does not apply. I'll try to unapply/reapply/fold the patches and see what it gives.

Plus, I've noticed that the "Text Edit Lock" patches are not in the immediates branch, thus it fails to compile. Immediate values depends on the Text Edit Lock patches.

Mathieu

Signature · 2 lines

Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

H. Peter Anvin

reviewer

2008-07-18 19:51 UTC #3 lore raw

Quoted 10 lines · Mathieu Desnoyers

Quoted 9 lines
 $ git-log-line linus.. arch/x86/kernel/immediate.c
 ee563d6: immediate values: jump liveliness
 e26875a: Immediate Values - Jump
 3fc8d03: Immediate Values - x86 Optimization NMI and MCE support
... but the topic is stalled right now, due to hpa having had objections. Have those concerns been resolved? (Peter Cc:-ed)
i'd have applied this fix, but it does not apply. The first chunk seems already be present (in a different form), the second chunk looks much different.

Quoted 8 lines

Hrm, I've edited directly the immediate values: jump liveliness patches, which explains why it does not apply. I'll try to unapply/reapply/fold the patches and see what it gives.
Plus, I've noticed that the "Text Edit Lock" patches are not in the immediates branch, thus it fails to compile. Immediate values depends on the Text Edit Lock patches.

My previous objection was that flow of control really does need to be understood by the compiler, and I don't see how that could have been resolved without involving gcc.

I'm not opposed to static jump optimization in general, far from it, but doing it behind the back of the compiler is fraught with peril, and even if it can be made correct is going to generate bad enough code that I have to question if it is worth the additional complexity.

I definitely do not approve of the attempt to truncate liveliness by putting a clobber after the if branch; there is still intervening code generated by the C compiler which is going to cause some extremely hard to debug problems at some point.

-hpa

Mathieu Desnoyers

author

2008-07-18 21:12 UTC #4 lore raw

Quoted 29 lines · * H. Peter Anvin ([email protected]) wrote:

Quoted 12 lines · Mathieu Desnoyers
Quoted 6 lines
 $ git-log-line linus.. arch/x86/kernel/immediate.c
 ee563d6: immediate values: jump liveliness
 e26875a: Immediate Values - Jump
 3fc8d03: Immediate Values - x86 Optimization NMI and MCE support
Quoted 6 lines
... but the topic is stalled right now, due to hpa having had objections. Have those concerns been resolved? (Peter Cc:-ed)
i'd have applied this fix, but it does not apply. The first chunk seems already be present (in a different form), the second chunk looks much different.
Quoted 6 lines
Hrm, I've edited directly the immediate values: jump liveliness patches, which explains why it does not apply. I'll try to unapply/reapply/fold the patches and see what it gives.
Plus, I've noticed that the "Text Edit Lock" patches are not in the immediates branch, thus it fails to compile. Immediate values depends on the Text Edit Lock patches.
My previous objection was that flow of control really does need to be understood by the compiler, and I don't see how that could have been resolved without involving gcc.
I'm not opposed to static jump optimization in general, far from it, but doing it behind the back of the compiler is fraught with peril, and even if it can be made correct is going to generate bad enough code that I have to question if it is worth the additional complexity.

Quoted 5 lines

I definitely do not approve of the attempt to truncate liveliness by putting a clobber after the if branch; there is still intervening code generated by the C compiler which is going to cause some extremely hard to debug problems at some point.

Hrm, I thought that by following the execution flow to both branches and by looking at the code pattern found (load immediate, test, branch) and by placing a constraint on the eax register to detect the liveliness region of that variable we would be guaranteed that the compiler could not re-use this variable outside of the pattern scope.

The generated code you are talking about will generate a different code pattern, won't it (e.g. saving the registers on the stack before the branch) ? And in this case, we fall-back on the standard immediate values.

However, I agree that doing this without compiler support has been a pain. One thing we could do while we wait for gcc support is to merge conditional-branch based immediate values only, which are less complex, and later on add the static jump feature when supported by the compiler.

Mathieu

Quoted 2 lines

-hpa

Signature · 2 lines

Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

H. Peter Anvin

reviewer

2008-07-18 23:37 UTC #5 lore raw

Quoted 7 lines · Mathieu Desnoyers

Hrm, I thought that by following the execution flow to both branches and by looking at the code pattern found (load immediate, test, branch) and by placing a constraint on the eax register to detect the liveliness region of that variable we would be guaranteed that the compiler could not re-use this variable outside of the pattern scope.

Nope, and you *cannot* allow such registers to be live *anywhere* the C compiler is generating code, even between two asm() statements with nothing in between.

-hpa

H. Peter Anvin

reviewer

2008-07-18 23:38 UTC #6 lore raw

In reply to Mathieu Desnoyers

Quoted 6 lines · Mathieu Desnoyers

The generated code you are talking about will generate a different code pattern, won't it (e.g. saving the registers on the stack before the branch) ? And in this case, we fall-back on the standard immediate values.

Not necessarily, and you *certainly* can't rely on pattern analysis to determine when it's unsafe.

-hpa

Mathieu Desnoyers

author

2008-07-18 21:51 UTC #7 lore raw

[git pull tip/tracing/immediates] Immediate values fixes/redux

In reply to H. Peter Anvin

Hi Ingo,

Here are fixes for the tip/tracing/immediates tree, with the last patch being the removal of the static branch code.

It is available at :

git://ltt.polymtl.ca/linux-2.6-instrumentation.git tracing/immediates-pull

I first added the missing dependencies (text edit lock and kprobes mutex modifications) :

kprobes-use-mutex-for-insn-pages.patch kprobes-dont-use-kprobes-mutex-in-arch-code.patch kprobes-declare-kprobes-mutex-static.patch text-edit-lock-architecture-independent-code.patch text-edit-lock-kprobes-architecture-independent-support.patch

Then comes the fixes and added debugging code :

immediate-values-remove-on_each_cpu-argument.patch immediate-values-use-i-asm-constraint.patch fix-immediate-values-x86_64-support-old-gcc.patch immediate-values-remove-var-size-field-from-jump-optimization.patch immediate-values-optimized-jump-add-debug.patch

And finally removal of static jump :

markers-use-conditional-branch-instead-of-static-jump.patch immediate-values-remove-static-jump.patch

Mathieu

Signature · 2 lines

Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Mathieu Desnoyers

author

2008-07-19 05:59 UTC #8 lore raw

Re: [git pull tip/tracing/immediates] Immediate values fixes/redux

Quoted 9 lines · * Mathieu Desnoyers ([email protected]) wrote:

Hi Ingo,
Here are fixes for the tip/tracing/immediates tree, with the last patch being the removal of the static branch code.
It is available at :
git://ltt.polymtl.ca/linux-2.6-instrumentation.git tracing/immediates-pull

Quoted 9 lines

I first added the missing dependencies (text edit lock and kprobes mutex modifications) :
kprobes-use-mutex-for-insn-pages.patch kprobes-dont-use-kprobes-mutex-in-arch-code.patch kprobes-declare-kprobes-mutex-static.patch text-edit-lock-architecture-independent-code.patch text-edit-lock-kprobes-architecture-independent-support.patch

Just to say : those last 5 patches have not been cherry-picked from tip/*. They are probably in one of these branches.

Mathieu

Quoted 19 lines

Then comes the fixes and added debugging code :
immediate-values-remove-on_each_cpu-argument.patch immediate-values-use-i-asm-constraint.patch fix-immediate-values-x86_64-support-old-gcc.patch immediate-values-remove-var-size-field-from-jump-optimization.patch immediate-values-optimized-jump-add-debug.patch
And finally removal of static jump :
markers-use-conditional-branch-instead-of-static-jump.patch immediate-values-remove-static-jump.patch
Mathieu
Signature · 2 lines
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Signature · 2 lines

Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Ingo Molnar

2008-07-18 22:32 UTC #9 lore raw

In reply to Mathieu Desnoyers

Quoted 30 lines · Mathieu Desnoyers

Quoted 19 lines · * Ingo Molnar ([email protected]) wrote:
Quoted 8 lines · Mathieu Desnoyers
Hi Ingo,
This fix would apply to the ftrace tree which contains the jump-based immediate values. I don't know if this tree is still under active development, but I guess that it wouldn't hurt to fix it.
Caused a kernel OOPS when loading the tracepoint probes once in a while on a 8-way x86_64.
We've still got tip/tracing/immediates, with these commits from you:
 $ git-log-line linus.. arch/x86/kernel/immediate.c
 ee563d6: immediate values: jump liveliness
 e26875a: Immediate Values - Jump
 3fc8d03: Immediate Values - x86 Optimization NMI and MCE support
Quoted 9 lines
... but the topic is stalled right now, due to hpa having had objections. Have those concerns been resolved? (Peter Cc:-ed)
i'd have applied this fix, but it does not apply. The first chunk seems already be present (in a different form), the second chunk looks much different.
Ingo

Quoted 7 lines

Hrm, I've edited directly the immediate values: jump liveliness patches, which explains why it does not apply. I'll try to unapply/reapply/fold the patches and see what it gives.
Plus, I've noticed that the "Text Edit Lock" patches are not in the immediates branch, thus it fails to compile. Immediate values depends on the Text Edit Lock patches.

feel free to git-cherry-pick the commits together into a coherent unit (preferrably all -git based), and we can give it a whirl in tip/master.

Ingo