Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 24 Nov 2022 19:00:11 +0100
From:   Petr Mladek <pmladek@suse.com>
To:     John Ogness <john.ogness@linutronix.de>
Cc:     Sergey Senozhatsky <senozhatsky@chromium.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        linux-kernel@vger.kernel.org,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Subject: Re: [PATCH printk v2 6/7] printk: Use an output buffer descriptor
 struct for emit
Message-ID: <Y3+xK7hHmUIlzq9w@alley>
References: <20221123231400.614679-1-john.ogness@linutronix.de>
 <20221123231400.614679-7-john.ogness@linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221123231400.614679-7-john.ogness@linutronix.de>
Precedence: bulk

On Thu 2022-11-24 00:19:59, John Ogness wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> To prepare for a new console infrastructure that is independent of
> the console BKL, wrap the output mode into a descriptor struct so
> the new infrastructure can share the emit code that dumps the
> ringbuffer record into the output text buffers.
> 
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2741,11 +2741,76 @@ static void __console_unlock(void)
>  	up_console_sem();
>  }
>  
> +/**
> + * console_get_next_message - Fill the output buffer with the next record
> + * @con:	The console to print on
> + * @cmsg:	Pointer to the output buffer descriptor
> + *
> + * Return: False if there is no pending record in the ringbuffer.
> + *	   True if there is a pending record in the ringbuffer.
> + *
> + * When the return value is true, then the caller must check
> + * @cmsg->outbuf. If not NULL it points to the first character to write
> + * to the device and @cmsg->outbuf_len contains the length of the message.
> + * If it is NULL then the record will be skipped.
> + */
> +static bool console_get_next_message(struct console *con, struct console_message *cmsg)
> +{

I wish, this change was done in two patches. 1st introducing and
using struct console_message. 2nd moving the code into separate
console_get_next_message().

I do not resist on it but it might help to see what exactly has changed.

> +	struct console_buffers *cbufs = cmsg->cbufs;
> +	static int panic_console_dropped;
> +	struct printk_info info;
> +	struct printk_record r;
> +	size_t write_text_size;
> +	char *write_text;
> +	size_t len;
> +
> +	cmsg->outbuf = NULL;
> +	cmsg->outbuf_len = 0;
> +
> +	prb_rec_init_rd(&r, &info, &cbufs->text[0], sizeof(cbufs->text));
> +
> +	if (!prb_read_valid(prb, con->seq, &r))
> +		return false;
> +
> +	if (con->seq != r.info->seq) {
> +		con->dropped += r.info->seq - con->seq;
> +		con->seq = r.info->seq;
> +		if (panic_in_progress() && panic_console_dropped++ > 10) {
> +			suppress_panic_printk = 1;
> +			pr_warn_once("Too many dropped messages. Suppress messages on non-panic CPUs to prevent livelock.\n");
> +		}
> +	}
> +
> +	/*
> +	 * Skip record that has level above the console loglevel.
> +	 * Return true so the caller knows a record exists and
> +	 * leave cmsg->outbuf NULL so the caller knows the record
> +	 * is being skipped.
> +	 */
> +	if (suppress_message_printing(r.info->level))
> +		return true;
> +
> +	if (cmsg->is_extmsg) {
> +		write_text = &cbufs->ext_text[0];
> +		write_text_size = sizeof(cbufs->ext_text);
> +		len = info_print_ext_header(write_text, write_text_size, r.info);
> +		len += msg_print_ext_body(write_text + len, write_text_size - len,
> +					  &r.text_buf[0], r.info->text_len, &r.info->dev_info);
> +	} else {
> +		write_text = &cbufs->text[0];
> +		len = record_print_text(&r, console_msg_format & MSG_FORMAT_SYSLOG, printk_time);
> +	}
> +
> +	cmsg->outbuf = write_text;
> +	cmsg->outbuf_len = len;

Please, remove "write_text" variable and use cmsg->outbuf directly.
It would safe one mental transition of buffer names:

   cbufs->text -> write_text -> cmsg->outbuf

vs.

   cbufs->text -> cmsg->outbuf

Best Regards,
Petr

PS: Please, wait a bit with updating the patches. I have got yet
    another idea when seeing the code around dropped messages.
    But I have to sleep over it.

    My concern is that the message about dropped messages need not
    fit into the smaller "cbufs->text" buffer. It might be better
    to put it into the bigger one.

    We might actually always use the bigger buffer as the output
    buffer. The smaller buffer might be only temporary when formatting
    the extended messages.

     We could replace

	struct console_buffers {
		char	ext_text[CONSOLE_EXT_LOG_MAX];
		char	text[CONSOLE_LOG_MAX];
	};

    with

	struct console_buffers {
		char	outbuf[CONSOLE_EXT_LOG_MAX];
		char	readbuf[CONSOLE_LOG_MAX];
	};

     Normal consoles would use only @outbuf. Only the extended console
     would need the @readbuf to read the messages before they are formatted.

     I guess that struct console_message won't be needed then at all.

     It might help to remove several twists in the code.

     I am sorry that I have not got this idea when reviewing v1.
     Well, the code was even more complicated at that time.