[PATCH] printk: remove unnecessary kmalloc() from syslog during clear

[PATCH] printk: remove unnecessary kmalloc() from syslog during clear

[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

Re: [PATCH] printk: remove unnecessary kmalloc() from syslog during clear

[Steven Rostedt]

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

Re: [PATCH] printk: remove unnecessary kmalloc() from syslog during clear

[Sergey Senozhatsky]

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

Re: [PATCH] printk: remove unnecessary kmalloc() from syslog during clear

[Petr Mladek]

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

Re: [PATCH] printk: remove unnecessary kmalloc() from syslog during clear

[Jiri Kosina]

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

Re: [PATCH] printk: remove unnecessary kmalloc() from syslog during clear

[Sergey Senozhatsky]

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

Re: [PATCH] printk: remove unnecessary kmalloc() from syslog during clear

[Steven Rostedt]

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

Re: [PATCH] printk: remove unnecessary kmalloc() from syslog during clear

[Sergey Senozhatsky]

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

Re: [PATCH] printk: remove unnecessary kmalloc() from syslog during clear

[Petr Mladek]

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

Re: [PATCH] printk: remove unnecessary kmalloc() from syslog during clear

[Steven Rostedt]

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

Re: [PATCH] printk: remove unnecessary kmalloc() from syslog during clear

[Petr Mladek]

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

Re: [PATCH] printk: remove unnecessary kmalloc() from syslog during clear

[Petr Mladek]

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