x86_64: cleanup IRQ on/off tracing
PATCH 1/1 x86_64: cleanup IRQ on/off tracing
Tracing of IRQ on/off paths on x86_64 currently suffers from heavy over-instrumentation. This patch aligns x86_64 in this regard with i386 (and hopefully the other tracer-supporting archs too). It also gets rid of related *_thunk indirections (I wonder if we cannot kill more of them...).
Jan
--- arch/x86_64/kernel/entry.S | 26 ++++++++++++++++++-------- arch/x86_64/kernel/ipipe.c | 16 ---------------- arch/x86_64/lib/thunk.S | 5 ----- include/asm-x86_64/irqflags.h | 15 ++++----------- 4 files changed, 22 insertions(+), 40 deletions(-)
arch/x86_64/kernel/entry.S +18 -8
@@ -542,16 +542,28 @@ END(stub_rt_sigreturn) */ TRACE_IRQS_OFF #ifdef CONFIG_IPIPE_TRACE_IRQSOFF - leaq RIP-8(%rdi), %rbp - pushq %rdi - movq ORIG_RAX(%rdi), %rdi + leaq RIP-8(%rdi), %rbp # make interrupted address show up in trace + pushq %rdi + movq ORIG_RAX(%rdi), %rdi # IRQ number + notq %rdi # ...is inverted, fix up + call ipipe_trace_begin + popq %rdi + popq %rbp + pushq %rbp + + call \func + + popq %rbp + pushq %rbp + movq 8-ARGOFFSET+ORIG_RAX(%rbp), %rdi + leaq 8-ARGOFFSET+RIP-8(%rbp), %rbp notq %rdi - call ipipe_trace_begin_thunk - popq %rdi + call ipipe_trace_end popq %rbp pushq %rbp -#endif +#else call \func +#endif .endm #ifdef CONFIG_IPIPE
@@ -602,7 +614,6 @@ ENTRY(common_interrupt) jnz ret_from_intr decl %gs:pda_irqcount leaveq - IPIPE_TRACE_IRQS_ON CFI_DEF_CFA_REGISTER rsp CFI_ADJUST_CFA_OFFSET -8 testl $3,CS-ARGOFFSET(%rsp)
@@ -734,7 +745,6 @@ END(common_interrupt) jnz ret_from_intr decl %gs:pda_irqcount leaveq - IPIPE_TRACE_IRQS_ON CFI_DEF_CFA_REGISTER rsp CFI_ADJUST_CFA_OFFSET -8 testl $3,CS-ARGOFFSET(%rsp)
arch/x86_64/kernel/ipipe.c +0 -16
@@ -760,10 +760,6 @@ finalize: finalize_nosync: -#ifdef CONFIG_IPIPE_TRACE_IRQSOFF -ipipe_trace_end (irq ); -#endif /* CONFIG_IPIPE_TRACE_IRQSOFF */ -if (!ipipe_root_domain_p ||test_bit (IPIPE_STALL_FLAG , &ipipe_root_cpudom_var (status)))return 0 ;
@@ -771,18 +767,6 @@ finalize_nosync:return 1 ; } -#ifdef CONFIG_IPIPE_TRACE_IRQSOFF -void notrace __ipipe_trace_irqs_off(void) -{ -ipipe_trace_begin (0x80000000 ); -} - -void notrace __ipipe_trace_irqs_on(void) -{ -ipipe_trace_end (0x80000000 ); -} -#endif /* CONFIG_IPIPE_TRACE_IRQSOFF */ -EXPORT_SYMBOL (__ipipe_tick_irq);EXPORT_SYMBOL (ipipe_critical_enter);EXPORT_SYMBOL (ipipe_critical_exit);
arch/x86_64/lib/thunk.S +0 -5
@@ -56,11 +56,6 @@ thunk_retrax __ipipe_preempt_schedule_irq_thunk,__ipipe_preempt_schedule_irq #endif thunk_retrax __ipipe_syscall_root_thunk,__ipipe_syscall_root -#ifdef CONFIG_IPIPE_TRACE_IRQSOFF - thunk __ipipe_trace_irqs_on_thunk,__ipipe_trace_irqs_on - thunk __ipipe_trace_irqs_off_thunk,__ipipe_trace_irqs_off - thunk ipipe_trace_begin_thunk,ipipe_trace_begin -#endif #endif /* SAVE_ARGS below is used only for the .cfi directives it contains. */
include/asm-x86_64/irqflags.h +4 -11
@@ -241,19 +241,12 @@ static inline void local_irq_enable_hw_n#endif /* CONFIG_IPIPE_TRACE_IRQSOFF */ #else /* __ASSEMBLY__: */ -#ifdef CONFIG_IPIPE_TRACE_IRQSOFF -#define IPIPE_TRACE_IRQS_ON call __ipipe_trace_irqs_on_thunk -#define IPIPE_TRACE_IRQS_OFF call __ipipe_trace_irqs_off_thunk -#else /* !CONFIG_IPIPE_TRACE_IRQSOFF */ -#define IPIPE_TRACE_IRQS_ON -#define IPIPE_TRACE_IRQS_OFF -#endif /* !CONFIG_IPIPE_TRACE_IRQSOFF */ # ifdef CONFIG_TRACE_IRQFLAGS -# define TRACE_IRQS_ON IPIPE_TRACE_IRQS_ON; call trace_hardirqs_on_thunk -# define TRACE_IRQS_OFF IPIPE_TRACE_IRQS_OFF; call trace_hardirqs_off_thunk +# define TRACE_IRQS_ON call trace_hardirqs_on_thunk +# define TRACE_IRQS_OFF call trace_hardirqs_off_thunk# else -# define TRACE_IRQS_ON IPIPE_TRACE_IRQS_ON -# define TRACE_IRQS_OFF IPIPE_TRACE_IRQS_OFF +# define TRACE_IRQS_ON +# define TRACE_IRQS_OFF # endif #endif
Quoted 2 lines · Jan Kiszka
Tracing of IRQ on/off paths on
x86_64currently suffers from heavy over-instrumentation.
I understand the point of grouping ipipe_trace_begin/ipipe_trace_end statements inside the .interrupt macros and using a lightweight thunk code since we are already covered by the SAVE_ARGS prologue, but I find the following hunk suspicious, since unlike i386, we do not virtualize inline sti/cli ops for x86_64. My concern is that removing this instrumentation would leave us naked in the cold the day some subtle upstream change introduces a hw masked section we don't immediately notice; such trace points would precisely help us spotting it.
include/asm-x86_64/irqflags.h +0 -9
@@ -241,19 +241,12 @@ static inline void local_irq_enable_hw_n#endif /* CONFIG_IPIPE_TRACE_IRQSOFF */ #else /* __ASSEMBLY__: */ -#ifdef CONFIG_IPIPE_TRACE_IRQSOFF -#define IPIPE_TRACE_IRQS_ON call __ipipe_trace_irqs_on_thunk -#define IPIPE_TRACE_IRQS_OFF call __ipipe_trace_irqs_off_thunk -#else /* !CONFIG_IPIPE_TRACE_IRQSOFF */ -#define IPIPE_TRACE_IRQS_ON -#define IPIPE_TRACE_IRQS_OFF -#endif /* !CONFIG_IPIPE_TRACE_IRQSOFF */ # ifdef CONFIG_TRACE_IRQFLAGS -# define TRACE_IRQS_ON IPIPE_TRACE_IRQS_ON; call trace_hardirqs_on_thunk -# define TRACE_IRQS_OFF IPIPE_TRACE_IRQS_OFF; call
trace_hardirqs_off_thunk
+# define TRACE_IRQS_ON call trace_hardirqs_on_thunk
+# define TRACE_IRQS_OFF call trace_hardirqs_off_thunk
# else -# define TRACE_IRQS_ON IPIPE_TRACE_IRQS_ON -# define TRACE_IRQS_OFF IPIPE_TRACE_IRQS_OFF +# define TRACE_IRQS_ON +# define TRACE_IRQS_OFF # endif #endif
This patch aligns x86_64 in this regard with i386
Quoted 3 lines
(and hopefully the other tracer-supporting archs too). It also gets rid of related
*_thunkindirections (I wonder if we cannot kill more of them...).
We could probably kill the __ipipe_preempt_schedule_irq thunk quite easily, since we branch to exit_intr which starts with a very limited register context. However, replacing the thunk routing __ipipe_syscall_root would require more work, since the syscall arg registers are expected to survive the call, and those are not callee-saved according to the AMD64 ABI (which makes sense). We would at least have to pull all the potentially spilled regs from the saved exception frame upon return from the I-pipe hook. Ok, this would save a few cycles we currently spend saving them...
Signature · 1 line
Philippe.
Quoted 4 lines · Philippe Gerum
Quoted 2 lines · Jan Kiszka
Tracing of IRQ on/off paths on
x86_64currently suffers from heavy over-instrumentation.
Quoted 8 lines
I understand the point of grouping ipipe_trace_begin/ipipe_trace_end statements inside the .interrupt macros and using a lightweight thunk code since we are already covered by the
SAVE_ARGSprologue, but I find the following hunk suspicious, since unlike i386, we do not virtualize inline sti/cli ops forx86_64. My concern is that removing this instrumentation would leave us naked in the cold the day some subtle upstream change introduces a hw masked section we don't immediately notice; such trace points would precisely help us spotting it.
The situation is not that different compared to i386:
- we do not add or remove enabling/disabled points
- we rely on the correctness of mainline here, so we are not on our own anyway
- if things actually change in mainline, the tracer will probably be the last indicator for it (patching breaks and/or leaking clis will fairly visible impact)
On the other side, there was a good reason not to instrument cli/sti in the assembly exit/entry code on i386, just like it is on x86_64: The effort to get worthwhile outputs is noticeable. As you see, the current approach is not very helpful due to loosing the caller context (thanks to the thunks). And those points only mark uninteresting micro paths, thus create a lot of noise in normal traces.
So, I see not good reason for fixing this instrumentation and still vote for killing it.
Jan
Quoted 13 lines · Jan Kiszka
Quoted 11 lines · Philippe Gerum
Quoted 2 lines · Jan Kiszka
Tracing of IRQ on/off paths on
x86_64currently suffers from heavy over-instrumentation.I understand the point of grouping ipipe_trace_begin/ipipe_trace_end statements inside the .interrupt macros and using a lightweight thunk code since we are already covered by the
SAVE_ARGSprologue, but I find the following hunk suspicious, since unlike i386, we do not virtualize inline sti/cli ops forx86_64. My concern is that removing this instrumentation would leave us naked in the cold the day some subtle upstream change introduces a hw masked section we don't immediately notice; such trace points would precisely help us spotting it.
Quoted 4 lines
The situation is not that different compared to i386:
- we do not add or remove enabling/disabled points
- we rely on the correctness of mainline here, so we are not on our own anyway
No we don't. Never, really. Gilles just found a bug in mainline's ARM syscall epilogue, which would run do_notify_resume() with hw interrupts off. Old virtualization patches had to face a subtle change in the exception #14 handling with x86, going from a trap gate to an interrupt gate, which basically means that hw interrupts where forced off branching to the handler in the latter case, unlike in the former one.
Quoted 4 lines
- if things actually change in mainline, the tracer will probably be the last indicator for it (patching breaks and/or leaking clis will fairly visible impact)
Sorry, I don't get what you mean here. Could you explain your rationale a bit more? The scenario I'm thinking of is when mainline adds some code to entry.S which may call into kernel routines, adding unnoticed latency to vanilla, but enough to be significant for any real-time activity (say
Quoted 5 lines
5us).
On the other side, there was a good reason not to instrument cli/sti in the assembly exit/entry code on i386, just like it is on
x86_64: The effort to get worthwhile outputs is noticeable.
Again, most of (pre-existing) cli/sti are virtualized by Adeos on x86, whilst they are not on x86_64, which means that x86 did not have to instrument the code about changes in the hw masking state anyway.
As you see, the current
Quoted 4 lines
approach is not very helpful due to loosing the caller context (thanks to the thunks). And those points only mark uninteresting micro paths, thus create a lot of noise in normal traces.
These are two separate issues: knowing whether the IRQs off flag is raised, and knowing which precise code spot reported this. The former is still a valuable information, despite the latter would be missing. At least, this tells you where to start digging. I agree wrt the noise issue, but this is a bit like an umbrella, it starts being useful only when it actually rains.
Btw, the caller's context is lost because we are relying on __builtin_return_address which imposes a well-defined call frame layout, we could find a more ad hoc way to handle the case of assembly-written call sites to pass this information to the tracer.
Quoted 3 lines
So, I see not good reason for fixing this instrumentation and still vote for killing it.
This is 100% tracer-related code which won't break anything, so I'm going to apply this patch if you want this to be merged. Still, you may also want to consider my point regarding vanilla's perceived infallibility. As a matter of fact, vanilla has some bugs too, and aside of that, its perception of what a pathological latency spot is may be very different (i.e. relaxed) compared to ours in some circumstances.
Quoted 2 lines
Jan
Signature · 1 line
Philippe.
Quoted 18 lines · Philippe Gerum
Quoted 12 lines · Jan Kiszka
Quoted 11 lines · Philippe Gerum
Quoted 2 lines · Jan Kiszka
Tracing of IRQ on/off paths on
x86_64currently suffers from heavy over-instrumentation.I understand the point of grouping ipipe_trace_begin/ipipe_trace_end statements inside the .interrupt macros and using a lightweight thunk code since we are already covered by the
SAVE_ARGSprologue, but I find the following hunk suspicious, since unlike i386, we do not virtualize inline sti/cli ops forx86_64. My concern is that removing this instrumentation would leave us naked in the cold the day some subtle upstream change introduces a hw masked section we don't immediately notice; such trace points would precisely help us spotting it.Quoted 4 lines
The situation is not that different compared to i386:
- we do not add or remove enabling/disabled points
- we rely on the correctness of mainline here, so we are not on our own anyway
Quoted 6 lines
No we don't. Never, really. Gilles just found a bug in mainline's ARM syscall epilogue, which would run
do_notify_resume()with hw interrupts off. Old virtualization patches had to face a subtle change in the exception #14 handling with x86, going from a trap gate to an interrupt gate, which basically means that hw interrupts where forced off branching to the handler in the latter case, unlike in the former one.
For such things we are already recording the hard IRQ mask with each and every trace point. And as long as we run with I-pipe through many common code paths in entry.S like Linux, we _do_ rely on their correctness, or better on us having carefully reviewed the code and ongoing mainline changes in new kernels.
Quoted 11 lines
Quoted 5 lines
- if things actually change in mainline, the tracer will probably be the last indicator for it (patching breaks and/or leaking clis will fairly visible impact)
Sorry, I don't get what you mean here. Could you explain your rationale a bit more? The scenario I'm thinking of is when mainline adds some code to
entry.Swhich may call into kernel routines, adding unnoticed latency to vanilla, but enough to be significant for any real-time activity (sayQuoted 1 line
5us).
Then these quirks will show up in those frozen paths we take with testsuite tools. IRQS_OFF tracing will only report it by itself if 5 us happen to be to longest path in the kernel with hard interrupts enable. Do you expect this to happen?
Quoted 31 lines
Quoted 4 lines
On the other side, there was a good reason not to instrument cli/sti in the assembly exit/entry code on i386, just like it is on
x86_64: The effort to get worthwhile outputs is noticeable.Again, most of (pre-existing) cli/sti are virtualized by Adeos on x86, whilst they are not on
x86_64, which means that x86 did not have to instrument the code about changes in the hw masking state anyway.As you see, the current
Quoted 4 lines
approach is not very helpful due to loosing the caller context (thanks to the thunks). And those points only mark uninteresting micro paths, thus create a lot of noise in normal traces.
These are two separate issues: knowing whether the IRQs off flag is raised, and knowing which precise code spot reported this. The former is still a valuable information, despite the latter would be missing. At least, this tells you where to start digging. I agree wrt the noise issue, but this is a bit like an umbrella, it starts being useful only when it actually rains.
Btw, the caller's context is lost because we are relying on
__builtin_return_addresswhich imposes a well-defined call frame layout, we could find a more ad hoc way to handle the case of assembly-written call sites to pass this information to the tracer.Quoted 3 lines
So, I see not good reason for fixing this instrumentation and still vote for killing it.
Quoted 6 lines
This is 100% tracer-related code which won't break anything, so I'm going to apply this patch if you want this to be merged. Still, you may also want to consider my point regarding vanilla's perceived infallibility. As a matter of fact, vanilla has some bugs too, and aside of that, its perception of what a pathological latency spot is may be very different (i.e. relaxed) compared to ours in some circumstances.
This particular instrumentation will not tell you by itself that something broke regarding cli/sti in [ia32]entry.S. The current version will not even be useful if you received noticed differently that something is fishy. You can fix the latter, but I would rather spent the required time on reading mainline git commits to *entry.S - more efficient IMHO because it addresses the root of (potential!) problems.
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT SE 2 Corporate Competence Center Embedded Linux
Quoted 25 lines · Jan Kiszka
Quoted 17 lines · Philippe Gerum
Quoted 12 lines · Jan Kiszka
Quoted 11 lines · Philippe Gerum
Quoted 2 lines · Jan Kiszka
Tracing of IRQ on/off paths on
x86_64currently suffers from heavy over-instrumentation.I understand the point of grouping ipipe_trace_begin/ipipe_trace_end statements inside the .interrupt macros and using a lightweight thunk code since we are already covered by the
SAVE_ARGSprologue, but I find the following hunk suspicious, since unlike i386, we do not virtualize inline sti/cli ops forx86_64. My concern is that removing this instrumentation would leave us naked in the cold the day some subtle upstream change introduces a hw masked section we don't immediately notice; such trace points would precisely help us spotting it.Quoted 4 lines
The situation is not that different compared to i386:
- we do not add or remove enabling/disabled points
- we rely on the correctness of mainline here, so we are not on our own anywayQuoted 6 lines
No we don't. Never, really. Gilles just found a bug in mainline's ARM syscall epilogue, which would run
do_notify_resume()with hw interrupts off. Old virtualization patches had to face a subtle change in the exception #14 handling with x86, going from a trap gate to an interrupt gate, which basically means that hw interrupts where forced off branching to the handler in the latter case, unlike in the former one.
Quoted 5 lines
For such things we are already recording the hard IRQ mask with each and every trace point. And as long as we run with I-pipe through many common code paths in
entry.Slike Linux, we _do_ rely on their correctness, or better on us having carefully reviewed the code and ongoing mainline changes in new kernels.
My point is that we might not have enough "density" in our instrumentation anymore to easily spot such regressions. Again, it's not like I'm saying that using the tracer is the only method to chase latencies, but since we have it now, we may want to use it whenever possible. I still don't agree on the fact that we rely on Linux's correctness though, that's partly why we indeed do analyze the standard code paths as you mentioned, actually.
Quoted 9 lines
Quoted 9 lines
Quoted 5 lines
- if things actually change in mainline, the tracer will probably be the last indicator for it (patching breaks and/or leaking clis will fairly visible impact)
Sorry, I don't get what you mean here. Could you explain your rationale a bit more? The scenario I'm thinking of is when mainline adds some code to
entry.Swhich may call into kernel routines, adding unnoticed latency to vanilla, but enough to be significant for any real-time activity (say
Quoted 6 lines
Quoted 1 line
Quoted 1 line
5us).
Then these quirks will show up in those frozen paths we take with testsuite tools.
IRQS_OFFtracing will only report it by itself if 5 us happen to be to longest path in the kernel with hard interrupts enable. Do you expect this to happen?
The point is that you may prefer having to analyze a code path made of 30 IRQs off spots, rather than one made of 200 or more. Ok, let's illustrate the scenario I have in mind:
Here is what would be the problematic kind of hunk, mainline might decide to add:
cli TRACE_IRQS_OFF ...
/* We get there hw interrupts off */sysret_careful:
bt $TIF_NEED_RESCHED,%edx
- jnc sysret_signal
+ jnc sysret_whatever
TRACE_IRQS_ON sti pushq %rdi CFI_ADJUST_CFA_OFFSET 8 call schedule popq %rdi CFI_ADJUST_CFA_OFFSET -8 jmp sysret_check /* Handle a signal */ sysret_signal: TRACE_IRQS_ON sti ...
+ sysret_whatever:
leaq -ARGOFFSET(%rsp),%rdi
call whatever_this_does_is_expensive
jmp sysret_signal
My fear is that, since we do not have instrumentation in TRACE_IRQ_xx macros anymore, we may have lesser information to spot the call to whatever_this_does_is_expensive() as being the potential latency raiser.
I do agree that we might catch this by dissecting each and every hunk mainline adds to entry.S, but still, we lose some runtime debugging support.
Quoted 35 lines
Quoted 28 lines
Quoted 4 lines
On the other side, there was a good reason not to instrument cli/sti in the assembly exit/entry code on i386, just like it is on
x86_64: The effort to get worthwhile outputs is noticeable.Again, most of (pre-existing) cli/sti are virtualized by Adeos on x86, whilst they are not on
x86_64, which means that x86 did not have to instrument the code about changes in the hw masking state anyway.As you see, the current
Quoted 4 lines
approach is not very helpful due to loosing the caller context (thanks to the thunks). And those points only mark uninteresting micro paths, thus create a lot of noise in normal traces.
These are two separate issues: knowing whether the IRQs off flag is raised, and knowing which precise code spot reported this. The former is still a valuable information, despite the latter would be missing. At least, this tells you where to start digging. I agree wrt the noise issue, but this is a bit like an umbrella, it starts being useful only when it actually rains.
Btw, the caller's context is lost because we are relying on
__builtin_return_addresswhich imposes a well-defined call frame layout, we could find a more ad hoc way to handle the case of assembly-written call sites to pass this information to the tracer.Quoted 3 lines
So, I see not good reason for fixing this instrumentation and still vote for killing it.
Quoted 6 lines
This is 100% tracer-related code which won't break anything, so I'm going to apply this patch if you want this to be merged. Still, you may also want to consider my point regarding vanilla's perceived infallibility. As a matter of fact, vanilla has some bugs too, and aside of that, its perception of what a pathological latency spot is may be very different (i.e. relaxed) compared to ours in some circumstances.
Quoted 7 lines
This particular instrumentation will not tell you by itself that something broke regarding cli/sti in [ia32]entry.S. The current version will not even be useful if you received noticed differently that something is fishy. You can fix the latter, but I would rather spent the required time on reading mainline git commits to *entry.S - more efficient IMHO because it addresses the root of (potential!) problems.
It's all a matter of method, and your argument is certainly right in a number of cases. But, we provide mechanisms to help debugging, we should not impose policies on debugging.
Quoted 2 lines
Jan
Signature · 1 line
Philippe.