Prevent wasting time to find out get_parent_ip

RFC/PATCH Minho Ban 4 participants 10 messages Apr 24, 2012 – Apr 25, 2012

Subsystems (2)

Scheduler ftrace

lore

Share & tools

lore

Minho Ban

author

2012-04-24 12:36 UTC #0 lore raw

trace_preempt_on/off looks empty if PREEMPT_TRACER is off. But actually it is spending time to find out get_parent_ip(even CALLER_ADDR for some ARCH) which is in argument. This seems not fair for those who expect to do nothing but increase or decrease count.

Cc Thomas Gleixner <[email protected]>

Signed-off-by Minho Ban <[email protected]>

---
 include/linux/ftrace.h |    3 ---
 kernel/sched/core.c    |    5 ++++-
 kernel/softirq.c       |    3 ++-
 3 files changed, 6 insertions(+), 5 deletions(-)

include/linux/ftrace.h +0 -3

index 72a6cab..7cd11fe 100644
@@ -484,9 +484,6 @@ static inline void __ftrace_enabled_restore(int enabled)
 #ifdef CONFIG_PREEMPT_TRACER
   extern void trace_preempt_on(unsigned long a0, unsigned long a1);
   extern void trace_preempt_off(unsigned long a0, unsigned long a1);
-#else
-  static inline void trace_preempt_on(unsigned long a0, unsigned long a1) { }
-  static inline void trace_preempt_off(unsigned long a0, unsigned long a1) { }
 #endif
 
 #ifdef CONFIG_FTRACE_MCOUNT_RECORD

kernel/sched/core.c +4 -1

index 4603b9d..efdd5a0 100644
@@ -3068,8 +3068,10 @@ void __kprobes add_preempt_count(int val)
 	DEBUG_LOCKS_WARN_ON((preempt_count() & PREEMPT_MASK) >=
 				PREEMPT_MASK - 10);
 #endif
+#ifdef CONFIG_PREEMPT_TRACER
 	if (preempt_count() == val)
 		trace_preempt_off(CALLER_ADDR0, get_parent_ip(CALLER_ADDR1));
+#endif
 }
 EXPORT_SYMBOL(add_preempt_count);

@@ -3088,9 +3090,10 @@ void __kprobes sub_preempt_count(int val)
 			!(preempt_count() & PREEMPT_MASK)))
 		return;
 #endif
-
+#ifdef CONFIG_PREEMPT_TRACER
 	if (preempt_count() == val)
 		trace_preempt_on(CALLER_ADDR0, get_parent_ip(CALLER_ADDR1));
+#endif
 	preempt_count() -= val;
 }
 EXPORT_SYMBOL(sub_preempt_count);

kernel/softirq.c +2 -1

index 671f959..e7c8336 100644
@@ -112,9 +112,10 @@ static void __local_bh_disable(unsigned long ip, unsigned int cnt)
 	if (softirq_count() == cnt)
 		trace_softirqs_off(ip);
 	raw_local_irq_restore(flags);
-
+#ifdef CONFIG_PREEMPT_TRACER
 	if (preempt_count() == cnt)
 		trace_preempt_off(CALLER_ADDR0, get_parent_ip(CALLER_ADDR1));
+#endif
 }
 #else /* !CONFIG_TRACE_IRQFLAGS */
 static inline void __local_bh_disable(unsigned long ip, unsigned int cnt)

Signature · 1 line

1.7.5.4

Peter Zijlstra

2012-04-24 12:54 UTC #1 lore raw

Quoted 4 lines · Minho Ban

trace_preempt_on/off looks empty if PREEMPT_TRACER is off. But actually it is spending time to find out get_parent_ip(even CALLER_ADDR for some ARCH) which is in argument. This seems not fair for those who expect to do nothing but increase or decrease count.

You can do the same by making them CPP macros and adding a comment as to why they're macros instead of inlines..

Steven Rostedt

maintainer

2012-04-24 18:18 UTC #2 lore raw

Quoted 8 lines · Peter Zijlstra

Quoted 4 lines · Minho Ban
trace_preempt_on/off looks empty if PREEMPT_TRACER is off. But actually it is spending time to find out get_parent_ip(even CALLER_ADDR for some ARCH) which is in argument. This seems not fair for those who expect to do nothing but increase or decrease count.
You can do the same by making them CPP macros and adding a comment as to why they're macros instead of inlines..

I should read the thread before answering. I just repeated basically exactly what you said ;-)

-- Steve

Minho Ban

author

2012-04-24 23:31 UTC #3 lore raw

In reply to Peter Zijlstra

Quoted 9 lines · Peter Zijlstra

Quoted 4 lines · Minho Ban
trace_preempt_on/off looks empty if PREEMPT_TRACER is off. But actually it is spending time to find out get_parent_ip(even CALLER_ADDR for some ARCH) which is in argument. This seems not fair for those who expect to do nothing but increase or decrease count.
You can do the same by making them CPP macros and adding a comment as to why they're macros instead of inlines..

Thank you for pointing this out, certainly macros look better. I'll amend this.

Josh Triplett

2012-04-24 23:45 UTC #4 lore raw

Quoted 7 lines · Minho Ban

Quoted 6 lines · Peter Zijlstra
Quoted 4 lines · Minho Ban
trace_preempt_on/off looks empty if PREEMPT_TRACER is off. But actually it is spending time to find out get_parent_ip(even CALLER_ADDR for some ARCH) which is in argument. This seems not fair for those who expect to do nothing but increase or decrease count.

Quoted 5 lines

Quoted 3 lines
You can do the same by making them CPP macros and adding a comment as to why they're macros instead of inlines..
Thank you for pointing this out, certainly macros look better. I'll amend this.

As an alternative, how about making get_parent_ip and its called functions static inlines? Then the compiler can eliminate them via dead code elimination.

Or, how about declaring get_parent_ip with the GCC "pure" attribute? That would tell GCC that it can safely eliminate calls to the function.

- Josh Triplett

Minho Ban

author

2012-04-25 03:21 UTC #5 lore raw

Quoted 14 lines · Josh Triplett

Quoted 7 lines · Minho Ban
Quoted 6 lines · Peter Zijlstra
Quoted 4 lines · Minho Ban
trace_preempt_on/off looks empty if PREEMPT_TRACER is off. But actually it is spending time to find out get_parent_ip(even CALLER_ADDR for some ARCH) which is in argument. This seems not fair for those who expect to do nothing but increase or decrease count.
Quoted 5 lines
Quoted 3 lines
You can do the same by making them CPP macros and adding a comment as to why they're macros instead of inlines..
Thank you for pointing this out, certainly macros look better. I'll amend this.

Quoted 9 lines

As an alternative, how about making get_parent_ip and its called functions static inlines? Then the compiler can eliminate them via dead code elimination.
Or, how about declaring get_parent_ip with the GCC "pure" attribute? That would tell GCC that it can safely eliminate calls to the function.
- Josh Triplett

Thank you for alternative method, but I'm afraid this could not cover the CALLER_ADDR.

Steven Rostedt

maintainer

2012-04-25 03:32 UTC #6 lore raw

Quoted 8 lines · Minho Ban

Quoted 6 lines
Or, how about declaring get_parent_ip with the GCC "pure" attribute? That would tell GCC that it can safely eliminate calls to the function.
- Josh Triplett
Thank you for alternative method, but I'm afraid this could not cover the CALLER_ADDR.

That's fine. I personally prefer the macro with the comment as that is not as subtle of a fix. It's the most straight forward.

-- Steve

Josh Triplett

2012-04-25 06:47 UTC #7 lore raw

In reply to Minho Ban

Quoted 19 lines · Minho Ban

Quoted 18 lines · Josh Triplett
Quoted 7 lines · Minho Ban
Quoted 6 lines · Peter Zijlstra
Quoted 4 lines · Minho Ban
trace_preempt_on/off looks empty if PREEMPT_TRACER is off. But actually it is spending time to find out get_parent_ip(even CALLER_ADDR for some ARCH) which is in argument. This seems not fair for those who expect to do nothing but increase or decrease count.
Quoted 5 lines
Quoted 3 lines
You can do the same by making them CPP macros and adding a comment as to why they're macros instead of inlines..
Thank you for pointing this out, certainly macros look better. I'll amend this.
As an alternative, how about making get_parent_ip and its called functions static inlines? Then the compiler can eliminate them via dead code elimination.

Quoted 7 lines

Quoted 5 lines
Or, how about declaring get_parent_ip with the GCC "pure" attribute? That would tell GCC that it can safely eliminate calls to the function.
- Josh Triplett
Thank you for alternative method, but I'm afraid this could not cover the CALLER_ADDR.

As far as I can tell, CALLER_ADDR() always means ftrace_caller(), and CALLER_ADDR[0-6] always reference __builtin_return_address() or return_address(). In the former case, you could mark ftrace_caller pure, and in the latter cases, if GCC for some reason generates code for those without eliminating it as dead code, you could add trivial wrappers around those functions that have the pure attribute set on them. That should allow GCC to completely eliminate all of those.

Would that work?

- Josh Triplett

Steven Rostedt

maintainer

2012-04-24 18:16 UTC #8 lore raw

In reply to Minho Ban

Quoted 5 lines · Minho Ban

trace_preempt_on/off looks empty if PREEMPT_TRACER is off. But actually it is spending time to find out get_parent_ip(even CALLER_ADDR for some ARCH) which is in argument. This seems not fair for those who expect to do nothing but increase or decrease count.

Yuck what an ugly patch. Please don't uglify the C code with #ifdefs!

Quoted 13 lines

Cc Thomas Gleixner <[email protected]>
Signed-off-by Minho Ban <[email protected]>
---
 include/linux/ftrace.h |    3 ---
 kernel/sched/core.c    |    5 ++++-
 kernel/softirq.c       |    3 ++-
 3 files changed, 6 insertions(+), 5 deletions(-)
include/linux/ftrace.h +0 -0
index 72a6cab..7cd11fe 100644

include/linux/ftrace.h static inline void __ftrace_enabled_restore(int enabled) · line 489

Quoted 6 lines

include/linux/ftrace.h +0 -3

@@ -484,9 +484,6 @@ static inline void __ftrace_enabled_restore(int enabled)
 #ifdef CONFIG_PREEMPT_TRACER
   extern void trace_preempt_on(unsigned long a0, unsigned long a1);
   extern void trace_preempt_off(unsigned long a0, unsigned long a1);
-#else
-  static inline void trace_preempt_on(unsigned long a0, unsigned long a1) { }
-  static inline void trace_preempt_off(unsigned long a0, unsigned long a1) { }

If you want to hide the "CALLER_ADDR" for other archs, then simply do:

# define trace_preempt_on(a0, a1) do { } while (0)
# define trace_preempt_off(a0, a1) do { } while (0)

and be done with it.

Oh, you should add a comment before these defines to the effect of:

/*
* Use defines instead of static inlines because some arches will make
* code out of the CALLER_ADDR, when we really want these to be a real
* nop.
*/

That way, you will document why we use defines instead of static inlines.

-- Steve

Quoted 46 lines

include/linux/ftrace.h +0 -0

 #endif
 
 #ifdef CONFIG_FTRACE_MCOUNT_RECORD

kernel/sched/core.c +4 -1

index 4603b9d..efdd5a0 100644
@@ -3068,8 +3068,10 @@ void __kprobes add_preempt_count(int val)
 	DEBUG_LOCKS_WARN_ON((preempt_count() & PREEMPT_MASK) >=
 				PREEMPT_MASK - 10);
 #endif
+#ifdef CONFIG_PREEMPT_TRACER
 	if (preempt_count() == val)
 		trace_preempt_off(CALLER_ADDR0, get_parent_ip(CALLER_ADDR1));
+#endif
 }
 EXPORT_SYMBOL(add_preempt_count);

@@ -3088,9 +3090,10 @@ void __kprobes sub_preempt_count(int val)
 			!(preempt_count() & PREEMPT_MASK)))
 		return;
 #endif
-
+#ifdef CONFIG_PREEMPT_TRACER
 	if (preempt_count() == val)
 		trace_preempt_on(CALLER_ADDR0, get_parent_ip(CALLER_ADDR1));
+#endif
 	preempt_count() -= val;
 }
 EXPORT_SYMBOL(sub_preempt_count);

kernel/softirq.c +2 -1

index 671f959..e7c8336 100644
@@ -112,9 +112,10 @@ static void __local_bh_disable(unsigned long ip, unsigned int cnt)
 	if (softirq_count() == cnt)
 		trace_softirqs_off(ip);
 	raw_local_irq_restore(flags);
-
+#ifdef CONFIG_PREEMPT_TRACER
 	if (preempt_count() == cnt)
 		trace_preempt_off(CALLER_ADDR0, get_parent_ip(CALLER_ADDR1));
+#endif
 }
 #else /* !CONFIG_TRACE_IRQFLAGS */
 static inline void __local_bh_disable(unsigned long ip, unsigned int cnt)

Minho Ban

author

2012-04-24 23:54 UTC #9 lore raw

Quoted 9 lines · Steven Rostedt

Quoted 5 lines · Minho Ban
trace_preempt_on/off looks empty if PREEMPT_TRACER is off. But actually it is spending time to find out get_parent_ip(even CALLER_ADDR for some ARCH) which is in argument. This seems not fair for those who expect to do nothing but increase or decrease count.
Yuck what an ugly patch. Please don't uglify the C code with #ifdefs!

Yes, I agree.

Quoted 35 lines

Quoted 13 lines
Cc Thomas Gleixner <[email protected]>
Signed-off-by Minho Ban <[email protected]>
---
 include/linux/ftrace.h |    3 ---
 kernel/sched/core.c    |    5 ++++-
 kernel/softirq.c       |    3 ++-
 3 files changed, 6 insertions(+), 5 deletions(-)
include/linux/ftrace.h +0 -0
index 72a6cab..7cd11fe 100644
include/linux/ftrace.h static inline void __ftrace_enabled_restore(int enabled) · line 489
Quoted 6 lines
include/linux/ftrace.h +0 -3
@@ -484,9 +484,6 @@ static inline void __ftrace_enabled_restore(int enabled)
 #ifdef CONFIG_PREEMPT_TRACER
   extern void trace_preempt_on(unsigned long a0, unsigned long a1);
   extern void trace_preempt_off(unsigned long a0, unsigned long a1);
-#else
-  static inline void trace_preempt_on(unsigned long a0, unsigned long a1) { }
-  static inline void trace_preempt_off(unsigned long a0, unsigned long a1) { }
If you want to hide the "CALLER_ADDR" for other archs, then simply do:
# define trace_preempt_on(a0, a1) do { } while (0)
# define trace_preempt_off(a0, a1) do { } while (0)
and be done with it.
Oh, you should add a comment before these defines to the effect of:
/*
* Use defines instead of static inlines because some arches will make
* code out of the CALLER_ADDR, when we really want these to be a real
* nop.
*/

Quoted 5 lines

That way, you will document why we use defines instead of static inlines.
-- Steve

Very appreciate your explanation and detailed(even complete) sample code. I'll apply your code.

Quoted 49 lines

Quoted 46 lines

include/linux/ftrace.h +0 -0

 #endif
 
 #ifdef CONFIG_FTRACE_MCOUNT_RECORD

kernel/sched/core.c +4 -1

index 4603b9d..efdd5a0 100644
@@ -3068,8 +3068,10 @@ void __kprobes add_preempt_count(int val)
 	DEBUG_LOCKS_WARN_ON((preempt_count() & PREEMPT_MASK) >=
 				PREEMPT_MASK - 10);
 #endif
+#ifdef CONFIG_PREEMPT_TRACER
 	if (preempt_count() == val)
 		trace_preempt_off(CALLER_ADDR0, get_parent_ip(CALLER_ADDR1));
+#endif
 }
 EXPORT_SYMBOL(add_preempt_count);

@@ -3088,9 +3090,10 @@ void __kprobes sub_preempt_count(int val)
 			!(preempt_count() & PREEMPT_MASK)))
 		return;
 #endif
-
+#ifdef CONFIG_PREEMPT_TRACER
 	if (preempt_count() == val)
 		trace_preempt_on(CALLER_ADDR0, get_parent_ip(CALLER_ADDR1));
+#endif
 	preempt_count() -= val;
 }
 EXPORT_SYMBOL(sub_preempt_count);

kernel/softirq.c +2 -1

index 671f959..e7c8336 100644
@@ -112,9 +112,10 @@ static void __local_bh_disable(unsigned long ip, unsigned int cnt)
 	if (softirq_count() == cnt)
 		trace_softirqs_off(ip);
 	raw_local_irq_restore(flags);
-
+#ifdef CONFIG_PREEMPT_TRACER
 	if (preempt_count() == cnt)
 		trace_preempt_off(CALLER_ADDR0, get_parent_ip(CALLER_ADDR1));
+#endif
 }
 #else /* !CONFIG_TRACE_IRQFLAGS */
 static inline void __local_bh_disable(unsigned long ip, unsigned int cnt)