ftrace: Remove a superfluous check

PATCH In review Borislav Petkov 4 participants 12 messages Mar 29, 2012 – Jun 20, 2012

Subsystems (2)

Tracing ftrace

lore

Share & tools

lore

Borislav Petkov

author

2012-03-29 16:44 UTC #0 lore raw

PATCH 1/1 ftrace: Remove a superfluous check

From: Borislav Petkov <[email protected]>

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 <[email protected]>

---
 kernel/trace/ftrace.c |    6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

kernel/trace/ftrace.c +1 -5

index 867bd1dd2dd0..f9212017406d 100644
@@ -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;

Signature · 1 line

1.7.9.3.362.g71319

John Kacur

2012-03-29 17:04 UTC #1 lore raw

Quoted 33 lines · Borislav Petkov

From: Borislav Petkov <[email protected]>
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 <[email protected]>
---
 kernel/trace/ftrace.c |    6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)
kernel/trace/ftrace.c +1 -2
index 867bd1dd2dd0..f9212017406d 100644
@@ -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))

kernel/trace/ftrace.c int register_ftrace_function(struct ftrace_ops *ops) · line 4313

Quoted 10 lines

kernel/trace/ftrace.c +0 -3

-		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

Borislav Petkov

author

2012-03-29 17:11 UTC #2 lore raw

Quoted 1 line · John Kacur

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

Jahaa... thanks.

Here's an updated patch:

Quoted 1 line

From 135c25a609739bbdf1c33e62119517b47f0e1d07 Mon Sep 17 00:00:00 2001

From: Borislav Petkov <[email protected]>
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 <[email protected]>

Signed-off-by Borislav Petkov <[email protected]>

---
 kernel/trace/ftrace.c |    8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

kernel/trace/ftrace.c +2 -6

index 867bd1dd2dd0..0df8f8088ffe 100644
@@ -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);

Signature · 13 lines

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

John Kacur

2012-03-29 17:18 UTC #3 lore raw

Quoted 61 lines · Borislav Petkov

Quoted 1 line · John Kacur
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 <[email protected]>
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 <[email protected]>
Signed-off-by Borislav Petkov <[email protected]>
---
 kernel/trace/ftrace.c |    8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)
kernel/trace/ftrace.c +2 -6
index 867bd1dd2dd0..0df8f8088ffe 100644
@@ -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);
Signature · 1 line
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

Steven Rostedt

maintainer

2012-03-29 17:24 UTC #4 lore raw

Quoted 6 lines · John Kacur

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

Borislav Petkov

author

2012-03-29 17:26 UTC #5 lore raw

In reply to John Kacur

Quoted 3 lines · John Kacur

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.

Quoted 1 line

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

Thanks.

Signature · 8 lines

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

Borislav Petkov

author

2012-05-24 16:32 UTC #6 lore raw

In reply to Borislav Petkov

Yo Steve,

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

Thanks.

Quoted 60 lines · Borislav Petkov

Quoted 1 line · John Kacur
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 <[email protected]>
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 <[email protected]>
Signed-off-by Borislav Petkov <[email protected]>
---
 kernel/trace/ftrace.c |    8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)
kernel/trace/ftrace.c +2 -6
index 867bd1dd2dd0..0df8f8088ffe 100644
@@ -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);
Signature · 1 line
1.7.9.3.362.g71319

Signature · 8 lines

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

Steven Rostedt

maintainer

2012-05-24 16:42 UTC #7 lore raw

Quoted 3 lines · Borislav Petkov

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

Quoted 13 lines

Thanks.
Quoted 9 lines · Borislav Petkov
Quoted 1 line · John Kacur
If you're going to do this, then you can drop the label too.
Jahaa... thanks.
Here's an updated patch:
--

Borislav Petkov

2012-05-24 17:05 UTC #8 lore raw

Quoted 7 lines · Steven Rostedt

Quoted 3 lines · Borislav Petkov
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.

Signature · 2 lines

Regards/Gruss,
    Boris.

tip-bot for Borislav Petkov

author

2012-06-20 10:39 UTC #9 lore raw

In reply to Borislav Petkov

Commit-ID:  8d240dd88cca33b704adf3fe281aa64b5aac2dd8
Gitweb:     http://git.kernel.org/tip/8d240dd88cca33b704adf3fe281aa64b5aac2dd8
Author:     Borislav Petkov <[email protected]>
AuthorDate: Thu, 29 Mar 2012 19:11:40 +0200
Committer:  Steven Rostedt <[email protected]>
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 open here

Cc Borislav Petkov <[email protected]>

Cc John Kacur <[email protected]>

Signed-off-by Borislav Petkov <[email protected]>

Signed-off-by Steven Rostedt <[email protected]>

---
 kernel/trace/ftrace.c |    8 ++------
 1 files changed, 2 insertions(+), 6 deletions(-)

kernel/trace/ftrace.c +2 -6

index a008663..b4f20fb 100644
@@ -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);

Steven Rostedt

maintainer

2012-03-29 17:15 UTC #10 lore raw

In reply to John Kacur

Quoted 10 lines · John Kacur

Quoted 10 lines

kernel/trace/ftrace.c +0 -3

 	mutex_lock(&ftrace_lock);
 
-	if (unlikely(ftrace_disabled))
-		goto out_unlock;
-
 	ret = __register_ftrace_function(ops);
 	if (!ret)
 		ret = ftrace_startup(ops, 0);

Quoted 8 lines

kernel/trace/ftrace.c int register_ftrace_function(struct ftrace_ops *ops) · line 4313
Quoted 5 lines
kernel/trace/ftrace.c +0 -1
-
  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

Borislav Petkov

author

2012-03-29 17:27 UTC #11 lore raw

Quoted 1 line · Steven Rostedt

Yeah, can you resubmit with John's suggestion.

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

Thanks.

Signature · 8 lines

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