* [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