Printing a single character alone when there's an immediately following
printk() is pretty pointless (and wasteful).
Signed-off-by: Jan Beulich <[email protected]>
---
arch/x86/kernel/process.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
--- 2.6.38-rc5/arch/x86/kernel/process.c
+++ 2.6.38-rc5-x86-show-regs-fold-printks/arch/x86/kernel/process.c
@@ -110,12 +110,9 @@ void show_regs_common(void)
init_utsname()->release,
(int)strcspn(init_utsname()->version, " "),
init_utsname()->version);
- printk(KERN_CONT " ");
- printk(KERN_CONT "%s %s", vendor, product);
- if (board) {
- printk(KERN_CONT "/");
- printk(KERN_CONT "%s", board);
- }
+ printk(KERN_CONT " %s %s", vendor, product);
+ if (board)
+ printk(KERN_CONT "/%s", board);
printk(KERN_CONT "\n");
}
On Thu, 2011-02-17 at 15:56 +0000, Jan Beulich wrote:
> Printing a single character alone when there's an immediately following
> printk() is pretty pointless (and wasteful).
>
> Signed-off-by: Jan Beulich <[email protected]>
Perhaps better still to use just one printk.
Signed-off-by: Joe Perches <[email protected]>
---
arch/x86/kernel/process.c | 22 ++++++++++------------
1 files changed, 10 insertions(+), 12 deletions(-)
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index ff45541..0f43771 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -103,20 +103,18 @@ void show_regs_common(void)
/* Board Name is optional */
board = dmi_get_system_info(DMI_BOARD_NAME);
+ if (!board)
+ board = "";
printk(KERN_CONT "\n");
- printk(KERN_DEFAULT "Pid: %d, comm: %.20s %s %s %.*s",
- current->pid, current->comm, print_tainted(),
- init_utsname()->release,
- (int)strcspn(init_utsname()->version, " "),
- init_utsname()->version);
- printk(KERN_CONT " ");
- printk(KERN_CONT "%s %s", vendor, product);
- if (board) {
- printk(KERN_CONT "/");
- printk(KERN_CONT "%s", board);
- }
- printk(KERN_CONT "\n");
+ printk(KERN_DEFAULT "Pid: %d, comm: %.20s %s %s %.*s %s %s%s%s\n",
+ current->pid, current->comm, print_tainted(),
+ init_utsname()->release,
+ (int)strcspn(init_utsname()->version, " "),
+ init_utsname()->version,
+ vendor, product,
+ strlen(board) ? "/" : "",
+ board);
}
void flush_thread(void)
Commit-ID: fd8fa4d3ddc4cc04ec8097e632b995d535c52beb
Gitweb: http://git.kernel.org/tip/fd8fa4d3ddc4cc04ec8097e632b995d535c52beb
Author: Jan Beulich <[email protected]>
AuthorDate: Thu, 17 Feb 2011 15:56:58 +0000
Committer: Ingo Molnar <[email protected]>
CommitDate: Fri, 18 Feb 2011 08:52:30 +0100
x86: Combine printk()s in show_regs_common()
Printing a single character alone when there's an immediately
following printk() is pretty pointless (and wasteful).
Signed-off-by: Jan Beulich <[email protected]>
LKML-Reference: <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
---
arch/x86/kernel/process.c | 9 +++------
1 files changed, 3 insertions(+), 6 deletions(-)
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index ff45541..99fa3ad 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -110,12 +110,9 @@ void show_regs_common(void)
init_utsname()->release,
(int)strcspn(init_utsname()->version, " "),
init_utsname()->version);
- printk(KERN_CONT " ");
- printk(KERN_CONT "%s %s", vendor, product);
- if (board) {
- printk(KERN_CONT "/");
- printk(KERN_CONT "%s", board);
- }
+ printk(KERN_CONT " %s %s", vendor, product);
+ if (board)
+ printk(KERN_CONT "/%s", board);
printk(KERN_CONT "\n");
}
On Fri, 2011-02-18 at 10:40 +0000, tip-bot for Jan Beulich wrote:
> Commit-ID: fd8fa4d3ddc4cc04ec8097e632b995d535c52beb
> Gitweb: http://git.kernel.org/tip/fd8fa4d3ddc4cc04ec8097e632b995d535c52beb
> Author: Jan Beulich <[email protected]>
> AuthorDate: Thu, 17 Feb 2011 15:56:58 +0000
> Committer: Ingo Molnar <[email protected]>
> CommitDate: Fri, 18 Feb 2011 08:52:30 +0100
>
> x86: Combine printk()s in show_regs_common()
>
> Printing a single character alone when there's an immediately
> following printk() is pretty pointless (and wasteful).
Ingo, why did you choose to apply this patch instead of
the alternative one I posted on the same thread?
https://patchwork.kernel.org/patch/571641/
* Joe Perches <[email protected]> wrote:
> On Fri, 2011-02-18 at 10:40 +0000, tip-bot for Jan Beulich wrote:
> > Commit-ID: fd8fa4d3ddc4cc04ec8097e632b995d535c52beb
> > Gitweb: http://git.kernel.org/tip/fd8fa4d3ddc4cc04ec8097e632b995d535c52beb
> > Author: Jan Beulich <[email protected]>
> > AuthorDate: Thu, 17 Feb 2011 15:56:58 +0000
> > Committer: Ingo Molnar <[email protected]>
> > CommitDate: Fri, 18 Feb 2011 08:52:30 +0100
> >
> > x86: Combine printk()s in show_regs_common()
> >
> > Printing a single character alone when there's an immediately
> > following printk() is pretty pointless (and wasteful).
>
> Ingo, why did you choose to apply this patch instead of
> the alternative one I posted on the same thread?
Your version:
/* Board Name is optional */
board = dmi_get_system_info(DMI_BOARD_NAME);
if (!board)
board = "";
printk(KERN_CONT "\n");
printk(KERN_DEFAULT "Pid: %d, comm: %.20s %s %s %.*s %s %s%s%s\n",
current->pid, current->comm, print_tainted(),
init_utsname()->release,
(int)strcspn(init_utsname()->version, " "),
init_utsname()->version,
vendor, product,
strlen(board) ? "/" : "",
board);
The 'board' fiddling and the strlen(board) check complicates things unnecessarily
and makes the code hard to read. Jan's version was at least simple.
Something like this:
/* Board Name is optional */
board = dmi_get_system_info(DMI_BOARD_NAME);
printk(KERN_CONT "\n");
printk(KERN_DEFAULT "Pid: %d, comm: %.20s %s %s %.*s %s %s %s %s\n",
current->pid, current->comm, print_tainted(),
init_utsname()->release,
(int)strcspn(init_utsname()->version, " "),
init_utsname()->version,
vendor, product, board ? : "");
Would be easier to read and gives similarly useful output.
Thanks,
Ingo
On Fri, 2011-02-18 at 21:05 +0100, Ingo Molnar wrote:
> * Joe Perches <[email protected]> wrote:
> > Ingo, why did you choose to apply this patch instead of
> > the alternative one I posted on the same thread?
> Your version:
>
> /* Board Name is optional */
> board = dmi_get_system_info(DMI_BOARD_NAME);
> if (!board)
> board = "";
>
> printk(KERN_CONT "\n");
>
> printk(KERN_DEFAULT "Pid: %d, comm: %.20s %s %s %.*s %s %s%s%s\n",
> current->pid, current->comm, print_tainted(),
> init_utsname()->release,
> (int)strcspn(init_utsname()->version, " "),
> init_utsname()->version,
> vendor, product,
> strlen(board) ? "/" : "",
> board);
>
> The 'board' fiddling and the strlen(board) check complicates things unnecessarily
> and makes the code hard to read. Jan's version was at least simple.
Perhaps you should look at the surrounding code.
It's uses the same style as I chose for "board".
void show_regs_common(void)
{
const char *vendor, *product, *board;
vendor = dmi_get_system_info(DMI_SYS_VENDOR);
if (!vendor)
vendor = "";
product = dmi_get_system_info(DMI_PRODUCT_NAME);
if (!product)
product = "";
...
> Something like this:
>
> /* Board Name is optional */
> board = dmi_get_system_info(DMI_BOARD_NAME);
>
> printk(KERN_CONT "\n");
>
> printk(KERN_DEFAULT "Pid: %d, comm: %.20s %s %s %.*s %s %s %s %s\n",
> current->pid, current->comm, print_tainted(),
> init_utsname()->release,
> (int)strcspn(init_utsname()->version, " "),
> init_utsname()->version,
> vendor, product, board ? : "");
>
> Would be easier to read and gives similarly useful output.
It's a question of using identical output or
merely similar output.
cheers, Joe
* Joe Perches <[email protected]> wrote:
> On Fri, 2011-02-18 at 21:05 +0100, Ingo Molnar wrote:
> > * Joe Perches <[email protected]> wrote:
> > > Ingo, why did you choose to apply this patch instead of
> > > the alternative one I posted on the same thread?
> > Your version:
> >
> > /* Board Name is optional */
> > board = dmi_get_system_info(DMI_BOARD_NAME);
> > if (!board)
> > board = "";
> >
> > printk(KERN_CONT "\n");
> >
> > printk(KERN_DEFAULT "Pid: %d, comm: %.20s %s %s %.*s %s %s%s%s\n",
> > current->pid, current->comm, print_tainted(),
> > init_utsname()->release,
> > (int)strcspn(init_utsname()->version, " "),
> > init_utsname()->version,
> > vendor, product,
> > strlen(board) ? "/" : "",
> > board);
> >
> > The 'board' fiddling and the strlen(board) check complicates things unnecessarily
> > and makes the code hard to read. Jan's version was at least simple.
>
> Perhaps you should look at the surrounding code.
> It's uses the same style as I chose for "board".
Yes, i realize that, but the strlen() trick really looks weird.
And 'weird' is not something i like seeing in essential bug reporting code.
Thanks,
Ingo