sched/core: Drop debugging leftover trace_printk call

PATCH In review Borislav Petkov 6 participants 13 messages Apr 3, 2015 – Apr 7, 2015

Subsystems (2)

Scheduler Checkpatch

lore

Share & tools

lore

Borislav Petkov

author

2015-04-03 08:44 UTC #0 lore raw

PATCH 1/1 sched/core: Drop debugging leftover trace_printk call

From: Borislav Petkov <[email protected]>

Commit

3c18d447b3b3 ("sched/core: Check for available DL bandwidth in cpuset_cpu_inactive()")

forgot a trace_printk debugging piece in and Steve's banner blew in dmesg. Remove it.

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

Cc Juri Lelli <[email protected]>

Cc Peter Zijlstra (Intel) <[email protected]>

Cc Juri Lelli <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Steven Rostedt <[email protected]>

---
 kernel/sched/core.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

kernel/sched/core.c +1 -3

index a27d38f5a464..dbfc93d40292 100644
@@ -7015,10 +7015,8 @@ static int cpuset_cpu_inactive(struct notifier_block *nfb, unsigned long action,
 
 			rcu_read_unlock_sched();
 
-			if (overflow) {
-				trace_printk("hotplug failed for cpu %lu", cpu);
+			if (overflow)
 				return notifier_from_errno(-EBUSY);
-			}
 		}
 		cpuset_update_active_cpus(false);
 		break;

Signature · 1 line

2.3.3

tip-bot for Borislav Petkov

merge bot

2015-04-03 08:52 UTC #1 lore raw

sched/core: Drop debugging leftover trace_printk call

Commit-ID:  62a935b256f68a71697716595347209fb5275426
Gitweb:     http://git.kernel.org/tip/62a935b256f68a71697716595347209fb5275426
Author:     Borislav Petkov <[email protected]>
AuthorDate: Fri, 3 Apr 2015 10:42:50 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Fri, 3 Apr 2015 10:48:25 +0200

sched/core: Drop debugging leftover trace_printk call

Commit:

3c18d447b3b3 ("sched/core: Check for available DL bandwidth in cpuset_cpu_inactive()")

forgot a trace_printk() debugging piece in and Steve's banner screamed in dmesg. Remove it.

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

Cc Juri Lelli <[email protected]>

Cc Peter Zijlstra (Intel) <[email protected]>

Cc Steven Rostedt <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Ingo Molnar <[email protected]>

---
 kernel/sched/core.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

kernel/sched/core.c +1 -3

index 28b0d75..8027cfd 100644
@@ -7015,10 +7015,8 @@ static int cpuset_cpu_inactive(struct notifier_block *nfb, unsigned long action,
 
 			rcu_read_unlock_sched();
 
-			if (overflow) {
-				trace_printk("hotplug failed for cpu %lu", cpu);
+			if (overflow)
 				return notifier_from_errno(-EBUSY);
-			}
 		}
 		cpuset_update_active_cpus(false);
 		break;

Steven Rostedt

reviewer

2015-04-03 13:24 UTC #2 lore raw

In reply to Borislav Petkov

Quoted 8 lines · On Fri, 3 Apr 2015 10:42:50 +0200 Borislav Petkov <[email protected]> wrote:

From: Borislav Petkov <[email protected]>
Commit
3c18d447b3b3 ("sched/core: Check for available DL bandwidth in cpuset_cpu_inactive()")
forgot a trace_printk debugging piece in and Steve's banner blew in dmesg. Remove it.

And people say that banner doesn't do anything ;-)

-- Steve

Borislav Petkov

author

2015-04-03 13:30 UTC #3 lore raw

Quoted 5 lines · Steven Rostedt

Quoted 2 lines · On Fri, 3 Apr 2015 10:42:50 +0200 Borislav Petkov <[email protected]> wrote:
From: Borislav Petkov <[email protected]>

Quoted 8 lines

Quoted 6 lines
Commit
3c18d447b3b3 ("sched/core: Check for available DL bandwidth in cpuset_cpu_inactive()")
forgot a trace_printk debugging piece in and Steve's banner blew in dmesg. Remove it.
And people say that banner doesn't do anything ;-)

Yeah, that'll teach 'em. And if it weren't that big, I could've just as well missed it when dmesg whizzes by.

So that banner was a good idea in hindsight!

:-D

Signature · 5 lines

Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Juri Lelli

2015-04-07 13:47 UTC #4 lore raw

In reply to Borislav Petkov

Quoted 9 lines · Borislav Petkov

From: Borislav Petkov <[email protected]>
Commit
3c18d447b3b3 ("sched/core: Check for available DL bandwidth in cpuset_cpu_inactive()")
forgot a trace_printk debugging piece in and Steve's banner blew in dmesg. Remove it.

Argh! Sorry about that! Shame on me, I didn't pay much attention to Rostedt's banner because I was working on several fixes at once :(.

Anyway, how about we add also something like this to checkpatch? (I'll add appropriate CCs if this makes sense).

Thanks,

- Juri

Quoted 1 line

From e5733b377d55fd760160fa0e7822bdefa4f3a2c4 Mon Sep 17 00:00:00 2001

From: Juri Lelli <[email protected]>
Date: Sun, 5 Apr 2015 09:57:04 +0100
Subject: [PATCH] scripts/checkpatch: check for uses of trace_printk

Production kernels will scream if trace_printk() is used (thanks to Rostedt's banner). Rather than waiting for that to happen, let's check patches beforehand.

Signed-off-by Juri Lelli <[email protected]>

---
 scripts/checkpatch.pl | 6 ++++++
 1 file changed, 6 insertions(+)

scripts/checkpatch.pl +6 -0

index d124359..1fc454c5 100755
@@ -3257,6 +3257,12 @@ sub process {
 			     "Prefer printk_ratelimited or pr_<level>_ratelimited to printk_ratelimit\n" . $herecurr);
 		}
 
+# check for uses of trace_printk
+		if ($line =~ /\btrace_printk\s*\(/) {
+			ERROR("TRACE_PRINTK",
+			      "Never use trace_printk in production code!\n" . $herecurr);
+		}
+
 # printk should use KERN_* levels.  Note that follow on printk's on the
 # same line do not need a level, so we use the current block context
 # to try and find and validate the current printk.  In summary the current

Signature · 1 line

2.3.0

Quoted 27 lines

Signed-off-by Borislav Petkov <[email protected]>
Cc Juri Lelli <[email protected]>
Cc Peter Zijlstra (Intel) <[email protected]>
Cc Juri Lelli <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Steven Rostedt <[email protected]>
---
 kernel/sched/core.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
kernel/sched/core.c +1 -3
index a27d38f5a464..dbfc93d40292 100644
@@ -7015,10 +7015,8 @@ static int cpuset_cpu_inactive(struct notifier_block *nfb, unsigned long action,
 
 			rcu_read_unlock_sched();
 
-			if (overflow) {
-				trace_printk("hotplug failed for cpu %lu", cpu);
+			if (overflow)
 				return notifier_from_errno(-EBUSY);
-			}
 		}
 		cpuset_update_active_cpus(false);
 		break;

Steven Rostedt

reviewer

2015-04-07 13:56 UTC #5 lore raw

Quoted 5 lines · On Tue, 07 Apr 2015 14:47:50 +0100 Juri Lelli <[email protected]> wrote:

Quoted 4 lines · Borislav Petkov
From: Borislav Petkov <[email protected]>
Commit

Quoted 8 lines

Quoted 5 lines
3c18d447b3b3 ("sched/core: Check for available DL bandwidth in cpuset_cpu_inactive()")
forgot a trace_printk debugging piece in and Steve's banner blew in dmesg. Remove it.
Argh! Sorry about that! Shame on me, I didn't pay much attention to Rostedt's banner because I was working on several fixes at once :(.

Right, but it lets other people notice it :-)

Quoted 5 lines

scripts/checkpatch.pl +0 -0
index d124359..1fc454c5 100755

scripts/checkpatch.pl sub process { · line 3265

Quoted 9 lines

scripts/checkpatch.pl +6 -0

@@ -3257,6 +3257,12 @@ sub process {
 			     "Prefer printk_ratelimited or pr_<level>_ratelimited to printk_ratelimit\n" . $herecurr);
 		}
 
+# check for uses of trace_printk
+		if ($line =~ /\btrace_printk\s*\(/) {
+			ERROR("TRACE_PRINTK",
+			      "Never use trace_printk in production code!\n" . $herecurr);
+		}
+

if you want to be robust here. You probably want to make an exception when the code is in kernel/trace/ because "trace_printk" in patches there would be to fix the trace_printk implementation, and not its use.

-- Steve

Quoted 3 lines

scripts/checkpatch.pl +0 -0

 # printk should use KERN_* levels.  Note that follow on printk's on the
 # same line do not need a level, so we use the current block context
 # to try and find and validate the current printk.  In summary the current

Juri Lelli

2015-04-07 14:18 UTC #6 lore raw

Quoted 10 lines · Steven Rostedt

Quoted 7 lines · On Tue, 07 Apr 2015 14:47:50 +0100 Juri Lelli <[email protected]> wrote:
Quoted 6 lines · Borislav Petkov
From: Borislav Petkov <[email protected]>
Commit
3c18d447b3b3 ("sched/core: Check for available DL bandwidth in cpuset_cpu_inactive()")

Quoted 9 lines

Quoted 6 lines
Quoted 3 lines
forgot a trace_printk debugging piece in and Steve's banner blew in dmesg. Remove it.
Argh! Sorry about that! Shame on me, I didn't pay much attention to Rostedt's banner because I was working on several fixes at once :(.
Right, but it lets other people notice it :-)

Sure, that was entirely my fault not having paid attention to it :/. And it might stayed there for a while if the banner wasn't there.

Quoted 10 lines

scripts/checkpatch.pl +0 -0

index d124359..1fc454c5 100755
@@ -3257,6 +3257,12 @@ sub process {
 			     "Prefer printk_ratelimited or pr_<level>_ratelimited to printk_ratelimit\n" . $herecurr);
 		}

Quoted 11 lines

scripts/checkpatch.pl sub process { · line 3265
Quoted 6 lines
scripts/checkpatch.pl +6 -0
+# check for uses of trace_printk
+		if ($line =~ /\btrace_printk\s*\(/) {
+			ERROR("TRACE_PRINTK",
+			      "Never use trace_printk in production code!\n" . $herecurr);
+		}
+
if you want to be robust here. You probably want to make an exception when the code is in kernel/trace/ because "trace_printk" in patches there would be to fix the trace_printk implementation, and not its use.

Oh, right. I'll try to follow-up with a v2 addressing Joe's comments as well.

Thanks a lot,

- Juri

Quoted 7 lines

-- Steve

Quoted 3 lines

scripts/checkpatch.pl +0 -0

 # printk should use KERN_* levels.  Note that follow on printk's on the
 # same line do not need a level, so we use the current block context
 # to try and find and validate the current printk.  In summary the current

Joe Perches

maintainer

2015-04-07 14:01 UTC #7 lore raw

In reply to Juri Lelli

Quoted 1 line · Juri Lelli

how about we add also something like this to checkpatch?

[]

Quoted 3 lines

Production kernels will scream if trace_printk() is used (thanks to Rostedt's banner). Rather than waiting for that to happen, let's check patches beforehand.

[]

scripts/checkpatch.pl

Quoted 1 line

scripts/checkpatch.pl +0 -0

[]

scripts/checkpatch.pl sub process { · line 3264

Quoted 9 lines

scripts/checkpatch.pl +5 -0

@@ -3257,6 +3257,12 @@ sub process {
 			     "Prefer printk_ratelimited or pr_<level>_ratelimited to printk_ratelimit\n" . $herecurr);
 		}
 
+# check for uses of trace_printk
+		if ($line =~ /\btrace_printk\s*\(/) {
+			ERROR("TRACE_PRINTK",
+			      "Never use trace_printk in production code!\n" . $herecurr);
+		}

OK by me with a couple Nits:

o Please add a test for $realfile !~ m@kernel/trace/@ or maybe $realfile !~ /(?:trace|tracing)/
o ERROR seems a bit strong, WARN is probably good enough

Steven Rostedt

reviewer

2015-04-07 14:10 UTC #8 lore raw

Quoted 3 lines · On Tue, 07 Apr 2015 07:01:37 -0700 Joe Perches <[email protected]> wrote:

o Please add a test for $realfile !~ m@kernel/trace/@ or maybe $realfile !~ /(?:trace|tracing)/
o ERROR seems a bit strong, WARN is probably good enough

I'm thinking ERROR is good. There's no reason to have it. In fact, you must never have it. Looking at the other ERROR() conditions, I say this is just as strong and perhaps even stronger. You have ERROR() for trailing white space. This is much worse than that.

This isn't a 80 character limit problem. This is something that if you add will be reverted right away, as it was for Juri's patch that added a trace_printk() by mistake, and Boris followed it with a patch to remove it.

-- Steve

Joe Perches

maintainer

2015-04-07 14:26 UTC #9 lore raw

Quoted 9 lines · Steven Rostedt

Quoted 3 lines · On Tue, 07 Apr 2015 07:01:37 -0700 Joe Perches <[email protected]> wrote:
o Please add a test for $realfile !~ m@kernel/trace/@ or maybe $realfile !~ /(?:trace|tracing)/
o ERROR seems a bit strong, WARN is probably good enough
I'm thinking ERROR is good. There's no reason to have it. In fact, you must never have it. Looking at the other ERROR() conditions, I say this

Look at trace_printk in fs/ext4/inline.c

It's in a section guarded by a CONFIG_FOO_DEBUG block. Is the use there an error? Perhaps not and I think it better if checkpatch ERROR messages are more definitive.

Quoted 2 lines

is just as strong and perhaps even stronger. You have ERROR() for trailing white space. This is much worse than that.

I'm not much of a fan of that one, nor of most of the ERROR uses in checkpatch actually.

I think it might be better if all of the checkpatch whitespace/style related messages were WARN not ERROR.

Steven Rostedt

reviewer

2015-04-07 14:34 UTC #10 lore raw

Quoted 4 lines · On Tue, 07 Apr 2015 07:26:13 -0700 Joe Perches <[email protected]> wrote:

Quoted 3 lines · Steven Rostedt
On Tue, 07 Apr 2015 07:01:37 -0700
Joe Perches <[email protected]> wrote:

Quoted 9 lines

Quoted 6 lines
Quoted 3 lines
o Please add a test for $realfile !~ m@kernel/trace/@ or maybe $realfile !~ /(?:trace|tracing)/
o ERROR seems a bit strong, WARN is probably good enough
I'm thinking ERROR is good. There's no reason to have it. In fact, you must never have it. Looking at the other ERROR() conditions, I say this
Look at trace_printk in fs/ext4/inline.c

Yeah, I've stumbled on that one before and looked and said to myself "WTF". But as it is masked around DEBUG, I let it slide. I still don't like it, because it really should be a tracepoint instead. The problem with trace_printk(), is that it is either all on or all off. You can not pick and choose, and they clutter the trace.

I may send patches to remove that anyway.

Quoted 5 lines

It's in a section guarded by a CONFIG_FOO_DEBUG block. Is the use there an error? Perhaps not and I think it better if checkpatch ERROR messages are more definitive.

Quoted 8 lines

Quoted 2 lines
is just as strong and perhaps even stronger. You have ERROR() for trailing white space. This is much worse than that.
I'm not much of a fan of that one, nor of most of the ERROR uses in checkpatch actually.
I think it might be better if all of the checkpatch whitespace/style related messages were WARN not ERROR.

I agree.

I still rather have this be an ERROR and not a WARN, because I rather avoid even those that encapsulate it with CONFIG_FOO_DEBUG.

-- Steve

Peter Zijlstra

maintainer

2015-04-07 14:39 UTC #11 lore raw

Quoted 2 lines · Steven Rostedt

I still rather have this be an ERROR and not a WARN, because I rather avoid even those that encapsulate it with CONFIG_FOO_DEBUG.

Lets look at it this way; Steve maintains that trace_printk() thing, if he says its an ERROR, it is.

Joe Perches

maintainer

2015-04-07 14:43 UTC #12 lore raw

In reply to Steven Rostedt

Quoted 4 lines · Steven Rostedt

Quoted 3 lines · On Tue, 07 Apr 2015 07:26:13 -0700 Joe Perches <[email protected]> wrote:
Quoted 2 lines · Steven Rostedt
Quoted 1 line · On Tue, 07 Apr 2015 07:01:37 -0700 Joe Perches <[email protected]> wrote:
o ERROR seems a bit strong, WARN is probably good enough

[]

Quoted 2 lines

I still rather have this be an ERROR and not a WARN, because I rather avoid even those that encapsulate it with CONFIG_FOO_DEBUG.

Whichever is chosen is fine with me.