[PATCH ftrace.git] Immedate Values Optimized Jump Fix

[PATCH ftrace.git] Immedate Values Optimized Jump Fix

[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.


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 <mathieu.desnoyers@polymtl.ca>
---
 arch/x86/kernel/immediate.c |    2 +-
 include/asm-x86/immediate.h |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Index: linux-2.6-lttng/arch/x86/kernel/immediate.c
===================================================================
--- linux-2.6-lttng.orig/arch/x86/kernel/immediate.c	2008-07-16 22:50:35.000000000 -0400
+++ linux-2.6-lttng/arch/x86/kernel/immediate.c	2008-07-16 22:51:07.000000000 -0400
@@ -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;
Index: linux-2.6-lttng/include/asm-x86/immediate.h
===================================================================
--- linux-2.6-lttng.orig/include/asm-x86/immediate.h	2008-07-16 22:49:19.000000000 -0400
+++ linux-2.6-lttng/include/asm-x86/immediate.h	2008-07-16 22:51:07.000000000 -0400
@@ -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"					\

-- 
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

Re: [PATCH ftrace.git] Immedate Values Optimized Jump Fix

[Ingo Molnar]

* Mathieu Desnoyers <compudj@krystal.dyndns.org> wrote:

> 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

Re: [PATCH ftrace.git] Immedate Values Optimized Jump Fix

[Mathieu Desnoyers]

* Ingo Molnar (mingo@elte.hu) wrote:
> 
> * Mathieu Desnoyers <compudj@krystal.dyndns.org> wrote:
> 
> > 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
> 

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


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

Re: [PATCH ftrace.git] Immedate Values Optimized Jump Fix

[H. Peter Anvin]

Mathieu Desnoyers wrote:
>>
>>  $ 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.
> 
> 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

Re: [PATCH ftrace.git] Immedate Values Optimized Jump Fix

[Mathieu Desnoyers]

* H. Peter Anvin (hpa@zytor.com) wrote:
> Mathieu Desnoyers wrote:
>>>
>>>  $ 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.
>> 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.
>

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

> 	-hpa
>



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

Re: [PATCH ftrace.git] Immedate Values Optimized Jump Fix

[H. Peter Anvin]

Mathieu Desnoyers wrote:
> 
> 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

Re: [PATCH ftrace.git] Immedate Values Optimized Jump Fix

[H. Peter Anvin]

Mathieu Desnoyers wrote:
> 
> 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

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

[Mathieu Desnoyers]

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


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

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

[Mathieu Desnoyers]

* Mathieu Desnoyers (compudj@krystal.dyndns.org) 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
> 
> 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

> 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
> 
> 
> -- 
> 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

Re: [PATCH ftrace.git] Immedate Values Optimized Jump Fix

[Ingo Molnar]

* Mathieu Desnoyers <compudj@krystal.dyndns.org> wrote:

> * Ingo Molnar (mingo@elte.hu) wrote:
> > 
> > * Mathieu Desnoyers <compudj@krystal.dyndns.org> wrote:
> > 
> > > 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
> > 
> 
> 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