[PATCH] ftrace: Remove a superfluous check

[PATCH] ftrace: Remove a superfluous check

[Borislav Petkov]

From: Borislav Petkov <borislav.petkov@amd.com>

register_ftrace_function() checks ftrace_disabled and calls
__register_ftrace_function which does it again.

Drop the first check and add the unlikely hint to the second one.

No functional change.

Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
---
 kernel/trace/ftrace.c |    6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 867bd1dd2dd0..f9212017406d 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -311,7 +311,7 @@ static int remove_ftrace_list_ops(struct ftrace_ops **list,
 
 static int __register_ftrace_function(struct ftrace_ops *ops)
 {
-	if (ftrace_disabled)
+	if (unlikely(ftrace_disabled))
 		return -ENODEV;
 
 	if (FTRACE_WARN_ON(ops == &global_ops))
@@ -4304,14 +4304,10 @@ int register_ftrace_function(struct ftrace_ops *ops)
 
 	mutex_lock(&ftrace_lock);
 
-	if (unlikely(ftrace_disabled))
-		goto out_unlock;
-
 	ret = __register_ftrace_function(ops);
 	if (!ret)
 		ret = ftrace_startup(ops, 0);
 
-
  out_unlock:
 	mutex_unlock(&ftrace_lock);
 	return ret;
-- 
1.7.9.3.362.g71319

Re: [PATCH] ftrace: Remove a superfluous check

[John Kacur]

On Thu, 29 Mar 2012, Borislav Petkov wrote:

> From: Borislav Petkov <borislav.petkov@amd.com>
> 
> register_ftrace_function() checks ftrace_disabled and calls
> __register_ftrace_function which does it again.
> 
> Drop the first check and add the unlikely hint to the second one.
> 
> No functional change.
> 
> Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
> ---
>  kernel/trace/ftrace.c |    6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 867bd1dd2dd0..f9212017406d 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -311,7 +311,7 @@ static int remove_ftrace_list_ops(struct ftrace_ops **list,
>  
>  static int __register_ftrace_function(struct ftrace_ops *ops)
>  {
> -	if (ftrace_disabled)
> +	if (unlikely(ftrace_disabled))
>  		return -ENODEV;
>  
>  	if (FTRACE_WARN_ON(ops == &global_ops))
> @@ -4304,14 +4304,10 @@ int register_ftrace_function(struct ftrace_ops *ops)
>  
>  	mutex_lock(&ftrace_lock);
>  
> -	if (unlikely(ftrace_disabled))
> -		goto out_unlock;
> -
>  	ret = __register_ftrace_function(ops);
>  	if (!ret)
>  		ret = ftrace_startup(ops, 0);
>  
> -
>   out_unlock:
>  	mutex_unlock(&ftrace_lock);
>  	return ret;
> -- 

If you're going to do this, then you can drop the label too.

Thanks
John Kacur

Re: [PATCH] ftrace: Remove a superfluous check

[Borislav Petkov]

On Thu, Mar 29, 2012 at 07:03:23PM +0200, John Kacur wrote:
> If you're going to do this, then you can drop the label too.

Jahaa... thanks.

Here's an updated patch:

--
>From 135c25a609739bbdf1c33e62119517b47f0e1d07 Mon Sep 17 00:00:00 2001
From: Borislav Petkov <borislav.petkov@amd.com>
Date: Thu, 29 Mar 2012 18:41:15 +0200
Subject: [PATCH] ftrace: Remove a superfluous check

register_ftrace_function() checks ftrace_disabled and calls
__register_ftrace_function which does it again.

Drop the first check and add the unlikely hint to the second one. Also,
drop the label as John correctly notices.

No functional change.

Cc: John Kacur <jkacur@redhat.com>
Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
---
 kernel/trace/ftrace.c |    8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 867bd1dd2dd0..0df8f8088ffe 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -311,7 +311,7 @@ static int remove_ftrace_list_ops(struct ftrace_ops **list,
 
 static int __register_ftrace_function(struct ftrace_ops *ops)
 {
-	if (ftrace_disabled)
+	if (unlikely(ftrace_disabled))
 		return -ENODEV;
 
 	if (FTRACE_WARN_ON(ops == &global_ops))
@@ -4304,16 +4304,12 @@ int register_ftrace_function(struct ftrace_ops *ops)
 
 	mutex_lock(&ftrace_lock);
 
-	if (unlikely(ftrace_disabled))
-		goto out_unlock;
-
 	ret = __register_ftrace_function(ops);
 	if (!ret)
 		ret = ftrace_startup(ops, 0);
 
-
- out_unlock:
 	mutex_unlock(&ftrace_lock);
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(register_ftrace_function);
-- 
1.7.9.3.362.g71319



-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: [PATCH] ftrace: Remove a superfluous check

[Steven Rostedt]

On Thu, 2012-03-29 at 19:03 +0200, John Kacur wrote:

> >  
> >  	mutex_lock(&ftrace_lock);
> >  
> > -	if (unlikely(ftrace_disabled))
> > -		goto out_unlock;
> > -
> >  	ret = __register_ftrace_function(ops);
> >  	if (!ret)
> >  		ret = ftrace_startup(ops, 0);
> >  
> > -
> >   out_unlock:
> >  	mutex_unlock(&ftrace_lock);
> >  	return ret;
> > -- 
> 
> If you're going to do this, then you can drop the label too.
> 

Yeah, can you resubmit with John's suggestion.

Thanks,

-- Steve

Re: [PATCH] ftrace: Remove a superfluous check

[John Kacur]

On Thu, 29 Mar 2012, Borislav Petkov wrote:

> On Thu, Mar 29, 2012 at 07:03:23PM +0200, John Kacur wrote:
> > If you're going to do this, then you can drop the label too.
> 
> Jahaa... thanks.
> 
> Here's an updated patch:
> 
> --
> From 135c25a609739bbdf1c33e62119517b47f0e1d07 Mon Sep 17 00:00:00 2001
> From: Borislav Petkov <borislav.petkov@amd.com>
> Date: Thu, 29 Mar 2012 18:41:15 +0200
> Subject: [PATCH] ftrace: Remove a superfluous check
> 
> register_ftrace_function() checks ftrace_disabled and calls
> __register_ftrace_function which does it again.
> 
> Drop the first check and add the unlikely hint to the second one. Also,
> drop the label as John correctly notices.
> 
> No functional change.
> 
> Cc: John Kacur <jkacur@redhat.com>
> Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
> ---
>  kernel/trace/ftrace.c |    8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 867bd1dd2dd0..0df8f8088ffe 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -311,7 +311,7 @@ static int remove_ftrace_list_ops(struct ftrace_ops **list,
>  
>  static int __register_ftrace_function(struct ftrace_ops *ops)
>  {
> -	if (ftrace_disabled)
> +	if (unlikely(ftrace_disabled))
>  		return -ENODEV;
>  
>  	if (FTRACE_WARN_ON(ops == &global_ops))
> @@ -4304,16 +4304,12 @@ int register_ftrace_function(struct ftrace_ops *ops)
>  
>  	mutex_lock(&ftrace_lock);
>  
> -	if (unlikely(ftrace_disabled))
> -		goto out_unlock;
> -
>  	ret = __register_ftrace_function(ops);
>  	if (!ret)
>  		ret = ftrace_startup(ops, 0);
>  
> -
> - out_unlock:
>  	mutex_unlock(&ftrace_lock);
> +
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(register_ftrace_function);
> -- 
> 1.7.9.3.362.g71319
> 

It looks okay to me. Technically it's not functionally equivalent though, 
because now when __register_ftrace_function is called directly, (in other 
paths), the test has an unlikely there. See what Steven says, otherwise, 
you can have my reviewed by.

Thanks

John

Re: [PATCH] ftrace: Remove a superfluous check

[Steven Rostedt]

On Thu, 2012-03-29 at 19:17 +0200, John Kacur wrote:
> 

> It looks okay to me. Technically it's not functionally equivalent though, 
> because now when __register_ftrace_function is called directly, (in other 
> paths), the test has an unlikely there. See what Steven says, otherwise, 
> you can have my reviewed by.

Unlikely()s shouldn't affect things functionally, its just an
optimization hint.

Also, this check is unlikely to be true, so the hint is valid here.

-- Steve

Re: [PATCH] ftrace: Remove a superfluous check

[Borislav Petkov]

On Thu, Mar 29, 2012 at 07:17:19PM +0200, John Kacur wrote:

> It looks okay to me. Technically it's not functionally equivalent
> though, because now when __register_ftrace_function is called
> directly, (in other paths), the test has an unlikely there.

I know, but ftrace_disabled is really unlikely to be set to 1 because
we set it only if ftrace_init() fails and on the panic path in
ftrace_kill().

So I agree that it is not functionally completely equivalent but it
should be correct with the unlikely.

> See what teven says, otherwise, you can have my reviewed by.

Thanks.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: [PATCH] ftrace: Remove a superfluous check

[Borislav Petkov]

On Thu, Mar 29, 2012 at 01:14:54PM -0400, Steven Rostedt wrote:
> Yeah, can you resubmit with John's suggestion.

Yep, already did: http://marc.info/?l=linux-kernel&m=133304119114872

Thanks.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: [PATCH] ftrace: Remove a superfluous check

[Borislav Petkov]

Yo Steve,

want to pick this one up? It still applies cleanly to current linus.

Thanks.

On Thu, Mar 29, 2012 at 07:11:40PM +0200, Borislav Petkov wrote:
> On Thu, Mar 29, 2012 at 07:03:23PM +0200, John Kacur wrote:
> > If you're going to do this, then you can drop the label too.
> 
> Jahaa... thanks.
> 
> Here's an updated patch:
> 
> --
> From 135c25a609739bbdf1c33e62119517b47f0e1d07 Mon Sep 17 00:00:00 2001
> From: Borislav Petkov <borislav.petkov@amd.com>
> Date: Thu, 29 Mar 2012 18:41:15 +0200
> Subject: [PATCH] ftrace: Remove a superfluous check
> 
> register_ftrace_function() checks ftrace_disabled and calls
> __register_ftrace_function which does it again.
> 
> Drop the first check and add the unlikely hint to the second one. Also,
> drop the label as John correctly notices.
> 
> No functional change.
> 
> Cc: John Kacur <jkacur@redhat.com>
> Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
> ---
>  kernel/trace/ftrace.c |    8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 867bd1dd2dd0..0df8f8088ffe 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -311,7 +311,7 @@ static int remove_ftrace_list_ops(struct ftrace_ops **list,
>  
>  static int __register_ftrace_function(struct ftrace_ops *ops)
>  {
> -	if (ftrace_disabled)
> +	if (unlikely(ftrace_disabled))
>  		return -ENODEV;
>  
>  	if (FTRACE_WARN_ON(ops == &global_ops))
> @@ -4304,16 +4304,12 @@ int register_ftrace_function(struct ftrace_ops *ops)
>  
>  	mutex_lock(&ftrace_lock);
>  
> -	if (unlikely(ftrace_disabled))
> -		goto out_unlock;
> -
>  	ret = __register_ftrace_function(ops);
>  	if (!ret)
>  		ret = ftrace_startup(ops, 0);
>  
> -
> - out_unlock:
>  	mutex_unlock(&ftrace_lock);
> +
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(register_ftrace_function);
> -- 
> 1.7.9.3.362.g71319

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: [PATCH] ftrace: Remove a superfluous check

[Steven Rostedt]

On Thu, 2012-05-24 at 18:32 +0200, Borislav Petkov wrote:
> Yo Steve,
> 
> want to pick this one up? It still applies cleanly to current linus.

I actually have it in my 3.6 queue. It didn't look like it was urgent
for 3.5. Is it?

-- Steve

> 
> Thanks.
> 
> On Thu, Mar 29, 2012 at 07:11:40PM +0200, Borislav Petkov wrote:
> > On Thu, Mar 29, 2012 at 07:03:23PM +0200, John Kacur wrote:
> > > If you're going to do this, then you can drop the label too.
> > 
> > Jahaa... thanks.
> > 
> > Here's an updated patch:
> > 
> > --
> >

Re: [PATCH] ftrace: Remove a superfluous check

[Borislav Petkov]

On Thu, May 24, 2012 at 12:41:50PM -0400, Steven Rostedt wrote:
> On Thu, 2012-05-24 at 18:32 +0200, Borislav Petkov wrote:
> > Yo Steve,
> > 
> > want to pick this one up? It still applies cleanly to current linus.
> 
> I actually have it in my 3.6 queue. It didn't look like it was urgent
> for 3.5. Is it?

Nah, I was just cleaning up my mbox and saw it.

Thanks.

-- 
Regards/Gruss,
    Boris.

[tip:perf/core] ftrace: Remove a superfluous check

[tip-bot for Borislav Petkov]

Commit-ID:  8d240dd88cca33b704adf3fe281aa64b5aac2dd8
Gitweb:     http://git.kernel.org/tip/8d240dd88cca33b704adf3fe281aa64b5aac2dd8
Author:     Borislav Petkov <bp@amd64.org>
AuthorDate: Thu, 29 Mar 2012 19:11:40 +0200
Committer:  Steven Rostedt <rostedt@goodmis.org>
CommitDate: Thu, 14 Jun 2012 15:22:12 -0400

ftrace: Remove a superfluous check

register_ftrace_function() checks ftrace_disabled and calls
__register_ftrace_function which does it again.

Drop the first check and add the unlikely hint to the second one. Also,
drop the label as John correctly notices.

No functional change.

Link: http://lkml.kernel.org/r/20120329171140.GE6409@aftab

Cc: Borislav Petkov <bp@amd64.org>
Cc: John Kacur <jkacur@redhat.com>
Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/trace/ftrace.c |    8 ++------
 1 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index a008663..b4f20fb 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -312,7 +312,7 @@ static int remove_ftrace_list_ops(struct ftrace_ops **list,
 
 static int __register_ftrace_function(struct ftrace_ops *ops)
 {
-	if (ftrace_disabled)
+	if (unlikely(ftrace_disabled))
 		return -ENODEV;
 
 	if (FTRACE_WARN_ON(ops == &global_ops))
@@ -4299,16 +4299,12 @@ int register_ftrace_function(struct ftrace_ops *ops)
 
 	mutex_lock(&ftrace_lock);
 
-	if (unlikely(ftrace_disabled))
-		goto out_unlock;
-
 	ret = __register_ftrace_function(ops);
 	if (!ret)
 		ret = ftrace_startup(ops, 0);
 
-
- out_unlock:
 	mutex_unlock(&ftrace_lock);
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(register_ftrace_function);