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(-)
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
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: Juri Lelli <[email protected]>
Cc: Peter Zijlstra (Intel) <[email protected]>
Cc: Steven Rostedt <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Ingo Molnar <[email protected]>
---
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;
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
On Fri, Apr 03, 2015 at 09:24:01AM -0400, Steven Rostedt wrote:
> 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 ;-)
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.
--
On 03/04/2015 09:42, Borislav Petkov 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.
>
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 <[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(+)
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 <[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(-)
>
> 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;
>
On Tue, 07 Apr 2015 14:47:50 +0100
Juri Lelli <[email protected]> wrote:
> On 03/04/2015 09:42, Borislav Petkov 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.
> >
>
> 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
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
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
On 07/04/15 14:56, Steven Rostedt wrote:
> On Tue, 07 Apr 2015 14:47:50 +0100
> Juri Lelli <[email protected]> wrote:
>
>> On 03/04/2015 09:42, Borislav Petkov 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.
>>>
>>
>> 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
>
On Tue, 2015-04-07 at 10:10 -0400, Steven Rostedt wrote:
> 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.
> 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.
On Tue, 07 Apr 2015 07:26:13 -0700
Joe Perches <[email protected]> wrote:
> On Tue, 2015-04-07 at 10:10 -0400, Steven Rostedt wrote:
> > 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
>
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
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.
On Tue, 2015-04-07 at 10:34 -0400, Steven Rostedt wrote:
> On Tue, 07 Apr 2015 07:26:13 -0700 Joe Perches <[email protected]> wrote:
> > On Tue, 2015-04-07 at 10:10 -0400, Steven Rostedt wrote:
> > > 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
[]
> 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.