[PATCH] sched/core: Drop debugging leftover trace_printk call

[PATCH] sched/core: Drop debugging leftover trace_printk call

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

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 <bp@suse.de>
Cc: Juri Lelli <juri.lelli@arm.com>
Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Juri Lelli <juri.lelli@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/sched/core.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index a27d38f5a464..dbfc93d40292 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -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;
-- 
2.3.3

[tip:sched/core] sched/core: Drop debugging leftover trace_printk call

[tip-bot for Borislav Petkov]

Commit-ID:  62a935b256f68a71697716595347209fb5275426
Gitweb:     http://git.kernel.org/tip/62a935b256f68a71697716595347209fb5275426
Author:     Borislav Petkov <bp@suse.de>
AuthorDate: Fri, 3 Apr 2015 10:42:50 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
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 <bp@suse.de>
Cc: Juri Lelli <juri.lelli@arm.com>
Cc: Juri Lelli <juri.lelli@gmail.com>
Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Link: http://lkml.kernel.org/r/1428050570-21041-1-git-send-email-bp@alien8.de
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/sched/core.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 28b0d75..8027cfd 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -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;

Re: [PATCH] sched/core: Drop debugging leftover trace_printk call

[Steven Rostedt]

On Fri,  3 Apr 2015 10:42:50 +0200
Borislav Petkov <bp@alien8.de> wrote:

> From: Borislav Petkov <bp@suse.de>
> 
> 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

Re: [PATCH] sched/core: Drop debugging leftover trace_printk call

[Borislav Petkov]

On Fri, Apr 03, 2015 at 09:24:01AM -0400, Steven Rostedt wrote:
> On Fri,  3 Apr 2015 10:42:50 +0200
> Borislav Petkov <bp@alien8.de> wrote:
> 
> > From: Borislav Petkov <bp@suse.de>
> > 
> > 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

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] sched/core: Drop debugging leftover trace_printk call

[Juri Lelli]

On 03/04/2015 09:42, Borislav Petkov wrote:
> From: Borislav Petkov <bp@suse.de>
> 
> 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

>From e5733b377d55fd760160fa0e7822bdefa4f3a2c4 Mon Sep 17 00:00:00 2001
From: Juri Lelli <juri.lelli@arm.com>
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 <juri.lelli@arm.com>
---
 scripts/checkpatch.pl | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index d124359..1fc454c5 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -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
-- 
2.3.0
 
> Signed-off-by: Borislav Petkov <bp@suse.de>
> Cc: Juri Lelli <juri.lelli@arm.com>
> Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
> Cc: Juri Lelli <juri.lelli@gmail.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> ---
>  kernel/sched/core.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index a27d38f5a464..dbfc93d40292 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -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;
> 

Re: [PATCH] sched/core: Drop debugging leftover trace_printk call

[Steven Rostedt]

On Tue, 07 Apr 2015 14:47:50 +0100
Juri Lelli <juri.lelli@arm.com> wrote:

> On 03/04/2015 09:42, Borislav Petkov wrote:
> > From: Borislav Petkov <bp@suse.de>
> > 
> > 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 :(.

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



> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index d124359..1fc454c5 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -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


>  # 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

Re: [PATCH] sched/core: Drop debugging leftover trace_printk call

[Joe Perches]

On Tue, 2015-04-07 at 14:47 +0100, Juri Lelli wrote:
> how about we add also something like this to checkpatch?

[]

> 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.
[]
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -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

Re: [PATCH] sched/core: Drop debugging leftover trace_printk call

[Steven Rostedt]

On Tue, 07 Apr 2015 07:01:37 -0700
Joe Perches <joe@perches.com> 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

Re: [PATCH] sched/core: Drop debugging leftover trace_printk call

[Juri Lelli]

On 07/04/15 14:56, Steven Rostedt wrote:
> On Tue, 07 Apr 2015 14:47:50 +0100
> Juri Lelli <juri.lelli@arm.com> wrote:
> 
>> On 03/04/2015 09:42, Borislav Petkov wrote:
>>> From: Borislav Petkov <bp@suse.de>
>>>
>>> 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 :(.
> 
> 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.

> 
> 
>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>> index d124359..1fc454c5 100755
>> --- a/scripts/checkpatch.pl
>> +++ b/scripts/checkpatch.pl
>> @@ -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.
> 

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

Thanks a lot,

- Juri

> -- Steve
> 
> 
>>  # 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
> 

Re: [PATCH] sched/core: Drop debugging leftover trace_printk call

[Joe Perches]

On Tue, 2015-04-07 at 10:10 -0400, Steven Rostedt wrote:
> On Tue, 07 Apr 2015 07:01:37 -0700
> Joe Perches <joe@perches.com> 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.

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

Re: [PATCH] sched/core: Drop debugging leftover trace_printk call

[Steven Rostedt]

On Tue, 07 Apr 2015 07:26:13 -0700
Joe Perches <joe@perches.com> wrote:

> On Tue, 2015-04-07 at 10:10 -0400, Steven Rostedt wrote:
> > On Tue, 07 Apr 2015 07:01:37 -0700
> > Joe Perches <joe@perches.com> 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
> 

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.

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

Re: [PATCH] sched/core: Drop debugging leftover trace_printk call

[Peter Zijlstra]

On Tue, Apr 07, 2015 at 10:34:04AM -0400, Steven Rostedt wrote:
> 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.

Re: [PATCH] sched/core: Drop debugging leftover trace_printk call

[Joe Perches]

On Tue, 2015-04-07 at 10:34 -0400, Steven Rostedt wrote:
> On Tue, 07 Apr 2015 07:26:13 -0700 Joe Perches <joe@perches.com> wrote:
> > On Tue, 2015-04-07 at 10:10 -0400, Steven Rostedt wrote:
> > > On Tue, 07 Apr 2015 07:01:37 -0700 Joe Perches <joe@perches.com> wrote:
> > > > o ERROR seems a bit strong, WARN is probably good enough
[]
> 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.