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, 5 Jan 2023 16:24:55 +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 v4 5/8] printk: introduce
 printk_get_next_message() and printk_message
Message-ID: <Y7brx08wN+TE/YLN@alley>
References: <20230105103735.880956-1-john.ogness@linutronix.de>
 <20230105103735.880956-6-john.ogness@linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230105103735.880956-6-john.ogness@linutronix.de>
Precedence: bulk

On Thu 2023-01-05 11:43:32, 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 printk_get_next_message() to handle the reading and
> formatting of the printk text. The helper does not require any
> locking so that in the future it can be used for other printing
> contexts as well.
> 
> This also introduces a new struct printk_message to wrap the struct
> printk_buffers, adding metadata about its contents. This allows
> users of printk_get_next_message() to receive all relevant
> information about the message that was read and formatted.
> 
> Why is struct printk_message a wrapper struct?
> 
> It is intentional that a wrapper struct is introduced instead of
> adding the metadata directly to struct printk_buffers. The upcoming
> atomic consoles support multiple printing contexts per CPU. This
> means that while a CPU is formatting a message, it can be
> interrupted and the interrupting context may also format a (possibly
> different) message. Since the printk buffers are rather large,
> there will only be one struct printk_buffers per CPU and it must be
> shared by the possible contexts of that CPU.
> 
> If the metadata was part of struct printk_buffers, interrupting
> contexts would clobber the metadata being prepared by the
> interrupted context. This could be handled by robustifying the
> message formatting functions to cope with metadata unexpectedly
> changing. However, this would require significant amounts of extra
> data copying, also adding significant complexity to the code.
> 
> Instead, the metadata can live on the stack of the formatting
> context and the message formatting functions do not need to be
> concerned about the metadata changing underneath them.
> 
> Note that the message formatting functions can handle unexpected
> text buffer changes. So it is perfectly OK if a shared text buffer
> is clobbered by an interrupting context. The atomic console
> implementation will recognize the interruption and avoid printing
> the (probably garbage) text buffer.

Great description!

> Signed-off-by: John Ogness <john.ogness@linutronix.de>

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

Best Regards,
Petr