Re: [PATCH 1/1] remove unnecessary calls of kmalloc and kfree from syslog.

[Steven Rostedt]

On Wed, 20 Jun 2018 15:46:24 +0530
Namit Gupta <gupta.namit@xxxxxxxxxxx> wrote:

> Below are the actual changes, rest all other visible changes are because of indentation.
> ==========================================================================================
> @@ -1348,16 +1348,23 @@ static int syslog_print_all(char __user *buf, int size, bool clear)
>  {
>     char *text;
>     int len = 0;
> +   u64 next_seq;
> +   u64 seq;
> +   u32 idx;

Please don't do this. We can figure out that the code is indented.
You can mention that the patch is mostly made up of changes because of
indention, but don't add a diff in the change log. Just explain what
and more importantly why you are making your changes here.

Also, the subject is wrong. It should include a topic like "printk:".
Please read Documentation/process/submitting-patches.rst for more
information.

That said, the patch looks fine to me. But please resubmit with a
proper change log.

Thanks,

-- Steve


> 
> Signed-off-by: Namit Gupta <gupta.namit@xxxxxxxxxxx>
> Signed-off-by: Himanshu Maithani <himanshu.m@xxxxxxxxxxx>
> 
> ---
>  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;
>  		}
>  	}
>