The following is a series of patches related to coding standards. The first
patch updates the CodingStandards document with respect to printk debugging
and un/likely use; the second patch updates the usage string for
checkpatch.pl; the third patch introduces a new small perl script that can
be used to check coding-standards compliance on multiple source files. This
new script, called check-coding-standards.pl, produces output in a standard
compiler-error format, which can be parsed by assorted tools including text
editors and help users locate the file that had the error and the offending
line-number more quickly.
Erez Zadok (3):
CodingStyle updates
Update usage string for checkpatch.pl
New script to check coding-style compliance on multiple regular files
Documentation/CodingStyle | 88 +++++++++++++++++++++++++++++++++++++-
scripts/check-coding-standards.pl | 59 +++++++++++++++++++++++++
scripts/checkpatch.pl | 1
3 files changed, 146 insertions(+), 2 deletions(-)
---
Erez Zadok
[email protected]
Signed-off-by: Erez Zadok <[email protected]>
---
scripts/checkpatch.pl | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index dae7d30..ecbb030 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -33,6 +33,7 @@ if ($#ARGV < 0) {
print "version: $V\n";
print "options: -q => quiet\n";
print " --no-tree => run without a kernel tree\n";
+ print " --no-signoff => don't check for signed-off-by\n";
exit(1);
}
--
1.5.2.2
The script calls checkpatch.pl on each file, and formats any error messages
to comply with standard compiler error messages:
file_name:line_number:error_message
This is particularly useful when run from within a text editor which can
parse these error messages and show the user a buffer with the file in
question, placing the cursor on the offending line (e.g., Emacs's "M-x
next-error" command, bound by default to C-x `).
Signed-off-by: Erez Zadok <[email protected]>
---
scripts/check-coding-standards.pl | 59 +++++++++++++++++++++++++++++++++++++
1 files changed, 59 insertions(+), 0 deletions(-)
create mode 100755 scripts/check-coding-standards.pl
diff --git a/scripts/check-coding-standards.pl b/scripts/check-coding-standards.pl
new file mode 100755
index 0000000..a1ba597
--- /dev/null
+++ b/scripts/check-coding-standards.pl
@@ -0,0 +1,59 @@
+#!/usr/bin/perl -w
+# (c) 2007, Erez Zadok <[email protected]>
+# Licensed under the terms of the GNU GPL License version 2
+#
+# Check one more more files for compliance with coding standards.
+# Outputs standard compiler-error format, one error per line, as follows
+# filename:lineno:errormsg
+# This standard error message can be parsed by various tools, including
+# Emacs's M-x next-error
+#
+# Usage: check-coding-standards.pl file [files...]
+
+
+if (!defined($ARGV[0])) {
+ printf("Usage: $0 file [files...]\n");
+ exit(1);
+}
+
+$msg="";
+$lineno=0;
+foreach $file (@ARGV) {
+ die "$file: $!" unless (-f $file);
+ open(FILE, "diff -u /dev/null $file | perl scripts/checkpatch.pl --no-signoff - |") || die "$file: $!";
+ while (($line = <FILE>)) {
+ chop $line;
+ next if ($line =~ /^$/);
+ if ($line =~ /^ERROR: /) {
+ $msg = $line;
+ next;
+ }
+ if ($line =~ /^WARNING: /) {
+ $msg = $line;
+ next;
+ }
+ if ($line =~ /^CHECK: /) {
+ $msg = $line;
+ next;
+ }
+ if ($line =~ /^#/) {
+ $lineno = (split(/:/, $line))[3];
+ next;
+ }
+ if ($line =~ /^\+/) {
+ if ($lineno > 0) {
+ printf(STDOUT "%s:%d:%s\n", $file, $lineno, $msg);
+ $msg="";
+ $lineno=0;
+ }
+ next;
+ }
+ next if ($line =~ /^\s+\^/);
+ next if ($line =~ /^Your patch has style problems, please review/);
+ next if ($line =~ /^are false positives report them to the/);
+ next if ($line =~ /^CHECKPATCH in MAINTAINERS/);
+ next if ($line =~ /^Your patch has no obvious style problems/);
+ die "unknown output from checkpatch: $line: $.";
+ }
+ close(FILE);
+}
--
1.5.2.2
1. Updates chapter 13 (printing kernel messages) to expand on the use of
pr_debug()/pr_info(), what to avoid, and how to hook your debug code with
kernel.h.
2. New chapter 19, branch prediction optimizations, discusses the whole
un/likely issue.
Cc: "Kok, Auke" <[email protected]>
Cc: Kyle Moffett <[email protected]>
Cc: Jan Engelhardt <[email protected]>
Cc: Adrian Bunk <[email protected]>
Cc: roel <[email protected]>
Signed-off-by: Erez Zadok <[email protected]>
---
Documentation/CodingStyle | 88 +++++++++++++++++++++++++++++++++++++++++++-
1 files changed, 86 insertions(+), 2 deletions(-)
diff --git a/Documentation/CodingStyle b/Documentation/CodingStyle
index 7f1730f..00b29e4 100644
--- a/Documentation/CodingStyle
+++ b/Documentation/CodingStyle
@@ -643,8 +643,26 @@ Printing numbers in parentheses (%d) adds no value and should be avoided.
There are a number of driver model diagnostic macros in <linux/device.h>
which you should use to make sure messages are matched to the right device
and driver, and are tagged with the right level: dev_err(), dev_warn(),
-dev_info(), and so forth. For messages that aren't associated with a
-particular device, <linux/kernel.h> defines pr_debug() and pr_info().
+dev_info(), and so forth.
+
+A number of people often like to define their own debugging printf's,
+wrapping printk's in #ifdef's that get turned on only when subsystem
+debugging is compiled in (e.g., dprintk, Dprintk, DPRINTK, etc.). Please
+don't reinvent the wheel but use existing mechanisms. For messages that
+aren't associated with a particular device, <linux/kernel.h> defines
+pr_debug() and pr_info(); the latter two translate to printk(KERN_DEBUG) and
+printk(KERN_INFO), respectively. However, to get pr_debug() to actually
+emit the message, you'll need to turn on DEBUG in your code, which can be
+done as follows in your subsystem Makefile:
+
+ifeq ($(CONFIG_WHATEVER_DEBUG),y)
+EXTRA_CFLAGS += -DDEBUG
+endif
+
+In this way, you can create a Kconfig parameter to turn on debugging at
+compile time, which will also turn on DEBUG, to enable pr_debug() to emit
+actual messages; conversely, when CONFIG_WHATEVER_DEBUG is off, DEBUG is
+off, and pr_debug() will display nothing.
Coming up with good debugging messages can be quite a challenge; and once
you have them, they can be a huge help for remote troubleshooting. Such
@@ -779,6 +797,69 @@ includes markers for indentation and mode configuration. People may use their
own custom mode, or may have some other magic method for making indentation
work correctly.
+ Chapter 19: branch prediction optimizations
+
+The kernel includes macros called likely() and unlikely(), which can be used
+as hints to the compiler to optimize branch prediction. They operate by
+asking gcc to shuffle the code around so that the more favorable outcome
+executes linearly, avoiding a JMP instruction; this can improve cache
+pipeline efficiency. For technical details how these macros work, see the
+References section at the end of this document.
+
+An example use of this as as follows:
+
+ ptr = kmalloc(size, GFP_KERNEL);
+ if (unlikely(!ptr))
+ ...
+
+or
+ err = some_function(...);
+ if (likely(!err))
+ ...
+
+The main two problems with using un/likely() are that (a) programmers can
+easily be wrong about their code's likelihood to take one branch
+vs. another, and (b) on average, gcc will do a much better job optimizing
+branches that the programmer can. The benefit on some systems for
+predicting correctly can be in saving a few instructions. But the penalty
+for wrong use of un/likely() can be very significant (possibly dozens of
+instructions), as you may be doing a JMP instruction, hurting your pipeline
+cache, EACH time you get to the branch in question!
+
+Therefore, use un/likely() sparingly, consider it primarily for hot paths,
+use it only when you are certain that the condition in question rarely
+happens, be sure that it happens with roughly the same probability under
+most/all user conditions. One rule of thumb suggested is that the
+probability of the branch un/taken should exceed 99% (although some would
+consider 95% as well). Of course, beware of silly mistakes such as
+intending to use likely() and using unlikely() instead.
+
+A good example of this is the above kmalloc(GFP_KERNEL) call. The chances
+of kmalloc() returning NULL are rather small, because even if it doesn't
+have memory to return to you at the moment, with GFP_KERNEL/__GFP_WAIT
+passed, kmalloc() will wait and suspend your thread, while it goes off to
+find some free memory (purging caches, flushing buffers, etc.). In other
+words, kmalloc() tries very hard to give you the memory you asked for by the
+time it return.
+
+Consider the next, bad example. Suppose you're developing a file system
+which performs logically different actions on different types of entities:
+files, directories, symlinks, devices, etc. and you use this code:
+
+ if (unlikely(S_ISBLK(mode))
+ ...
+
+On first glance, the above use of unlikely() seems right. After all, most
+file system objects are files and directories, and very few of them tend to
+be block devices. So this optimization should work well, no? Although it's
+true that it'll work well for most users, what about some user who happens
+to have a file system with lots of block devices? Or what if the user has
+only one block device object on the file system, but the user has an
+application which causes the above conditional to be traversed very
+frequently (e.g., a shell script that deals with devices)? Such users will
+be penalized heavily for going [sic] down the wrong path... Therefore, you
+should consider also whether a seemingly-rare condition is indeed rare ALL
+the time.
Appendix I: References
@@ -804,6 +885,9 @@ language C, URL: http://www.open-std.org/JTC1/SC22/WG14/
Kernel CodingStyle, by [email protected] at OLS 2002:
http://www.kroah.com/linux/talks/ols_2002_kernel_codingstyle_talk/html/
+FAQ/LikelyUnlikely:
+http://kernelnewbies.org/FAQ/LikelyUnlikely
+
--
Last updated on 2007-July-13.
--
1.5.2.2
On Fri, 28 Sep 2007 17:32:00 -0400 Erez Zadok wrote:
> 1. Updates chapter 13 (printing kernel messages) to expand on the use of
> pr_debug()/pr_info(), what to avoid, and how to hook your debug code with
> kernel.h.
>
> 2. New chapter 19, branch prediction optimizations, discusses the whole
> un/likely issue.
>
> Cc: "Kok, Auke" <[email protected]>
> Cc: Kyle Moffett <[email protected]>
> Cc: Jan Engelhardt <[email protected]>
> Cc: Adrian Bunk <[email protected]>
> Cc: roel <[email protected]>
>
> Signed-off-by: Erez Zadok <[email protected]>
A few comments below...
Acked-by: Randy Dunlap <[email protected]>
> ---
> Documentation/CodingStyle | 88 +++++++++++++++++++++++++++++++++++++++++++-
> 1 files changed, 86 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/CodingStyle b/Documentation/CodingStyle
> index 7f1730f..00b29e4 100644
> --- a/Documentation/CodingStyle
> +++ b/Documentation/CodingStyle
...
> @@ -779,6 +797,69 @@ includes markers for indentation and mode configuration. People may use their
> own custom mode, or may have some other magic method for making indentation
> work correctly.
>
> + Chapter 19: branch prediction optimizations
> +
> +The kernel includes macros called likely() and unlikely(), which can be used
> +as hints to the compiler to optimize branch prediction. They operate by
> +asking gcc to shuffle the code around so that the more favorable outcome
> +executes linearly, avoiding a JMP instruction; this can improve cache
> +pipeline efficiency. For technical details how these macros work, see the
> +References section at the end of this document.
> +
> +An example use of this as as follows:
> +
> + ptr = kmalloc(size, GFP_KERNEL);
> + if (unlikely(!ptr))
> + ...
> +
> +or
> + err = some_function(...);
> + if (likely(!err))
> + ...
> +
> +The main two problems with using un/likely() are that (a) programmers can
> +easily be wrong about their code's likelihood to take one branch
> +vs. another, and (b) on average, gcc will do a much better job optimizing
> +branches that the programmer can. The benefit on some systems for
> +predicting correctly can be in saving a few instructions. But the penalty
> +for wrong use of un/likely() can be very significant (possibly dozens of
> +instructions), as you may be doing a JMP instruction, hurting your pipeline
> +cache, EACH time you get to the branch in question!
> +
> +Therefore, use un/likely() sparingly, consider it primarily for hot paths,
> +use it only when you are certain that the condition in question rarely
> +happens, be sure that it happens with roughly the same probability under
> +most/all user conditions. One rule of thumb suggested is that the
> +probability of the branch un/taken should exceed 99% (although some would
> +consider 95% as well). Of course, beware of silly mistakes such as
> +intending to use likely() and using unlikely() instead.
> +
> +A good example of this is the above kmalloc(GFP_KERNEL) call. The chances
> +of kmalloc() returning NULL are rather small, because even if it doesn't
> +have memory to return to you at the moment, with GFP_KERNEL/__GFP_WAIT
> +passed, kmalloc() will wait and suspend your thread, while it goes off to
> +find some free memory (purging caches, flushing buffers, etc.). In other
> +words, kmalloc() tries very hard to give you the memory you asked for by the
> +time it return.
returns.
> +
> +Consider the next, bad example. Suppose you're developing a file system
^comma seems odd here
> +which performs logically different actions on different types of entities:
> +files, directories, symlinks, devices, etc. and you use this code:
> +
> + if (unlikely(S_ISBLK(mode))
> + ...
> +
> +On first glance, the above use of unlikely() seems right. After all, most
> +file system objects are files and directories, and very few of them tend to
> +be block devices. So this optimization should work well, no? Although it's
> +true that it'll work well for most users, what about some user who happens
> +to have a file system with lots of block devices? Or what if the user has
> +only one block device object on the file system, but the user has an
> +application which causes the above conditional to be traversed very
> +frequently (e.g., a shell script that deals with devices)? Such users will
> +be penalized heavily for going [sic] down the wrong path... Therefore, you
> +should consider also whether a seemingly-rare condition is indeed rare ALL
> +the time.
>
>
> Appendix I: References
> @@ -804,6 +885,9 @@ language C, URL: http://www.open-std.org/JTC1/SC22/WG14/
> Kernel CodingStyle, by [email protected] at OLS 2002:
> http://www.kroah.com/linux/talks/ols_2002_kernel_codingstyle_talk/html/
>
> +FAQ/LikelyUnlikely:
> +http://kernelnewbies.org/FAQ/LikelyUnlikely
> +
> --
---
~Randy
Phaedrus says that Quality is about caring.
Hi Erez.
On Fri, Sep 28, 2007 at 05:32:02PM -0400, Erez Zadok wrote:
> The script calls checkpatch.pl on each file, and formats any error messages
> to comply with standard compiler error messages:
>
> file_name:line_number:error_message
>
> This is particularly useful when run from within a text editor which can
> parse these error messages and show the user a buffer with the file in
> question, placing the cursor on the offending line (e.g., Emacs's "M-x
> next-error" command, bound by default to C-x `).
The concept is wrong here. If we want checkpatch to generate
message in standard gcc format then we should fix checkpatch and
not add a wrapper script around it.
For checkpatch related patches please forward them to the checkpatch
maintainers.
>From MAINTAINERS:
CHECKPATCH
P: Andy Whitcroft
M: [email protected]
P: Randy Dunlap
M: [email protected]
P: Joel Schopp
M: [email protected]
S: Supported
Sam
On Fri, Sep 28, 2007 at 05:32:00PM -0400, Erez Zadok wrote:
> 1. Updates chapter 13 (printing kernel messages) to expand on the use of
> pr_debug()/pr_info(), what to avoid, and how to hook your debug code with
> kernel.h.
>
> 2. New chapter 19, branch prediction optimizations, discusses the whole
> un/likely issue.
>
> Cc: "Kok, Auke" <[email protected]>
> Cc: Kyle Moffett <[email protected]>
> Cc: Jan Engelhardt <[email protected]>
> Cc: Adrian Bunk <[email protected]>
> Cc: roel <[email protected]>
>
> Signed-off-by: Erez Zadok <[email protected]>
> ---
> Documentation/CodingStyle | 88 +++++++++++++++++++++++++++++++++++++++++++-
> 1 files changed, 86 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/CodingStyle b/Documentation/CodingStyle
> index 7f1730f..00b29e4 100644
> --- a/Documentation/CodingStyle
> +++ b/Documentation/CodingStyle
> @@ -643,8 +643,26 @@ Printing numbers in parentheses (%d) adds no value and should be avoided.
> There are a number of driver model diagnostic macros in <linux/device.h>
> which you should use to make sure messages are matched to the right device
> and driver, and are tagged with the right level: dev_err(), dev_warn(),
> -dev_info(), and so forth. For messages that aren't associated with a
> -particular device, <linux/kernel.h> defines pr_debug() and pr_info().
> +dev_info(), and so forth.
> +
> +A number of people often like to define their own debugging printf's,
> +wrapping printk's in #ifdef's that get turned on only when subsystem
> +debugging is compiled in (e.g., dprintk, Dprintk, DPRINTK, etc.). Please
> +don't reinvent the wheel but use existing mechanisms. For messages that
> +aren't associated with a particular device, <linux/kernel.h> defines
> +pr_debug() and pr_info(); the latter two translate to printk(KERN_DEBUG) and
The latter two? Since there are only two presented I think there is no
reason to say "latter".
> +printk(KERN_INFO), respectively. However, to get pr_debug() to actually
> +emit the message, you'll need to turn on DEBUG in your code, which can be
> +done as follows in your subsystem Makefile:
> +
> +ifeq ($(CONFIG_WHATEVER_DEBUG),y)
> +EXTRA_CFLAGS += -DDEBUG
> +endif
> +
> +In this way, you can create a Kconfig parameter to turn on debugging at
> +compile time, which will also turn on DEBUG, to enable pr_debug() to emit
> +actual messages; conversely, when CONFIG_WHATEVER_DEBUG is off, DEBUG is
> +off, and pr_debug() will display nothing.
>
> Coming up with good debugging messages can be quite a challenge; and once
> you have them, they can be a huge help for remote troubleshooting. Such
> @@ -779,6 +797,69 @@ includes markers for indentation and mode configuration. People may use their
> own custom mode, or may have some other magic method for making indentation
> work correctly.
>
> + Chapter 19: branch prediction optimizations
> +
> +The kernel includes macros called likely() and unlikely(), which can be used
> +as hints to the compiler to optimize branch prediction. They operate by
> +asking gcc to shuffle the code around so that the more favorable outcome
> +executes linearly, avoiding a JMP instruction; this can improve cache
> +pipeline efficiency. For technical details how these macros work, see the
> +References section at the end of this document.
> +
> +An example use of this as as follows:
^^^^^^
> +
> + ptr = kmalloc(size, GFP_KERNEL);
> + if (unlikely(!ptr))
> + ...
> +
> +or
> + err = some_function(...);
> + if (likely(!err))
> + ...
--
Shawn
A couple of comments interspersed...
On 9/28/07, Erez Zadok <[email protected]> wrote:
...
> There are a number of driver model diagnostic macros in <linux/device.h>
> which you should use to make sure messages are matched to the right device
---
I think this "which" is non-restrictive, so it should have a comma
after it (I realize that's not part of your patch). It's also possible
to read it as restrictive, in which case "that" would be preferable.
---
> and driver, and are tagged with the right level: dev_err(), dev_warn(),
> -dev_info(), and so forth. For messages that aren't associated with a
> -particular device, <linux/kernel.h> defines pr_debug() and pr_info().
> +dev_info(), and so forth.
> +
> +A number of people often like to define their own debugging printf's,
---
"A number of people often like to..." is awkward. How about
"Developers sometimes..." or "Too many people..."
---
> +wrapping printk's in #ifdef's that get turned on only when subsystem
> + Chapter 19: branch prediction optimizations
> +
> +The kernel includes macros called likely() and unlikely(), which can be used
---
You might say "The kernel provides the macros likely()..."
---
> +as hints to the compiler to optimize branch prediction. They operate by
...
> +A good example of this is the above kmalloc(GFP_KERNEL) call. The chances
> +of kmalloc() returning NULL are rather small, because even if it doesn't
> +have memory to return to you at the moment, with GFP_KERNEL/__GFP_WAIT
> +passed, kmalloc() will wait and suspend your thread, while it goes off to
---
The commas after "passed" and "thread" is unnecessary.
---
> +find some free memory (purging caches, flushing buffers, etc.). In other
> +words, kmalloc() tries very hard to give you the memory you asked for by the
> +time it return.
---
"...by the time it return." should be "...by the time it returns."
---
> +
> +Consider the next, bad example. Suppose you're developing a file system
> +which performs logically different actions on different types of entities:
---
This "which" is restrictive; it would be preferable to use "that" instead.
---
> +files, directories, symlinks, devices, etc. and you use this code:
> +
...
> +be penalized heavily for going [sic] down the wrong path... Therefore, you
> +should consider also whether a seemingly-rare condition is indeed rare ALL
---
The hyphen isn't necessary when the first word of the compount
adjective is an adverb ending in "-ly", so, just "seemingly rare"; or
switch to "apparently rare".
---
> +the time.
...
--
scott preece
On Fri, 28 Sep 2007 17:32:00 -0400 Erez Zadok wrote:
> 1. Updates chapter 13 (printing kernel messages) to expand on the use of
> pr_debug()/pr_info(), what to avoid, and how to hook your debug code with
> kernel.h.
>
> Signed-off-by: Erez Zadok <[email protected]>
> ---
> Documentation/CodingStyle | 88 +++++++++++++++++++++++++++++++++++++++++++-
> 1 files changed, 86 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/CodingStyle b/Documentation/CodingStyle
> index 7f1730f..00b29e4 100644
> --- a/Documentation/CodingStyle
> +++ b/Documentation/CodingStyle
> @@ -643,8 +643,26 @@ Printing numbers in parentheses (%d) adds no value and should be avoided.
> There are a number of driver model diagnostic macros in <linux/device.h>
> which you should use to make sure messages are matched to the right device
> and driver, and are tagged with the right level: dev_err(), dev_warn(),
> -dev_info(), and so forth. For messages that aren't associated with a
> -particular device, <linux/kernel.h> defines pr_debug() and pr_info().
> +dev_info(), and so forth.
> +
> +A number of people often like to define their own debugging printf's,
> +wrapping printk's in #ifdef's that get turned on only when subsystem
> +debugging is compiled in (e.g., dprintk, Dprintk, DPRINTK, etc.). Please
> +don't reinvent the wheel but use existing mechanisms. For messages that
> +aren't associated with a particular device, <linux/kernel.h> defines
> +pr_debug() and pr_info(); the latter two translate to printk(KERN_DEBUG) and
> +printk(KERN_INFO), respectively. However, to get pr_debug() to actually
> +emit the message, you'll need to turn on DEBUG in your code, which can be
> +done as follows in your subsystem Makefile:
> +
> +ifeq ($(CONFIG_WHATEVER_DEBUG),y)
> +EXTRA_CFLAGS += -DDEBUG
> +endif
Alternatively, that can be done in your source file, but doing this
in the Makefile makes good sense if you have more than one source file.
At any rate, it is done in some source files, and when it's done in
source files, #define-ing DEBUG should be done before #include
<linux/kernel.h> so that the macros are defined as expected.
> +In this way, you can create a Kconfig parameter to turn on debugging at
> +compile time, which will also turn on DEBUG, to enable pr_debug() to emit
> +actual messages; conversely, when CONFIG_WHATEVER_DEBUG is off, DEBUG is
> +off, and pr_debug() will display nothing.
>
> Coming up with good debugging messages can be quite a challenge; and once
> you have them, they can be a huge help for remote troubleshooting. Such
---
~Randy
Phaedrus says that Quality is about caring.
On Fri, 28 Sep 2007, Erez Zadok wrote:
>
> Documentation/CodingStyle | 88 +++++++++++++++++++++++++++++++++++++-
I'm not very happy with this.
"CodingStyle" should be about the big issues, not about details. Yes,
we've messed that up over the years, but let's not continue that.
In other words, I'd suggest *removing* lines from CodingStyle, not adding
them. The file has already gone from a "good general principles" to "lots
of stupid details". Let's not make it worse.
Linus
On Sat, Sep 29, 2007 at 11:01:29AM -0700, Randy Dunlap wrote:
> On Fri, 28 Sep 2007 17:32:00 -0400 Erez Zadok wrote:
>
> > 1. Updates chapter 13 (printing kernel messages) to expand on the use of
> > pr_debug()/pr_info(), what to avoid, and how to hook your debug code with
> > kernel.h.
> >
> > Signed-off-by: Erez Zadok <[email protected]>
> > ---
> > Documentation/CodingStyle | 88 +++++++++++++++++++++++++++++++++++++++++++-
> > 1 files changed, 86 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/CodingStyle b/Documentation/CodingStyle
> > index 7f1730f..00b29e4 100644
> > --- a/Documentation/CodingStyle
> > +++ b/Documentation/CodingStyle
> > @@ -643,8 +643,26 @@ Printing numbers in parentheses (%d) adds no value and should be avoided.
> > There are a number of driver model diagnostic macros in <linux/device.h>
> > which you should use to make sure messages are matched to the right device
> > and driver, and are tagged with the right level: dev_err(), dev_warn(),
> > -dev_info(), and so forth. For messages that aren't associated with a
> > -particular device, <linux/kernel.h> defines pr_debug() and pr_info().
> > +dev_info(), and so forth.
> > +
> > +A number of people often like to define their own debugging printf's,
> > +wrapping printk's in #ifdef's that get turned on only when subsystem
> > +debugging is compiled in (e.g., dprintk, Dprintk, DPRINTK, etc.). Please
> > +don't reinvent the wheel but use existing mechanisms. For messages that
> > +aren't associated with a particular device, <linux/kernel.h> defines
> > +pr_debug() and pr_info(); the latter two translate to printk(KERN_DEBUG) and
> > +printk(KERN_INFO), respectively. However, to get pr_debug() to actually
> > +emit the message, you'll need to turn on DEBUG in your code, which can be
> > +done as follows in your subsystem Makefile:
> > +
> > +ifeq ($(CONFIG_WHATEVER_DEBUG),y)
> > +EXTRA_CFLAGS += -DDEBUG
> > +endif
The abvoe hurst my eyes each time I see it.
Lately I have considered extending the kbuild syntax a bit.
Introducing
ccflags-y
asflags-y
[with same functionality as the EXTRA_CFLAGS, EXTRA_AFLAGS]
would allow us to do:
ccflags-$(CONFIG_WHATEVER_DEBUG) := -DDEBUG
Much nicer IMHO.
Sam
On Sat, Sep 29, 2007 at 11:18:05AM -0700, Linus Torvalds wrote:
> I'm not very happy with this.
>
> "CodingStyle" should be about the big issues, not about details. Yes,
> we've messed that up over the years, but let's not continue that.
>
> In other words, I'd suggest *removing* lines from CodingStyle, not adding
> them. The file has already gone from a "good general principles" to "lots
> of stupid details". Let's not make it worse.
It'd be nice to split the current CodingStyle into two documents:
- A shorter CodingStyle that gives the spirit of the style
(short functions, minimal nesting, logic as straightforward as
possible, etc.), and addresses the most commonly repeated
mistakes, without so much detail that people's eyes glaze
over. You want to be able to recommend it to your students
(or whoever) in reasonable confidence that they'll actually
read it and have fun (leave the jokes in!). Currently I'm
suspicious that it's becoming something that everybody
recommends but noone bothers to sit down and read anymore
unless they're working on it.
- A CodingStyleReference that's just a long dry list of rules,
organized to make it easy to look up an individual rule when
needed. That'd also take the pressure of CodingStyle to
accept every new detail.
It'd be a start just to revert CodingStyle to its original content and
move the rest to CodingStyleReference. But someone would want to skim
through the CodingStyle history for any legimate corrections that we
want to keep.
--b.
On Sat, 29 Sep 2007 15:56:38 -0400 J. Bruce Fields wrote:
> On Sat, Sep 29, 2007 at 11:18:05AM -0700, Linus Torvalds wrote:
> > I'm not very happy with this.
> >
> > "CodingStyle" should be about the big issues, not about details. Yes,
> > we've messed that up over the years, but let's not continue that.
> >
> > In other words, I'd suggest *removing* lines from CodingStyle, not adding
> > them. The file has already gone from a "good general principles" to "lots
> > of stupid details". Let's not make it worse.
>
> It'd be nice to split the current CodingStyle into two documents:
I agree. This is just what I was thinking during lunch.
> - A shorter CodingStyle that gives the spirit of the style
> (short functions, minimal nesting, logic as straightforward as
> possible, etc.), and addresses the most commonly repeated
> mistakes, without so much detail that people's eyes glaze
> over. You want to be able to recommend it to your students
> (or whoever) in reasonable confidence that they'll actually
> read it and have fun (leave the jokes in!). Currently I'm
> suspicious that it's becoming something that everybody
> recommends but noone bothers to sit down and read anymore
> unless they're working on it.
>
> - A CodingStyleReference that's just a long dry list of rules,
> organized to make it easy to look up an individual rule when
> needed. That'd also take the pressure of CodingStyle to
> accept every new detail.
>
> It'd be a start just to revert CodingStyle to its original content and
> move the rest to CodingStyleReference. But someone would want to skim
> through the CodingStyle history for any legimate corrections that we
> want to keep.
---
~Randy
Phaedrus says that Quality is about caring.
Hi Sam,
On Saturday 29 September 2007, Sam Ravnborg wrote:
> Lately I have considered extending the kbuild syntax a bit.
>
> Introducing
> ccflags-y
> asflags-y
>
> [with same functionality as the EXTRA_CFLAGS, EXTRA_AFLAGS]
> would allow us to do:
>
> ccflags-$(CONFIG_WHATEVER_DEBUG) := -DDEBUG
Please do!
That is very useful for testing and developing new modules.
I learnt a lot from you in this regard and used that kind
of syntax to the extreme in some other non-kernel project
of mine. There it included also ccflags, asflags and so on.
I further split that into -debug-y and -optimize-y flags,
but that was just for my own convenience.
Best Regards
Ingo Oeser
> > Lately I have considered extending the kbuild syntax a bit.
> >
> > Introducing
> > ccflags-y
> > asflags-y
> >
> > [with same functionality as the EXTRA_CFLAGS, EXTRA_AFLAGS]
> > would allow us to do:
> >
> > ccflags-$(CONFIG_WHATEVER_DEBUG) := -DDEBUG
>
> Please do!
>
> That is very useful for testing and developing new modules.
> I learnt a lot from you in this regard and used that kind
> of syntax to the extreme in some other non-kernel project
> of mine. There it included also ccflags, asflags and so on.
>
> I further split that into -debug-y and -optimize-y flags,
> but that was just for my own convenience.
Thanks for feedback Ingo.
I just started a new thread were I outlined the ideas I have
for further extending (some may say uglifying) the kbuild syntax.
I will see the outcome of this thread before implmenting something.
PS. The implementation part is obviously trivial, the
hard part is the documentation :-)
Sam
On Sat, 29 Sep 2007, Linus Torvalds wrote:
>
>
> On Fri, 28 Sep 2007, Erez Zadok wrote:
> >
> > Documentation/CodingStyle | 88 +++++++++++++++++++++++++++++++++++++-
>
> I'm not very happy with this.
>
> "CodingStyle" should be about the big issues, not about details.
> Yes, we've messed that up over the years, but let's not continue
> that.
>
> In other words, I'd suggest *removing* lines from CodingStyle, not
> adding them. The file has already gone from a "good general
> principles" to "lots of stupid details". Let's not make it worse.
here's a radical idea -- what about splitting the content across two
documents? you know ... perhaps "coding style aesthetics" versus
"coding distinctions" or something like that.
oh, wait ...
http://lkml.org/lkml/2007/1/1/18
:-)
rday
--
========================================================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry
Waterloo, Ontario, CANADA
http://crashcourse.ca
========================================================================
In message <[email protected]>, Linus Torvalds writes:
>
>
> On Fri, 28 Sep 2007, Erez Zadok wrote:
> >
> > Documentation/CodingStyle | 88 +++++++++++++++++++++++++++++++++++++-
>
> I'm not very happy with this.
>
> "CodingStyle" should be about the big issues, not about details. Yes,
> we've messed that up over the years, but let's not continue that.
>
> In other words, I'd suggest *removing* lines from CodingStyle, not adding
> them. The file has already gone from a "good general principles" to "lots
> of stupid details". Let's not make it worse.
>
> Linus
There's a lot of good value in having all those details, as it helps people
standardize on more common practices, including details. I think removing
all those details may only increase the amount-of less-accepted code to be
posted, resulting in more lkml people having to repeat themselves on what
not to do. Only now, they won't be able to point people to the CodingStyle
file for what they should do right.
Would you prefer if CodingStyle was reorganized or even split into (1)
general principles and (2) details? Perhaps we need a CodingStylePrinciples
and a CodingStyleDetails?
IOW, where should that very useful info live?
Erez.
On Sat, 29 Sep 2007, Erez Zadok wrote:
>
> Would you prefer if CodingStyle was reorganized or even split into (1)
> general principles and (2) details? Perhaps we need a CodingStylePrinciples
> and a CodingStyleDetails?
I'm certainly ok with the split into two files.
What I'm not ok with is really important stuff (indentation), and then
mixing in silly rules ("parenthesis are bad in printk's"?)
Linus
On Sat, Sep 29, 2007 at 03:56:38PM -0400, J. Bruce Fields wrote:
> It'd be a start just to revert CodingStyle to its original content and
> move the rest to CodingStyleReference. But someone would want to skim
> through the CodingStyle history for any legimate corrections that we
> want to keep.
How about CodingStyleSuggestions? And let's make it clear they are
suggestions, and not things that checkpatch should be flaming about
unless people request the all of the annoying associated false
positives via --spam-me-harder. :-)
- Ted
On Sat, 29 Sep 2007 11:18:05 PDT, Linus Torvalds said:
> "CodingStyle" should be about the big issues, not about details. Yes,
> we've messed that up over the years, but let's not continue that.
>
> In other words, I'd suggest *removing* lines from CodingStyle, not adding
> them. The file has already gone from a "good general principles" to "lots
> of stupid details". Let's not make it worse.
I think there needs to be a "sense of fairness" attached here - CodingStyle
should cover all the stuff maintainers/reviewers are allowed to whinge about.
Otherwise, we get maintainers whinging about undocumented style rules, and
we get code submitters who are unhappy because they have no real way to tell
if their code really *is* stylistically lacking, or if they just have a
jerk maintainer who wants to keep their code out-of-tree for political
reasons.
On Sat, 29 Sep 2007, [email protected] wrote:
>
> I think there needs to be a "sense of fairness" attached here - CodingStyle
> should cover all the stuff maintainers/reviewers are allowed to whinge about.
No.
People whine too much as is. Don't give them *license* to do so.
Linus
On Sat, Sep 29, 2007 at 10:24:01PM -0400, [email protected] wrote:
> On Sat, 29 Sep 2007 11:18:05 PDT, Linus Torvalds said:
>
> > "CodingStyle" should be about the big issues, not about details. Yes,
> > we've messed that up over the years, but let's not continue that.
> >
> > In other words, I'd suggest *removing* lines from CodingStyle, not adding
> > them. The file has already gone from a "good general principles" to "lots
> > of stupid details". Let's not make it worse.
>
> I think there needs to be a "sense of fairness" attached here - CodingStyle
> should cover all the stuff maintainers/reviewers are allowed to whinge about.
> Otherwise, we get maintainers whinging about undocumented style rules, and
> we get code submitters who are unhappy because they have no real way to tell
> if their code really *is* stylistically lacking, or if they just have a
> jerk maintainer who wants to keep their code out-of-tree for political
> reasons.
... and no matter how many rules you put down, it's still possible to
write a code that will be awful stylistically while adhering to all of
them. Religiously.
IOW, it's not going to eliminate that kind of fights.
In message <[email protected]>, Theodore Tso writes:
> On Sat, Sep 29, 2007 at 03:56:38PM -0400, J. Bruce Fields wrote:
> > It'd be a start just to revert CodingStyle to its original content and
> > move the rest to CodingStyleReference. But someone would want to skim
> > through the CodingStyle history for any legimate corrections that we
> > want to keep.
>
> How about CodingStyleSuggestions? And let's make it clear they are
> suggestions, and not things that checkpatch should be flaming about
> unless people request the all of the annoying associated false
> positives via --spam-me-harder. :-)
FWIW, the checkpatch script is woefully out of sync with CodingStyle. I
assume that checkpatch.pl is generally more authoritative (sans "(%d)" :-)
> - Ted
Erez.
On Sat, 29 Sep 2007 20:00:01 PDT, Linus Torvalds said:
> On Sat, 29 Sep 2007, [email protected] wrote:
> > I think there needs to be a "sense of fairness" attached here - CodingStyle
> > should cover all the stuff maintainers/reviewers are allowed to whinge about.
>
> No.
>
> People whine too much as is. Don't give them *license* to do so.
I kind of meant the *maintainers* couldn't whine about things not in CodingStyle ;)
On Sat, 29 Sep 2007, [email protected] wrote:
>
> I kind of meant the *maintainers* couldn't whine about things not in
> CodingStyle ;)
No, I understood you.
I'm personally of the opinion that the automated style checking, and
having detailed rules is always a mistake.
It's *much* better to teach people the big things. And the small things
will vary from person to person _anyway_, so it's not like you can really
document them.
Linus
On Sun, 30 Sep 2007 04:27:24 BST, Al Viro said:
> ... and no matter how many rules you put down, it's still possible to
> write a code that will be awful stylistically while adhering to all of
> them. Religiously.
I've run into those sort of programmers. Unfortunately, there's no real
cure for them short of a baseball/cricket bat or similar implement.
In message <[email protected]>, Linus Torvalds writes:
>
>
> On Sat, 29 Sep 2007, Erez Zadok wrote:
> >
> > Would you prefer if CodingStyle was reorganized or even split into (1)
> > general principles and (2) details? Perhaps we need a CodingStylePrinciples
> > and a CodingStyleDetails?
>
> I'm certainly ok with the split into two files.
>
> What I'm not ok with is really important stuff (indentation), and then
> mixing in silly rules ("parenthesis are bad in printk's"?)
>
> Linus
OK, looking at CodingStyle, I see two kinds of chapters. The first is stuff
that's more generic to C, and the other is more specific to Linux and how
one codes in the linux kernel. So I propose the following:
1. we create a new file called CodingSuggestions
2. we keep in CodingStyle the following chapters
Chapter 1: Indentation
Chapter 2: Breaking long lines and strings
Chapter 3: Placing Braces and Spaces
Chapter 4: Naming
Chapter 5: Typedefs
Chapter 6: Functions
Chapter 7: Centralized exiting of functions
Chapter 8: Commenting
Chapter 9: You've made a mess of it
Note: I'd suggest we rename the title of ch9 to "Custom Editor
Programming/Indentation Modes" or something more descriptive.
Chapter 10: Kconfig configuration files
Chapter 11: Data structures
Chapter 12: Macros, Enums and RTL
Chapter 15: The inline disease
Chapter 16: Function return values and names
Chapter 18: Editor modelines and other cruft
3. move the following chapters to CodingSuggestions:
Chapter 13: Printing kernel messages
Note: ch13 is the one which mentions the don't put parentheses around %d.
Chapter 14: Allocating memory
Chapter 17: Don't re-invent the kernel macros
Chapter 19: branch prediction optimizations (the un/likely debacle)
4. We go through checkpatch.pl and ensure that every test in checkpatch is
documented either in CodingStyle or in CodingSuggestions, determined by
whether checkpatch considers it an ERROR, WARNING, or just CHECK.
I think the above chapter split will be a reasonable start, and of course we
can tweak things over time. The general idea is that CodingStyle will
remain largely unchanged (until such day as the kernel is rewritten in Java
:-), while CodingSuggestions will evolve over time and be kept in sync with
checkpatch.
But, there's something really nice abuot having to point people to just one
file instead of two. We could also keep it all in one file, but split it
into two parts:
Part 1: Mandatory Coding Style
Chapter 1: indentation
...
Part 2: Coding Style Suggestions
Chapter n: printing kernel messages
...
Erez.
Erez Zadok wrote:
> In message <[email protected]>, Linus Torvalds writes:
>>
>> On Sat, 29 Sep 2007, Erez Zadok wrote:
>>> Would you prefer if CodingStyle was reorganized or even split into (1)
>>> general principles and (2) details? Perhaps we need a CodingStylePrinciples
>>> and a CodingStyleDetails?
>> I'm certainly ok with the split into two files.
>>
>> What I'm not ok with is really important stuff (indentation), and then
>> mixing in silly rules ("parenthesis are bad in printk's"?)
>>
>> Linus
>
> OK, looking at CodingStyle, I see two kinds of chapters. The first is stuff
> that's more generic to C, and the other is more specific to Linux and how
> one codes in the linux kernel. So I propose the following:
>
> 1. we create a new file called CodingSuggestions
>
> 2. we keep in CodingStyle the following chapters
>
> Chapter 1: Indentation
> Chapter 2: Breaking long lines and strings
> Chapter 3: Placing Braces and Spaces
> Chapter 4: Naming
> Chapter 5: Typedefs
> Chapter 6: Functions
> Chapter 7: Centralized exiting of functions
> Chapter 8: Commenting
> Chapter 9: You've made a mess of it
>
> Note: I'd suggest we rename the title of ch9 to "Custom Editor
> Programming/Indentation Modes" or something more descriptive.
>
> Chapter 10: Kconfig configuration files
> Chapter 11: Data structures
> Chapter 12: Macros, Enums and RTL
> Chapter 15: The inline disease
> Chapter 16: Function return values and names
> Chapter 18: Editor modelines and other cruft
>
> 3. move the following chapters to CodingSuggestions:
>
> Chapter 13: Printing kernel messages
> Note: ch13 is the one which mentions the don't put parentheses around %d.
>
> Chapter 14: Allocating memory
> Chapter 17: Don't re-invent the kernel macros
> Chapter 19: branch prediction optimizations (the un/likely debacle)
Super. And then, in the spirit of Linus's request,
we can submit another patch that simply removes that second file
completely from the kernel source tree!
Yay! The suits actually lose for once!
(okay, so I can fantasize if I want to).
On Sat, Sep 29, 2007 at 11:29:33PM -0400, [email protected] wrote:
> On Sat, 29 Sep 2007 20:00:01 PDT, Linus Torvalds said:
> > On Sat, 29 Sep 2007, [email protected] wrote:
> > > I think there needs to be a "sense of fairness" attached here - CodingStyle
> > > should cover all the stuff maintainers/reviewers are allowed to whinge about.
> >
> > No.
> >
> > People whine too much as is. Don't give them *license* to do so.
>
> I kind of meant the *maintainers* couldn't whine about things not in CodingStyle ;)
>
And yet people want to give license for maintainers to whine about
parenthesis in printk's?
Wow.
- Ted
On Sun, 30 Sep 2007 13:40:13 -0400 Mark Lord wrote:
> Erez Zadok wrote:
> > In message <[email protected]>, Linus Torvalds writes:
> >>
> >> On Sat, 29 Sep 2007, Erez Zadok wrote:
> >>> Would you prefer if CodingStyle was reorganized or even split into (1)
> >>> general principles and (2) details? Perhaps we need a CodingStylePrinciples
> >>> and a CodingStyleDetails?
> >> I'm certainly ok with the split into two files.
> >>
> >> What I'm not ok with is really important stuff (indentation), and then
> >> mixing in silly rules ("parenthesis are bad in printk's"?)
> >>
> >> Linus
> >
> > OK, looking at CodingStyle, I see two kinds of chapters. The first is stuff
> > that's more generic to C, and the other is more specific to Linux and how
> > one codes in the linux kernel. So I propose the following:
> >
> > 1. we create a new file called CodingSuggestions
> >
> > 2. we keep in CodingStyle the following chapters
> >
> > Chapter 1: Indentation
> > Chapter 2: Breaking long lines and strings
> > Chapter 3: Placing Braces and Spaces
> > Chapter 4: Naming
> > Chapter 5: Typedefs
> > Chapter 6: Functions
> > Chapter 7: Centralized exiting of functions
> > Chapter 8: Commenting
> > Chapter 9: You've made a mess of it
> >
> > Note: I'd suggest we rename the title of ch9 to "Custom Editor
> > Programming/Indentation Modes" or something more descriptive.
> >
> > Chapter 10: Kconfig configuration files
> > Chapter 11: Data structures
> > Chapter 12: Macros, Enums and RTL
> > Chapter 15: The inline disease
> > Chapter 16: Function return values and names
> > Chapter 18: Editor modelines and other cruft
> >
> > 3. move the following chapters to CodingSuggestions:
> >
> > Chapter 13: Printing kernel messages
> > Note: ch13 is the one which mentions the don't put parentheses around %d.
> >
> > Chapter 14: Allocating memory
> > Chapter 17: Don't re-invent the kernel macros
> > Chapter 19: branch prediction optimizations (the un/likely debacle)
>
> Super. And then, in the spirit of Linus's request,
> we can submit another patch that simply removes that second file
> completely from the kernel source tree!
or just revert CodingStyle to its contents of say 4 years ago.
> Yay! The suits actually lose for once!
> (okay, so I can fantasize if I want to).
Yep, you can.
The other losers are new contributors who get rejected for not
understanding implicit kernel coding styles and people who
get to try to explain to them on a repeating basis (if even
that happens).
or the kernel maintainers if coding style doesn't matter so much
and uglier code begins to be merged.
---
~Randy