* [PATCH] printk: remove unnecessary kmalloc() from syslog during clear
[not found] <CGME20180620135951epcas5p3bd2a8f25ec689ca333bce861b527dba2@epcas5p3.samsung.com>
@ 2018-06-20 13:56 ` Namit Gupta
2018-06-20 14:16 ` Steven Rostedt
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Namit Gupta @ 2018-06-20 13:56 UTC (permalink / raw)
To: pmladek, sergey.senozhatsky, rostedt
Cc: linux-kernel, pankaj.m, a.sahrawat, himanshu.m, Namit Gupta
When the request is only for clearing logs, there is no need for
allocation/deallocation. Only the indexes need to be reset and returned.
Rest of the patch is mostly made up of changes because of indention.
Signed-off-by: Namit Gupta <gupta.namit@samsung.com>
Signed-off-by: Himanshu Maithani <himanshu.m@samsung.com>
---
kernel/printk/printk.c | 111 ++++++++++++++++++++++++++-----------------------
1 file changed, 60 insertions(+), 51 deletions(-)
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 512f7c2..53952ce 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -1348,71 +1348,80 @@ static int syslog_print_all(char __user *buf, int size, bool clear)
{
char *text;
int len = 0;
+ u64 next_seq;
+ u64 seq;
+ u32 idx;
+
+ if (!buf) {
+ if (clear) {
+ logbuf_lock_irq();
+ clear_seq = log_next_seq;
+ clear_idx = log_next_idx;
+ logbuf_unlock_irq();
+ }
+ return 0;
+ }
+
text = kmalloc(LOG_LINE_MAX + PREFIX_MAX, GFP_KERNEL);
if (!text)
return -ENOMEM;
logbuf_lock_irq();
- if (buf) {
- u64 next_seq;
- u64 seq;
- u32 idx;
- /*
- * Find first record that fits, including all following records,
- * into the user-provided buffer for this dump.
- */
- seq = clear_seq;
- idx = clear_idx;
- while (seq < log_next_seq) {
- struct printk_log *msg = log_from_idx(idx);
-
- len += msg_print_text(msg, true, NULL, 0);
- idx = log_next(idx);
- seq++;
- }
+ /*
+ * Find first record that fits, including all following records,
+ * into the user-provided buffer for this dump.
+ */
+ seq = clear_seq;
+ idx = clear_idx;
+ while (seq < log_next_seq) {
+ struct printk_log *msg = log_from_idx(idx);
- /* move first record forward until length fits into the buffer */
- seq = clear_seq;
- idx = clear_idx;
- while (len > size && seq < log_next_seq) {
- struct printk_log *msg = log_from_idx(idx);
+ len += msg_print_text(msg, true, NULL, 0);
+ idx = log_next(idx);
+ seq++;
+ }
- len -= msg_print_text(msg, true, NULL, 0);
- idx = log_next(idx);
- seq++;
- }
+ /* move first record forward until length fits into the buffer */
+ seq = clear_seq;
+ idx = clear_idx;
+ while (len > size && seq < log_next_seq) {
+ struct printk_log *msg = log_from_idx(idx);
- /* last message fitting into this dump */
- next_seq = log_next_seq;
+ len -= msg_print_text(msg, true, NULL, 0);
+ idx = log_next(idx);
+ seq++;
+ }
- len = 0;
- while (len >= 0 && seq < next_seq) {
- struct printk_log *msg = log_from_idx(idx);
- int textlen;
+ /* last message fitting into this dump */
+ next_seq = log_next_seq;
- textlen = msg_print_text(msg, true, text,
- LOG_LINE_MAX + PREFIX_MAX);
- if (textlen < 0) {
- len = textlen;
- break;
- }
- idx = log_next(idx);
- seq++;
+ len = 0;
+ while (len >= 0 && seq < next_seq) {
+ struct printk_log *msg = log_from_idx(idx);
+ int textlen;
- logbuf_unlock_irq();
- if (copy_to_user(buf + len, text, textlen))
- len = -EFAULT;
- else
- len += textlen;
- logbuf_lock_irq();
+ textlen = msg_print_text(msg, true, text,
+ LOG_LINE_MAX + PREFIX_MAX);
+ if (textlen < 0) {
+ len = textlen;
+ break;
+ }
+ idx = log_next(idx);
+ seq++;
- if (seq < log_first_seq) {
- /* messages are gone, move to next one */
- seq = log_first_seq;
- idx = log_first_idx;
- }
+ logbuf_unlock_irq();
+ if (copy_to_user(buf + len, text, textlen))
+ len = -EFAULT;
+ else
+ len += textlen;
+ logbuf_lock_irq();
+
+ if (seq < log_first_seq) {
+ /* messages are gone, move to next one */
+ seq = log_first_seq;
+ idx = log_first_idx;
}
}
--
1.9.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] printk: remove unnecessary kmalloc() from syslog during clear
2018-06-20 13:56 ` [PATCH] printk: remove unnecessary kmalloc() from syslog during clear Namit Gupta
@ 2018-06-20 14:16 ` Steven Rostedt
2018-06-21 6:58 ` Sergey Senozhatsky
2018-06-25 13:37 ` Petr Mladek
2 siblings, 0 replies; 12+ messages in thread
From: Steven Rostedt @ 2018-06-20 14:16 UTC (permalink / raw)
To: Namit Gupta
Cc: pmladek, sergey.senozhatsky, linux-kernel, pankaj.m, a.sahrawat,
himanshu.m
On Wed, 20 Jun 2018 19:26:19 +0530
Namit Gupta <gupta.namit@samsung.com> wrote:
> When the request is only for clearing logs, there is no need for
> allocation/deallocation. Only the indexes need to be reset and returned.
> Rest of the patch is mostly made up of changes because of indention.
Thanks for the update!
>
> Signed-off-by: Namit Gupta <gupta.namit@samsung.com>
> Signed-off-by: Himanshu Maithani <himanshu.m@samsung.com>
> ---
>
Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
-- Steve
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] printk: remove unnecessary kmalloc() from syslog during clear
2018-06-20 13:56 ` [PATCH] printk: remove unnecessary kmalloc() from syslog during clear Namit Gupta
2018-06-20 14:16 ` Steven Rostedt
@ 2018-06-21 6:58 ` Sergey Senozhatsky
2018-06-25 13:37 ` Petr Mladek
2 siblings, 0 replies; 12+ messages in thread
From: Sergey Senozhatsky @ 2018-06-21 6:58 UTC (permalink / raw)
To: Namit Gupta
Cc: pmladek, sergey.senozhatsky, rostedt, linux-kernel, pankaj.m,
a.sahrawat, himanshu.m
On (06/20/18 19:26), Namit Gupta wrote:
[..]
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 512f7c2..53952ce 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -1348,71 +1348,80 @@ static int syslog_print_all(char __user *buf, int size, bool clear)
> {
> char *text;
> int len = 0;
> + u64 next_seq;
> + u64 seq;
> + u32 idx;
> +
> + if (!buf) {
> + if (clear) {
> + logbuf_lock_irq();
> + clear_seq = log_next_seq;
> + clear_idx = log_next_idx;
> + logbuf_unlock_irq();
> + }
> + return 0;
> + }
---
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 4a07e7158898..33a1e45293a5 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -1395,13 +1395,11 @@ static int syslog_print_all(char __user *buf, int size, bool clear)
return 0;
}
-
text = kmalloc(LOG_LINE_MAX + PREFIX_MAX, GFP_KERNEL);
if (!text)
return -ENOMEM;
logbuf_lock_irq();
-
/*
* Find first record that fits, including all following records,
* into the user-provided buffer for this dump.
@@ -1436,7 +1434,7 @@ static int syslog_print_all(char __user *buf, int size, bool clear)
int textlen;
textlen = msg_print_text(msg, true, text,
- LOG_LINE_MAX + PREFIX_MAX);
+ LOG_LINE_MAX + PREFIX_MAX);
if (textlen < 0) {
len = textlen;
break;
---
Other than that, looks OK to me.
Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
-ss
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] printk: remove unnecessary kmalloc() from syslog during clear
2018-06-20 13:56 ` [PATCH] printk: remove unnecessary kmalloc() from syslog during clear Namit Gupta
2018-06-20 14:16 ` Steven Rostedt
2018-06-21 6:58 ` Sergey Senozhatsky
@ 2018-06-25 13:37 ` Petr Mladek
2018-06-25 13:42 ` Jiri Kosina
` (2 more replies)
2 siblings, 3 replies; 12+ messages in thread
From: Petr Mladek @ 2018-06-25 13:37 UTC (permalink / raw)
To: Namit Gupta
Cc: sergey.senozhatsky, rostedt, linux-kernel, pankaj.m, a.sahrawat,
himanshu.m
On Wed 2018-06-20 19:26:19, Namit Gupta wrote:
> When the request is only for clearing logs, there is no need for
> allocation/deallocation. Only the indexes need to be reset and returned.
> Rest of the patch is mostly made up of changes because of indention.
>
> Signed-off-by: Namit Gupta <gupta.namit@samsung.com>
> Signed-off-by: Himanshu Maithani <himanshu.m@samsung.com>
> ---
> kernel/printk/printk.c | 111 ++++++++++++++++++++++++++-----------------------
> 1 file changed, 60 insertions(+), 51 deletions(-)
>
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 512f7c2..53952ce 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -1348,71 +1348,80 @@ static int syslog_print_all(char __user *buf, int size, bool clear)
> {
> char *text;
> int len = 0;
> + u64 next_seq;
> + u64 seq;
> + u32 idx;
> +
> + if (!buf) {
> + if (clear) {
> + logbuf_lock_irq();
> + clear_seq = log_next_seq;
> + clear_idx = log_next_idx;
> + logbuf_unlock_irq();
I pushed a bit different version into printk.git, branch for-4.19,
see below. It removes the code duplication. Also it keeps the original
indentation. IMHO, it helped to better distinguish the code for printing
and clearing.
It is rather a cosmetic change, so I do not want you to resend
Reviewed-by tags. But feel free to disagree and ask me to use
the original variant.
This is in printk.git now:
From 41cb6dcedd9257d51fd310bf9b2958d11d93aa2b Mon Sep 17 00:00:00 2001
From: Namit Gupta <gupta.namit@samsung.com>
Date: Mon, 25 Jun 2018 14:58:05 +0200
Subject: [PATCH] printk: remove unnecessary kmalloc() from syslog during
When the request is only for clearing logs, there is no need for
allocation/deallocation. Only the indexes need to be reset and returned.
Rest of the patch is mostly made up of changes because of indention.
Link: http://lkml.kernel.org/r/20180620135951epcas5p3bd2a8f25ec689ca333bce861b527dba2~54wyKcT0_3155531555epcas5p3y@epcas5p3.samsung.com
Cc: linux-kernel@vger.kernel.org
Cc: pankaj.m@samsung.com
Cc: a.sahrawat@samsung.com
Signed-off-by: Namit Gupta <gupta.namit@samsung.com>
Signed-off-by: Himanshu Maithani <himanshu.m@samsung.com>
Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
[pmladek@suse.com: Removed code duplication.]
Signed-off-by: Petr Mladek <pmladek@suse.com>
---
kernel/printk/printk.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 247808333ba4..0fa2ca6fd8f9 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -1350,12 +1350,14 @@ static int syslog_print(char __user *buf, int size)
static int syslog_print_all(char __user *buf, int size, bool clear)
{
- char *text;
+ char *text = NULL;
int len = 0;
- text = kmalloc(LOG_LINE_MAX + PREFIX_MAX, GFP_KERNEL);
- if (!text)
- return -ENOMEM;
+ if (buf) {
+ text = kmalloc(LOG_LINE_MAX + PREFIX_MAX, GFP_KERNEL);
+ if (!text)
+ return -ENOMEM;
+ }
logbuf_lock_irq();
if (buf) {
@@ -1426,7 +1428,8 @@ static int syslog_print_all(char __user *buf, int size, bool clear)
}
logbuf_unlock_irq();
- kfree(text);
+ if (text)
+ kfree(text);
return len;
}
--
2.13.7
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] printk: remove unnecessary kmalloc() from syslog during clear
2018-06-25 13:37 ` Petr Mladek
@ 2018-06-25 13:42 ` Jiri Kosina
2018-06-25 14:35 ` Sergey Senozhatsky
2018-06-25 14:37 ` Steven Rostedt
2 siblings, 0 replies; 12+ messages in thread
From: Jiri Kosina @ 2018-06-25 13:42 UTC (permalink / raw)
To: Petr Mladek
Cc: Namit Gupta, sergey.senozhatsky, rostedt, linux-kernel, pankaj.m,
a.sahrawat, himanshu.m
On Mon, 25 Jun 2018, Petr Mladek wrote:
> @@ -1426,7 +1428,8 @@ static int syslog_print_all(char __user *buf, int size, bool clear)
> }
> logbuf_unlock_irq();
>
> - kfree(text);
> + if (text)
> + kfree(text);
Nit: you don't need to duplicate the check kfree() is doing anyway.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] printk: remove unnecessary kmalloc() from syslog during clear
2018-06-25 13:37 ` Petr Mladek
2018-06-25 13:42 ` Jiri Kosina
@ 2018-06-25 14:35 ` Sergey Senozhatsky
2018-06-25 14:37 ` Steven Rostedt
2 siblings, 0 replies; 12+ messages in thread
From: Sergey Senozhatsky @ 2018-06-25 14:35 UTC (permalink / raw)
To: Petr Mladek
Cc: Namit Gupta, sergey.senozhatsky, rostedt, linux-kernel, pankaj.m,
a.sahrawat, himanshu.m
On (06/25/18 15:37), Petr Mladek wrote:
>
> I pushed a bit different version into printk.git, branch for-4.19,
> see below. It removes the code duplication. Also it keeps the original
> indentation. IMHO, it helped to better distinguish the code for printing
> and clearing.
Agreed. Good job.
-ss
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] printk: remove unnecessary kmalloc() from syslog during clear
2018-06-25 13:37 ` Petr Mladek
2018-06-25 13:42 ` Jiri Kosina
2018-06-25 14:35 ` Sergey Senozhatsky
@ 2018-06-25 14:37 ` Steven Rostedt
2018-06-25 14:44 ` Sergey Senozhatsky
2 siblings, 1 reply; 12+ messages in thread
From: Steven Rostedt @ 2018-06-25 14:37 UTC (permalink / raw)
To: Petr Mladek
Cc: Namit Gupta, sergey.senozhatsky, linux-kernel, pankaj.m,
a.sahrawat, himanshu.m
On Mon, 25 Jun 2018 15:37:05 +0200
Petr Mladek <pmladek@suse.com> wrote:
> On Wed 2018-06-20 19:26:19, Namit Gupta wrote:
> > When the request is only for clearing logs, there is no need for
> > allocation/deallocation. Only the indexes need to be reset and returned.
> > Rest of the patch is mostly made up of changes because of indention.
> >
> > Signed-off-by: Namit Gupta <gupta.namit@samsung.com>
> > Signed-off-by: Himanshu Maithani <himanshu.m@samsung.com>
>
> > ---
> > kernel/printk/printk.c | 111 ++++++++++++++++++++++++++-----------------------
> > 1 file changed, 60 insertions(+), 51 deletions(-)
> >
> > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> > index 512f7c2..53952ce 100644
> > --- a/kernel/printk/printk.c
> > +++ b/kernel/printk/printk.c
> > @@ -1348,71 +1348,80 @@ static int syslog_print_all(char __user *buf, int size, bool clear)
> > {
> > char *text;
> > int len = 0;
> > + u64 next_seq;
> > + u64 seq;
> > + u32 idx;
> > +
> > + if (!buf) {
> > + if (clear) {
> > + logbuf_lock_irq();
> > + clear_seq = log_next_seq;
> > + clear_idx = log_next_idx;
> > + logbuf_unlock_irq();
>
> I pushed a bit different version into printk.git, branch for-4.19,
> see below. It removes the code duplication. Also it keeps the original
> indentation. IMHO, it helped to better distinguish the code for printing
> and clearing.
>
> It is rather a cosmetic change, so I do not want you to resend
> Reviewed-by tags. But feel free to disagree and ask me to use
> the original variant.
>
I actually prefer the original version. It's not really duplicating
much text, and it's cleaner, because it lets you know exactly what is
happening when buf == NULL.
if (buf)
text = kmalloc();
logbuf_lock_irq();
if (buf) {
[...]
}
Is IMHO rather ugly.
And the original patch has one more advantage. If buf and clear are
both NULL/zero, we don't take any locks.
-- Steve
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] printk: remove unnecessary kmalloc() from syslog during clear
2018-06-25 14:37 ` Steven Rostedt
@ 2018-06-25 14:44 ` Sergey Senozhatsky
2018-06-26 12:43 ` Petr Mladek
0 siblings, 1 reply; 12+ messages in thread
From: Sergey Senozhatsky @ 2018-06-25 14:44 UTC (permalink / raw)
To: Steven Rostedt
Cc: Petr Mladek, Namit Gupta, sergey.senozhatsky, linux-kernel,
pankaj.m, a.sahrawat, himanshu.m
On (06/25/18 10:37), Steven Rostedt wrote:
>
> Is IMHO rather ugly.
Either way works for me. So I'll leave it to you and Petr to decide :)
> And the original patch has one more advantage. If buf and clear are
> both NULL/zero, we don't take any locks.
But we never use syslog_print_all(buf = NULL, clear = false). It's either
NULL/true [move forward clear idx, do not copy to user], or !NULL/use defined
value [copy to user, move or don't move clear idx forward]
-ss
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] printk: remove unnecessary kmalloc() from syslog during clear
2018-06-25 14:44 ` Sergey Senozhatsky
@ 2018-06-26 12:43 ` Petr Mladek
2018-06-26 14:39 ` Steven Rostedt
0 siblings, 1 reply; 12+ messages in thread
From: Petr Mladek @ 2018-06-26 12:43 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: Steven Rostedt, Namit Gupta, linux-kernel, pankaj.m, a.sahrawat,
himanshu.m
On Mon 2018-06-25 23:44:07, Sergey Senozhatsky wrote:
> On (06/25/18 10:37), Steven Rostedt wrote:
> >
> > Is IMHO rather ugly.
>
> Either way works for me. So I'll leave it to you and Petr to decide :)
>
> > And the original patch has one more advantage. If buf and clear are
> > both NULL/zero, we don't take any locks.
>
> But we never use syslog_print_all(buf = NULL, clear = false). It's either
> NULL/true [move forward clear idx, do not copy to user], or !NULL/use defined
> value [copy to user, move or don't move clear idx forward]
Yup, I suggest the following version as a compromise. It has the code
duplication but I agree that it is negligible. Otherwise, it looks
cleaner.
From be6e161b3d75830e710c3ada56f5eeb615fae002 Mon Sep 17 00:00:00 2001
From: Namit Gupta <gupta.namit@samsung.com>
Date: Wed, 20 Jun 2018 19:26:19 +0530
Subject: [PATCH] printk: remove unnecessary kmalloc() from syslog during clear
When the request is only for clearing logs, there is no need for
allocation/deallocation. Only the indexes need to be reset and returned.
This patch implements this simple case separately. And syslog_print_all()
is always called with a valid buf and could use a more sane indentation.
Link: http://lkml.kernel.org/r/20180620135951epcas5p3bd2a8f25ec689ca333bce861b527dba2~54wyKcT0_3155531555epcas5p3y@epcas5p3.samsung.com
Cc: linux-kernel@vger.kernel.org
Cc: pankaj.m@samsung.com
Cc: a.sahrawat@samsung.com
Signed-off-by: Namit Gupta <gupta.namit@samsung.com>
Signed-off-by: Himanshu Maithani <himanshu.m@samsung.com>
Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Signed-off-by: Petr Mladek <pmladek@suse.com>
---
kernel/printk/printk.c | 111 ++++++++++++++++++++++++++-----------------------
1 file changed, 58 insertions(+), 53 deletions(-)
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 247808333ba4..588f23c9c14f 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -1352,71 +1352,68 @@ static int syslog_print_all(char __user *buf, int size, bool clear)
{
char *text;
int len = 0;
+ u64 next_seq;
+ u64 seq;
+ u32 idx;
text = kmalloc(LOG_LINE_MAX + PREFIX_MAX, GFP_KERNEL);
if (!text)
return -ENOMEM;
logbuf_lock_irq();
- if (buf) {
- u64 next_seq;
- u64 seq;
- u32 idx;
+ /*
+ * Find first record that fits, including all following records,
+ * into the user-provided buffer for this dump.
+ */
+ seq = clear_seq;
+ idx = clear_idx;
+ while (seq < log_next_seq) {
+ struct printk_log *msg = log_from_idx(idx);
- /*
- * Find first record that fits, including all following records,
- * into the user-provided buffer for this dump.
- */
- seq = clear_seq;
- idx = clear_idx;
- while (seq < log_next_seq) {
- struct printk_log *msg = log_from_idx(idx);
-
- len += msg_print_text(msg, true, NULL, 0);
- idx = log_next(idx);
- seq++;
- }
+ len += msg_print_text(msg, true, NULL, 0);
+ idx = log_next(idx);
+ seq++;
+ }
- /* move first record forward until length fits into the buffer */
- seq = clear_seq;
- idx = clear_idx;
- while (len > size && seq < log_next_seq) {
- struct printk_log *msg = log_from_idx(idx);
+ /* move first record forward until length fits into the buffer */
+ seq = clear_seq;
+ idx = clear_idx;
+ while (len > size && seq < log_next_seq) {
+ struct printk_log *msg = log_from_idx(idx);
- len -= msg_print_text(msg, true, NULL, 0);
- idx = log_next(idx);
- seq++;
- }
+ len -= msg_print_text(msg, true, NULL, 0);
+ idx = log_next(idx);
+ seq++;
+ }
- /* last message fitting into this dump */
- next_seq = log_next_seq;
+ /* last message fitting into this dump */
+ next_seq = log_next_seq;
- len = 0;
- while (len >= 0 && seq < next_seq) {
- struct printk_log *msg = log_from_idx(idx);
- int textlen;
+ len = 0;
+ while (len >= 0 && seq < next_seq) {
+ struct printk_log *msg = log_from_idx(idx);
+ int textlen;
- textlen = msg_print_text(msg, true, text,
- LOG_LINE_MAX + PREFIX_MAX);
- if (textlen < 0) {
- len = textlen;
- break;
- }
- idx = log_next(idx);
- seq++;
+ textlen = msg_print_text(msg, true, text,
+ LOG_LINE_MAX + PREFIX_MAX);
+ if (textlen < 0) {
+ len = textlen;
+ break;
+ }
+ idx = log_next(idx);
+ seq++;
- logbuf_unlock_irq();
- if (copy_to_user(buf + len, text, textlen))
- len = -EFAULT;
- else
- len += textlen;
- logbuf_lock_irq();
-
- if (seq < log_first_seq) {
- /* messages are gone, move to next one */
- seq = log_first_seq;
- idx = log_first_idx;
- }
+ logbuf_unlock_irq();
+ if (copy_to_user(buf + len, text, textlen))
+ len = -EFAULT;
+ else
+ len += textlen;
+ logbuf_lock_irq();
+
+ if (seq < log_first_seq) {
+ /* messages are gone, move to next one */
+ seq = log_first_seq;
+ idx = log_first_idx;
}
}
@@ -1430,6 +1427,14 @@ static int syslog_print_all(char __user *buf, int size, bool clear)
return len;
}
+static void syslog_clear(void)
+{
+ logbuf_lock_irq();
+ clear_seq = log_next_seq;
+ clear_idx = log_next_idx;
+ logbuf_unlock_irq();
+}
+
int do_syslog(int type, char __user *buf, int len, int source)
{
bool clear = false;
@@ -1474,7 +1479,7 @@ int do_syslog(int type, char __user *buf, int len, int source)
break;
/* Clear ring buffer */
case SYSLOG_ACTION_CLEAR:
- syslog_print_all(NULL, 0, true);
+ syslog_clear();
break;
/* Disable logging to console */
case SYSLOG_ACTION_CONSOLE_OFF:
--
2.13.7
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] printk: remove unnecessary kmalloc() from syslog during clear
2018-06-26 12:43 ` Petr Mladek
@ 2018-06-26 14:39 ` Steven Rostedt
2018-06-27 15:06 ` Petr Mladek
0 siblings, 1 reply; 12+ messages in thread
From: Steven Rostedt @ 2018-06-26 14:39 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Namit Gupta, linux-kernel, pankaj.m,
a.sahrawat, himanshu.m
On Tue, 26 Jun 2018 14:43:32 +0200
Petr Mladek <pmladek@suse.com> wrote:
> On Mon 2018-06-25 23:44:07, Sergey Senozhatsky wrote:
> > On (06/25/18 10:37), Steven Rostedt wrote:
> > >
> > > Is IMHO rather ugly.
> >
> > Either way works for me. So I'll leave it to you and Petr to decide :)
> >
> > > And the original patch has one more advantage. If buf and clear are
> > > both NULL/zero, we don't take any locks.
> >
> > But we never use syslog_print_all(buf = NULL, clear = false). It's either
> > NULL/true [move forward clear idx, do not copy to user], or !NULL/use defined
> > value [copy to user, move or don't move clear idx forward]
>
> Yup, I suggest the following version as a compromise. It has the code
> duplication but I agree that it is negligible. Otherwise, it looks
> cleaner.
>
This looks fine. For proper history though, what I do in this case,
would be to make this into two patches. One with Namit's original
patch, and then a second that removes the code duplication (your patch).
-- Steve
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] printk: remove unnecessary kmalloc() from syslog during clear
2018-06-26 14:39 ` Steven Rostedt
@ 2018-06-27 15:06 ` Petr Mladek
2018-07-09 11:48 ` Petr Mladek
0 siblings, 1 reply; 12+ messages in thread
From: Petr Mladek @ 2018-06-27 15:06 UTC (permalink / raw)
To: Steven Rostedt
Cc: Sergey Senozhatsky, Namit Gupta, linux-kernel, pankaj.m,
a.sahrawat, himanshu.m
On Tue 2018-06-26 10:39:59, Steven Rostedt wrote:
> On Tue, 26 Jun 2018 14:43:32 +0200
> Petr Mladek <pmladek@suse.com> wrote:
>
> > On Mon 2018-06-25 23:44:07, Sergey Senozhatsky wrote:
> > > On (06/25/18 10:37), Steven Rostedt wrote:
> > > >
> > > > Is IMHO rather ugly.
> > >
> > > Either way works for me. So I'll leave it to you and Petr to decide :)
> > >
> > > > And the original patch has one more advantage. If buf and clear are
> > > > both NULL/zero, we don't take any locks.
> > >
> > > But we never use syslog_print_all(buf = NULL, clear = false). It's either
> > > NULL/true [move forward clear idx, do not copy to user], or !NULL/use defined
> > > value [copy to user, move or don't move clear idx forward]
> >
> > Yup, I suggest the following version as a compromise. It has the code
> > duplication but I agree that it is negligible. Otherwise, it looks
> > cleaner.
> >
>
> This looks fine. For proper history though, what I do in this case,
> would be to make this into two patches. One with Namit's original
> patch, and then a second that removes the code duplication (your patch).
Makes sense. I have just pushed the original patch (with Sergey's
formatting fixes) into printk.git, for-4.19 branch. And I propose
the clean up patch, see below.
I am sorry for the mess. It seems that nothing can be considered
a simple obvious change. And my attempt to safe people time had
the opposite effect.
Proposed patch:
From: Petr Mladek <pmladek@suse.com>
Date: Wed, 27 Jun 2018 16:37:42 +0200
Subject: [PATCH] printk: Clean up syslog_print_all()
syslog_print_all() is called twice. Once with a valid buffer
and once just to set the indexes.
Both variants are already handled separately. This patch just
makes it more obvious. It does not change the existing behavior.
Signed-off-by: Petr Mladek <pmladek@suse.com>
---
kernel/printk/printk.c | 20 +++++++++-----------
1 file changed, 9 insertions(+), 11 deletions(-)
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 16b02cc51a14..fcc1992c040a 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -1356,16 +1356,6 @@ static int syslog_print_all(char __user *buf, int size, bool clear)
u64 seq;
u32 idx;
- if (!buf) {
- if (clear) {
- logbuf_lock_irq();
- clear_seq = log_next_seq;
- clear_idx = log_next_idx;
- logbuf_unlock_irq();
- }
- return 0;
- }
-
text = kmalloc(LOG_LINE_MAX + PREFIX_MAX, GFP_KERNEL);
if (!text)
return -ENOMEM;
@@ -1437,6 +1427,14 @@ static int syslog_print_all(char __user *buf, int size, bool clear)
return len;
}
+static void syslog_clear(void)
+{
+ logbuf_lock_irq();
+ clear_seq = log_next_seq;
+ clear_idx = log_next_idx;
+ logbuf_unlock_irq();
+}
+
int do_syslog(int type, char __user *buf, int len, int source)
{
bool clear = false;
@@ -1481,7 +1479,7 @@ int do_syslog(int type, char __user *buf, int len, int source)
break;
/* Clear ring buffer */
case SYSLOG_ACTION_CLEAR:
- syslog_print_all(NULL, 0, true);
+ syslog_clear();
break;
/* Disable logging to console */
case SYSLOG_ACTION_CONSOLE_OFF:
--
2.13.7
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] printk: remove unnecessary kmalloc() from syslog during clear
2018-06-27 15:06 ` Petr Mladek
@ 2018-07-09 11:48 ` Petr Mladek
0 siblings, 0 replies; 12+ messages in thread
From: Petr Mladek @ 2018-07-09 11:48 UTC (permalink / raw)
To: Steven Rostedt
Cc: Sergey Senozhatsky, Namit Gupta, linux-kernel, pankaj.m,
a.sahrawat, himanshu.m
On Wed 2018-06-27 17:06:41, Petr Mladek wrote:
> On Tue 2018-06-26 10:39:59, Steven Rostedt wrote:
> > This looks fine. For proper history though, what I do in this case,
> > would be to make this into two patches. One with Namit's original
> > patch, and then a second that removes the code duplication (your patch).
>
> Proposed patch:
>
> From: Petr Mladek <pmladek@suse.com>
> Date: Wed, 27 Jun 2018 16:37:42 +0200
> Subject: [PATCH] printk: Clean up syslog_print_all()
>
> syslog_print_all() is called twice. Once with a valid buffer
> and once just to set the indexes.
>
> Both variants are already handled separately. This patch just
> makes it more obvious. It does not change the existing behavior.
JFYI, I have pushed this separate clean up patch into printk.git,
branch for-4.19.
Best Regards,
Petr
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2018-07-09 11:48 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CGME20180620135951epcas5p3bd2a8f25ec689ca333bce861b527dba2@epcas5p3.samsung.com>
2018-06-20 13:56 ` [PATCH] printk: remove unnecessary kmalloc() from syslog during clear Namit Gupta
2018-06-20 14:16 ` Steven Rostedt
2018-06-21 6:58 ` Sergey Senozhatsky
2018-06-25 13:37 ` Petr Mladek
2018-06-25 13:42 ` Jiri Kosina
2018-06-25 14:35 ` Sergey Senozhatsky
2018-06-25 14:37 ` Steven Rostedt
2018-06-25 14:44 ` Sergey Senozhatsky
2018-06-26 12:43 ` Petr Mladek
2018-06-26 14:39 ` Steven Rostedt
2018-06-27 15:06 ` Petr Mladek
2018-07-09 11:48 ` Petr Mladek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).