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:   Mon, 2 Jan 2023 16:52:05 +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
Subject: Re: [PATCH printk v3 5/6] printk: introduce
 console_get_next_message() and console_message
Message-ID: <Y7L9pdSo9fSmx/F5@alley>
References: <20221221202704.857925-1-john.ogness@linutronix.de>
 <20221221202704.857925-6-john.ogness@linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221221202704.857925-6-john.ogness@linutronix.de>
Precedence: bulk

On Wed 2022-12-21 21:33:03, John Ogness wrote:
> Code for performing the console output is intermixed with code
> that is formatting the output for that console. Introduce a new
> helper function console_get_next_message() to handle the reading
> and formatting of the console text. The helper does not require
> any locking so that in the future it can be used for other console
> printing contexts as well.
> 
> This also introduces a new struct console_message to wrap the
> struct console_buffers adding meta-data about its contents. This
> allows users of console_get_next_message() to receive all relevant
> information about the message that was read and formatted.
> 
> The reason a wrapper struct is introduced instead of adding the
> meta-data to struct console_buffers is because the upcoming atomic
> consoles will need per-cpu and per-context console_buffers. It
> would not make sense to make the meta-data also per-cpu and
> per-context as that data can be easily stored on the stack of the
> console printing context.
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 2e5e2eda1fa1..7cac636600f8 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2725,34 +2725,34 @@ static void __console_unlock(void)
>  }
>  
>  /*
> - * Print one record for the given console. The record printed is whatever
> - * record is the next available record for the given console.
> + * Read and format the specified record (or a later record if the specified
> + * record is not available).
>   *
> - * @handover will be set to true if a printk waiter has taken over the
> - * console_lock, in which case the caller is no longer holding both the
> - * console_lock and the SRCU read lock. Otherwise it is set to false.
> + * @cmsg will contain the formatted result. @cmsg->cbufs must point to a
> + * struct console_buffers.
>   *
> - * @cookie is the cookie from the SRCU read lock.
> + * @seq is the record to read and format. If it is not available, the next
> + * valid record is read.
>   *
> - * Returns false if the given console has no next record to print, otherwise
> - * true.
> + * @is_extended specifies the message should be formatted for extended
> + * console output.
>   *
> - * Requires the console_lock and the SRCU read lock.
> + * Returns false if no record is available. Otherwise true and all fields
> + * of @cmsg are valid. (See the documentation of struct console_message
> + * for information about the @cmsg fields.)
>   */
> -static bool console_emit_next_record(struct console *con, bool *handover, int cookie)
> -{
> -	bool is_extended = console_srcu_read_flags(con) & CON_EXTENDED;
> -	static char dropped_text[DROPPED_TEXT_MAX];
> -	static struct console_buffers cbufs;
> -	const size_t scratchbuf_sz = sizeof(cbufs.scratchbuf);
> -	const size_t outbuf_sz = sizeof(cbufs.outbuf);
> -	char *scratchbuf = &cbufs.scratchbuf[0];
> -	char *outbuf = &cbufs.outbuf[0];
> +static bool console_get_next_message(struct console_message *cmsg, u64 seq,
> +				     bool is_extended)
> +{
> +	struct console_buffers *cbufs = cmsg->cbufs;
> +	const size_t scratchbuf_sz = sizeof(cbufs->scratchbuf);
> +	const size_t outbuf_sz = sizeof(cbufs->outbuf);
> +	char *scratchbuf = &cbufs->scratchbuf[0];
> +	char *outbuf = &cbufs->outbuf[0];
>  	static int panic_console_dropped;
>  	struct printk_info info;
>  	struct printk_record r;
> -	unsigned long flags;
> -	size_t len;
> +	size_t len = 0;
>  
>  	/*
>  	 * Formatting extended messages requires a separate buffer, so use the
> @@ -2766,33 +2766,77 @@ static bool console_emit_next_record(struct console *con, bool *handover, int co
>  	else
>  		prb_rec_init_rd(&r, &info, outbuf, outbuf_sz);
>  
> -	*handover = false;
> -
> -	if (!prb_read_valid(prb, con->seq, &r))
> +	if (!prb_read_valid(prb, 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");
> -		}
> +	cmsg->outbuf_seq = r.info->seq;
> +	cmsg->dropped = r.info->seq - seq;
> +
> +	/*
> +	 * Check for dropped messages in panic here so that printk
> +	 * suppression can occur as early as possible if necessary.
> +	 */
> +	if (cmsg->dropped &&
> +	    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. */
> -	if (suppress_message_printing(r.info->level)) {
> -		con->seq++;
> -		goto skip;
> -	}
> +	if (suppress_message_printing(r.info->level))
> +		goto out;
>  
>  	if (is_extended) {
>  		len = info_print_ext_header(outbuf, outbuf_sz, r.info);
>  		len += msg_print_ext_body(outbuf + len, outbuf_sz - len,
> -					  &r.text_buf[0], r.info->text_len, &r.info->dev_info);
> +					  r.text_buf, r.info->text_len, &r.info->dev_info);

This probably was not intended.

If you agree then I could revert this change when pushing.
Or feel free to send respin of this patch.


>  	} else {
>  		len = record_print_text(&r, console_msg_format & MSG_FORMAT_SYSLOG, printk_time);
>  	}
> +out:
> +	cmsg->outbuf_len = len;
> +	return true;
> +}

Otherwise, it looks good:

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr