Re: [PATCH] tracing: Cleanup the convoluted softirq tracepoints
From: Jason Baron
Date: Tue Oct 19 2010 - 20:44:26 EST
On Tue, Oct 19, 2010 at 04:39:07PM -0700, H. Peter Anvin wrote:
> On 10/19/2010 03:27 PM, Peter Zijlstra wrote:
> >
> > Due to not actually having a sane key type the above is not easy to
> > implement, but I tried:
> >
> > #define _SWITCH_POINT(x)\
> > ({ \
> > __label__ jl_enabled; \
> > bool ret = true; \
> > JUMP_LABEL(x, jl_enabled); \
> > ret = false; \
> > jl_enabled: \
> > ret; })
> >
> > #define SWITCH_POINT(x) unlikely(_SWITCH_POINT(x))
> >
> > #define COND_STMT(key, stmt) \
> > do { \
> > if (SWITCH_POINT(key)) { \
> > stmt; \
> > } \
> > } while (0)
> >
> >
> > and that's still generating these double jumps.
> >
>
> I just experimented with it, and the ({...}) construct doesn't work,
> because it looks like a merged flow of control to gcc.
>
> Replacing the ({ ... }) with an inline does indeed remove the double
> jumps.
>
> diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h
> index b67cb18..2ff829d 100644
> --- a/include/linux/jump_label.h
> +++ b/include/linux/jump_label.h
> @@ -61,12 +61,22 @@ static inline int jump_label_text_reserved(void
> *start, void *end)
>
> #endif
>
> +static __always_inline __pure bool _SWITCH_POINT(void *x)
> +{
> + asm goto("# SWITCH_POINT %0\n\t"
> + ".byte 0x66,0x66,0x66,0x66,0x90\n"
> + "1:"
> + : : "i" (x) : : jl_enabled);
> + return false;
> +jl_enabled:
> + return true;
> +}
> +
> +#define SWITCH_POINT(x) unlikely(_SWITCH_POINT(x))
> +
> #define COND_STMT(key, stmt) \
> do { \
> - __label__ jl_enabled; \
> - JUMP_LABEL(key, jl_enabled); \
> - if (0) { \
> -jl_enabled: \
> + if (SWITCH_POINT(key)) { \
> stmt; \
> } \
> } while (0)
>
>
> The key here seems to be to not use the JUMP_LABEL macro as implemented;
> I have utterly failed to make JUMP_LABEL() do the right thing.
>
ok, I tried this out for the tracepoint code, but I still seem to be
getting the double jump.
patch:
diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h
index 1947a12..7bc2537 100644
--- a/include/linux/jump_label.h
+++ b/include/linux/jump_label.h
@@ -66,12 +66,22 @@ static inline void jump_label_unlock(void) {}
#endif
+static __always_inline __pure bool _SWITCH_POINT(void *x)
+{
+ asm goto("# SWITCH_POINT %0\n\t"
+ ".byte 0x66,0x66,0x66,0x66,0x90\n"
+ "1:"
+ : : "i" (x) : : jl_enabled);
+ return false;
+jl_enabled:
+ return true;
+}
+
+#define SWITCH_POINT(x) unlikely(_SWITCH_POINT(x))
+
#define COND_STMT(key, stmt) \
do { \
- __label__ jl_enabled; \
- JUMP_LABEL(key, jl_enabled); \
- if (0) { \
-jl_enabled: \
+ if (SWITCH_POINT(key)) { \
stmt; \
} \
} while (0)
diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
index a4a90b6..1f8d14f 100644
--- a/include/linux/tracepoint.h
+++ b/include/linux/tracepoint.h
@@ -146,12 +146,7 @@ static inline void tracepoint_update_probe_range(struct tracepoint *begin,
extern struct tracepoint __tracepoint_##name; \
static inline void trace_##name(proto) \
{ \
- JUMP_LABEL(&__tracepoint_##name.state, do_trace); \
- return; \
-do_trace: \
- __DO_TRACE(&__tracepoint_##name, \
- TP_PROTO(data_proto), \
- TP_ARGS(data_args)); \
+ COND_STMT(&__tracepoint_##name.state, __DO_TRACE(&__tracepoint_##name, TP_PROTO(data_proto), TP_ARGS(data_args))); \
} \
static inline int \
register_trace_##name(void (*probe)(data_proto), void *data) \
disassemly:
ffffffff810360a6 <set_task_cpu>:
ffffffff810360a6: 55 push %rbp
ffffffff810360a7: 48 89 e5 mov %rsp,%rbp
ffffffff810360aa: 41 55 push %r13
ffffffff810360ac: 41 54 push %r12
ffffffff810360ae: 41 89 f4 mov %esi,%r12d
ffffffff810360b1: 53 push %rbx
ffffffff810360b2: 48 89 fb mov %rdi,%rbx
ffffffff810360b5: 48 81 ec b8 00 00 00 sub $0xb8,%rsp
ffffffff810360bc: 66 66 66 66 90 data32 data32 data32
xchg %ax,%ax
ffffffff810360c1: eb 19 jmp ffffffff810360dc
<set_task_cpu+0x36>
ffffffff810360c3: 49 8b 7d 08 mov 0x8(%r13),%rdi
ffffffff810360c7: 44 89 e2 mov %r12d,%edx
ffffffff810360ca: 48 89 de mov %rbx,%rsi
ffffffff810360cd: 41 ff 55 00 callq *0x0(%r13)
ffffffff810360d1: 49 83 c5 10 add $0x10,%r13
ffffffff810360d5: 49 83 7d 00 00 cmpq $0x0,0x0(%r13)
ffffffff810360da: eb 6c jmp ffffffff81036148
<set_task_cpu+0xa2>
ffffffff810360dc: 48 8b 43 08 mov 0x8(%rbx),%rax
ffffffff810360e0: 44 39 60 18 cmp %r12d,0x18(%rax)
ffffffff810360e4: 74 37 je ffffffff8103611d
<set_task_cpu+0x77>
ffffffff810360e6: 48 ff 83 98 00 00 00 incq 0x98(%rbx)
ffffffff810360ed: e9 00 00 00 00 jmpq ffffffff810360f2
<set_task_cpu+0x4c>
ffffffff810360f2: eb 29 jmp ffffffff8103611d
<set_task_cpu+0x77>
ffffffff810360f4: 4c 8d ad 30 ff ff ff lea -0xd0(%rbp),%r13
ffffffff810360fb: 4c 89 ef mov %r13,%rdi
ffffffff810360fe: e8 c7 94 ff ff callq ffffffff8102f5ca
<perf_fetch_caller_regs>
ffffffff81036103: 45 31 c0 xor %r8d,%r8d
ffffffff81036106: 4c 89 e9 mov %r13,%rcx
ffffffff81036109: ba 01 00 00 00 mov $0x1,%edx
ffffffff8103610e: be 01 00 00 00 mov $0x1,%esi
ffffffff81036113: bf 04 00 00 00 mov $0x4,%edi
ffffffff81036118: e8 67 19 07 00 callq ffffffff810a7a84
<__perf_sw_event>
ffffffff8103611d: 44 89 e6 mov %r12d,%esi
ffffffff81036120: 48 89 df mov %rbx,%rdi
ffffffff81036123: e8 2f 75 ff ff callq ffffffff8102d657
<set_task_rq>
ffffffff81036128: 48 8b 43 08 mov 0x8(%rbx),%rax
ffffffff8103612c: 44 89 60 18 mov %r12d,0x18(%rax)
ffffffff81036130: 48 81 c4 b8 00 00 00 add $0xb8,%rsp
ffffffff81036137: 5b pop %rbx
ffffffff81036138: 41 5c pop %r12
ffffffff8103613a: 41 5d pop %r13
ffffffff8103613c: c9 leaveq
ffffffff8103613d: c3 retq
ffffffff8103613e: 4c 8b 2d 3b 26 a9 00 mov
0xa9263b(%rip),%r13 # ffffffff81ac8780
<__tracepoint_sched_migrate_task+0x20>
ffffffff81036145: 4d 85 ed test %r13,%r13
ffffffff81036148: 0f 85 75 ff ff ff jne ffffffff810360c3
<set_task_cpu+0x1d>
ffffffff8103614e: eb 8c jmp ffffffff810360dc
<set_task_cpu+0x36>
I'm using gcc (GCC) 4.5.1 20100812
is my patch wrong?
thanks,
-Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/