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(-)
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
* Mathieu Desnoyers <[email protected]> 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
* Ingo Molnar ([email protected]) wrote:
>
> * Mathieu Desnoyers <[email protected]> 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
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
* H. Peter Anvin ([email protected]) 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
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
* Mathieu Desnoyers <[email protected]> wrote:
> * Ingo Molnar ([email protected]) wrote:
> >
> > * Mathieu Desnoyers <[email protected]> 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
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
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
* 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
>
> 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