Re: [PATCH] sched/core: Drop debugging leftover trace_printk call
On Tue, 2015-04-07 at 10:34 -0400, Steven Rostedt wrote: > On Tue, 07 Apr 2015 07:26:13 -0700 Joe Perches wrote: > > On Tue, 2015-04-07 at 10:10 -0400, Steven Rostedt wrote: > > > On Tue, 07 Apr 2015 07:01:37 -0700 Joe Perches 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sched/core: Drop debugging leftover trace_printk call
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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sched/core: Drop debugging leftover trace_printk call
On Tue, 07 Apr 2015 07:26:13 -0700 Joe Perches wrote: > On Tue, 2015-04-07 at 10:10 -0400, Steven Rostedt wrote: > > On Tue, 07 Apr 2015 07:01:37 -0700 > > Joe Perches 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sched/core: Drop debugging leftover trace_printk call
On Tue, 2015-04-07 at 10:10 -0400, Steven Rostedt wrote: > On Tue, 07 Apr 2015 07:01:37 -0700 > Joe Perches 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sched/core: Drop debugging leftover trace_printk call
On 07/04/15 14:56, Steven Rostedt wrote:
> On Tue, 07 Apr 2015 14:47:50 +0100
> Juri Lelli wrote:
>
>> On 03/04/2015 09:42, Borislav Petkov wrote:
>>> From: Borislav Petkov
>>>
>>> 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__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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sched/core: Drop debugging leftover trace_printk call
On Tue, 07 Apr 2015 07:01:37 -0700 Joe Perches 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sched/core: Drop debugging leftover trace_printk call
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__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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sched/core: Drop debugging leftover trace_printk call
On Tue, 07 Apr 2015 14:47:50 +0100
Juri Lelli wrote:
> On 03/04/2015 09:42, Borislav Petkov wrote:
> > From: Borislav Petkov
> >
> > 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__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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sched/core: Drop debugging leftover trace_printk call
On 03/04/2015 09:42, Borislav Petkov wrote:
> From: Borislav Petkov
>
> 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
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
---
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__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
> Cc: Juri Lelli
> Cc: Peter Zijlstra (Intel)
> Cc: Juri Lelli
> Cc: Ingo Molnar
> Cc: Steven Rostedt
> ---
> 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;
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sched/core: Drop debugging leftover trace_printk call
On Fri, Apr 03, 2015 at 09:24:01AM -0400, Steven Rostedt wrote:
> On Fri, 3 Apr 2015 10:42:50 +0200
> Borislav Petkov wrote:
>
> > From: Borislav Petkov
> >
> > 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.
--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sched/core: Drop debugging leftover trace_printk call
On Fri, 3 Apr 2015 10:42:50 +0200
Borislav Petkov wrote:
> From: Borislav Petkov
>
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] sched/core: Drop debugging leftover trace_printk call
From: Borislav Petkov
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
Cc: Juri Lelli
Cc: Peter Zijlstra (Intel)
Cc: Juri Lelli
Cc: Ingo Molnar
Cc: Steven Rostedt
---
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
