[RFC/PATCH] Prevent wasting time to find out get_parent_ip

[RFC/PATCH] Prevent wasting time to find out get_parent_ip

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

Cc: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Minho Ban <mhban@samsung.com>
---
 include/linux/ftrace.h |    3 ---
 kernel/sched/core.c    |    5 ++++-
 kernel/softirq.c       |    3 ++-
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 72a6cab..7cd11fe 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -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
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 4603b9d..efdd5a0 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -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);
diff --git a/kernel/softirq.c b/kernel/softirq.c
index 671f959..e7c8336 100644
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -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)
-- 
1.7.5.4

Re: [RFC/PATCH] Prevent wasting time to find out get_parent_ip

[Peter Zijlstra]

On Tue, 2012-04-24 at 21:36 +0900, Minho Ban wrote:
> 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..

Re: [RFC/PATCH] Prevent wasting time to find out get_parent_ip

[Steven Rostedt]

On Tue, 2012-04-24 at 21:36 +0900, Minho Ban wrote:
> 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!

> Cc: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Minho Ban <mhban@samsung.com>
> ---
>  include/linux/ftrace.h |    3 ---
>  kernel/sched/core.c    |    5 ++++-
>  kernel/softirq.c       |    3 ++-
>  3 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index 72a6cab..7cd11fe 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -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

>  #endif
>  
>  #ifdef CONFIG_FTRACE_MCOUNT_RECORD
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 4603b9d..efdd5a0 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -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);
> diff --git a/kernel/softirq.c b/kernel/softirq.c
> index 671f959..e7c8336 100644
> --- a/kernel/softirq.c
> +++ b/kernel/softirq.c
> @@ -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)

Re: [RFC/PATCH] Prevent wasting time to find out get_parent_ip

[Steven Rostedt]

On Tue, 2012-04-24 at 14:53 +0200, Peter Zijlstra wrote:
> On Tue, 2012-04-24 at 21:36 +0900, Minho Ban wrote:
> > 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

Re: [RFC/PATCH] Prevent wasting time to find out get_parent_ip

[Minho Ban]

On 04/24/2012 09:53 PM, Peter Zijlstra wrote:
> On Tue, 2012-04-24 at 21:36 +0900, Minho Ban wrote:
>> 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.

Re: [RFC/PATCH] Prevent wasting time to find out get_parent_ip

[Josh Triplett]

On Wed, Apr 25, 2012 at 08:31:24AM +0900, Minho Ban wrote:
> On 04/24/2012 09:53 PM, Peter Zijlstra wrote:
> > On Tue, 2012-04-24 at 21:36 +0900, Minho Ban wrote:
> >> 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.

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

Re: [RFC/PATCH] Prevent wasting time to find out get_parent_ip

[Minho Ban]

On 04/25/2012 03:16 AM, Steven Rostedt wrote:
> On Tue, 2012-04-24 at 21:36 +0900, Minho Ban wrote:
>> 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.

>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Signed-off-by: Minho Ban <mhban@samsung.com>
>> ---
>>  include/linux/ftrace.h |    3 ---
>>  kernel/sched/core.c    |    5 ++++-
>>  kernel/softirq.c       |    3 ++-
>>  3 files changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
>> index 72a6cab..7cd11fe 100644
>> --- a/include/linux/ftrace.h
>> +++ b/include/linux/ftrace.h
>> @@ -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
> 

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

>>  #endif
>>  
>>  #ifdef CONFIG_FTRACE_MCOUNT_RECORD
>> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
>> index 4603b9d..efdd5a0 100644
>> --- a/kernel/sched/core.c
>> +++ b/kernel/sched/core.c
>> @@ -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);
>> diff --git a/kernel/softirq.c b/kernel/softirq.c
>> index 671f959..e7c8336 100644
>> --- a/kernel/softirq.c
>> +++ b/kernel/softirq.c
>> @@ -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)
> 
> 
> 

Re: [RFC/PATCH] Prevent wasting time to find out get_parent_ip

[Minho Ban]

On 04/25/2012 08:45 AM, Josh Triplett wrote:
> On Wed, Apr 25, 2012 at 08:31:24AM +0900, Minho Ban wrote:
>> On 04/24/2012 09:53 PM, Peter Zijlstra wrote:
>>> On Tue, 2012-04-24 at 21:36 +0900, Minho Ban wrote:
>>>> 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.
> 
> 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.

Re: [RFC/PATCH] Prevent wasting time to find out get_parent_ip

[Steven Rostedt]

On Wed, 2012-04-25 at 12:21 +0900, Minho Ban wrote:
> > 
> > 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

Re: [RFC/PATCH] Prevent wasting time to find out get_parent_ip

[Josh Triplett]

On Wed, Apr 25, 2012 at 12:21:36PM +0900, Minho Ban wrote:
> On 04/25/2012 08:45 AM, Josh Triplett wrote:
> > On Wed, Apr 25, 2012 at 08:31:24AM +0900, Minho Ban wrote:
> >> On 04/24/2012 09:53 PM, Peter Zijlstra wrote:
> >>> On Tue, 2012-04-24 at 21:36 +0900, Minho Ban wrote:
> >>>> 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.
> > 
> > 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.

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