diff mbox series

[printk,v5,06/40] printk: fix setting first seq for consoles

Message ID 20221116162152.193147-7-john.ogness@linutronix.de
State New
Headers
Series reduce console_lock scope |

Commit Message

John Ogness Nov. 16, 2022, 4:21 p.m. UTC 
  It used to be that all consoles were synchronized with respect to
which message they were printing. After commit a699449bb13b ("printk:
refactor and rework printing logic"), all consoles have their own
@seq for tracking which message they are on. That commit also changed
how the initial sequence number was chosen. Instead of choosing the
next non-printed message, it chose the sequence number of the next
message that will be added to the ringbuffer.

That change created a possibility that a non-boot console taking over
for a boot console might skip messages if the boot console was behind
and did not have a chance to catch up before being unregistered.

Since it is not known which boot console is the same device, flush
all consoles and, if necessary, start with the message of the enabled
boot console that is the furthest behind. If no boot consoles are
enabled, begin with the next message that will be added to the
ringbuffer.

Also, since boot consoles are meant to be used at boot time, handle
them the same as CON_PRINTBUFFER to ensure that no initial messages
are skipped.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 kernel/printk/printk.c | 50 +++++++++++++++++++++++++++++++++++++-----
 1 file changed, 45 insertions(+), 5 deletions(-)

Comments

Petr Mladek Nov. 18, 2022, 10:23 a.m. UTC | #1 
On Wed 2022-11-16 17:27:18, John Ogness wrote:
> It used to be that all consoles were synchronized with respect to
> which message they were printing. After commit a699449bb13b ("printk:
> refactor and rework printing logic"), all consoles have their own
> @seq for tracking which message they are on. That commit also changed
> how the initial sequence number was chosen. Instead of choosing the
> next non-printed message, it chose the sequence number of the next
> message that will be added to the ringbuffer.
> 
> That change created a possibility that a non-boot console taking over
> for a boot console might skip messages if the boot console was behind
> and did not have a chance to catch up before being unregistered.
> 
> Since it is not known which boot console is the same device, flush
> all consoles and, if necessary, start with the message of the enabled
> boot console that is the furthest behind. If no boot consoles are
> enabled, begin with the next message that will be added to the
> ringbuffer.
> 
> Also, since boot consoles are meant to be used at boot time, handle
> them the same as CON_PRINTBUFFER to ensure that no initial messages
> are skipped.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

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

See one possible improvement below.

> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -3131,16 +3131,56 @@ static void try_enable_default_console(struct console *newcon)
>  	       (con->flags & CON_BOOT) ? "boot" : "",	\
>  	       con->name, con->index, ##__VA_ARGS__)
>  
> -static void console_init_seq(struct console *newcon)
> +static void console_init_seq(struct console *newcon, bool bootcon_registered)
>  {
> -	if (newcon->flags & CON_PRINTBUFFER) {
> +	struct console *con;
> +	bool handover;
> +
> +	if (newcon->flags & (CON_PRINTBUFFER | CON_BOOT)) {
>  		/* Get a consistent copy of @syslog_seq. */
>  		mutex_lock(&syslog_lock);
>  		newcon->seq = syslog_seq;
>  		mutex_unlock(&syslog_lock);
>  	} else {
> -		/* Begin with next message. */
> +		/* Begin with next message added to ringbuffer. */
>  		newcon->seq = prb_next_seq(prb);
> +
> +		/*
> +		 * If any enabled boot consoles are due to be unregistered
> +		 * shortly, some may not be caught up and may be the same
> +		 * device as @newcon. Since it is not known which boot console
> +		 * is the same device, flush all consoles and, if necessary,
> +		 * start with the message of the enabled boot console that is
> +		 * the furthest behind.
> +		 */
> +		if (bootcon_registered && !keep_bootcon) {
> +			/*
> +			 * Flush all consoles and set the console to start at
> +			 * the next unprinted sequence number.
> +			 */
> +			if (!console_flush_all(true, &newcon->seq, &handover)) {
> +				/*
> +				 * Flushing failed. Just choose the lowest
> +				 * sequence of the enabled boot consoles.
> +				 */
> +
> +				/*
> +				 * If there was a handover, this context no
> +				 * longer holds the console_lock.
> +				 */
> +				if (handover)
> +					console_lock();

Another improvement might be to disable handover in this case.
It would be safe because we are in a sleepable context.
It would increase the chance that console_fluhs_all() succeeded.

On the other hand, it might cause that this caller gets stuck
here because of flood of messages printed by another caller.

We could do this later when there are problems with this approach.
The problem with the handover has been there even before.

I do not want to delay this patchset by discussion this non-critical
problem to the death ;-)

Best Regards,
Petr
John Ogness Nov. 18, 2022, 10:52 a.m. UTC | #2 
On 2022-11-18, Petr Mladek <pmladek@suse.com> wrote:
> Another improvement might be to disable handover in this case.
> It would be safe because we are in a sleepable context.
> It would increase the chance that console_flush_all() succeeded.

I also considered this. The problem is that you cannot disable the
handover with the current API and it seemed crazy to dig all the up just
for this.

> We could do this later when there are problems with this approach.
> The problem with the handover has been there even before.

Agreed.

> I do not want to delay this patchset by discussion this non-critical
> problem to the death ;-)

Thanks! :-)

John
diff mbox series

Patch

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 31d9d1cf8682..c84654444a02 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3131,16 +3131,56 @@  static void try_enable_default_console(struct console *newcon)
 	       (con->flags & CON_BOOT) ? "boot" : "",	\
 	       con->name, con->index, ##__VA_ARGS__)
 
-static void console_init_seq(struct console *newcon)
+static void console_init_seq(struct console *newcon, bool bootcon_registered)
 {
-	if (newcon->flags & CON_PRINTBUFFER) {
+	struct console *con;
+	bool handover;
+
+	if (newcon->flags & (CON_PRINTBUFFER | CON_BOOT)) {
 		/* Get a consistent copy of @syslog_seq. */
 		mutex_lock(&syslog_lock);
 		newcon->seq = syslog_seq;
 		mutex_unlock(&syslog_lock);
 	} else {
-		/* Begin with next message. */
+		/* Begin with next message added to ringbuffer. */
 		newcon->seq = prb_next_seq(prb);
+
+		/*
+		 * If any enabled boot consoles are due to be unregistered
+		 * shortly, some may not be caught up and may be the same
+		 * device as @newcon. Since it is not known which boot console
+		 * is the same device, flush all consoles and, if necessary,
+		 * start with the message of the enabled boot console that is
+		 * the furthest behind.
+		 */
+		if (bootcon_registered && !keep_bootcon) {
+			/*
+			 * Flush all consoles and set the console to start at
+			 * the next unprinted sequence number.
+			 */
+			if (!console_flush_all(true, &newcon->seq, &handover)) {
+				/*
+				 * Flushing failed. Just choose the lowest
+				 * sequence of the enabled boot consoles.
+				 */
+
+				/*
+				 * If there was a handover, this context no
+				 * longer holds the console_lock.
+				 */
+				if (handover)
+					console_lock();
+
+				newcon->seq = prb_next_seq(prb);
+				for_each_console(con) {
+					if ((con->flags & CON_BOOT) &&
+					    (con->flags & CON_ENABLED) &&
+					    con->seq < newcon->seq) {
+						newcon->seq = con->seq;
+					}
+				}
+			}
+		}
 	}
 }
 
@@ -3235,13 +3275,13 @@  void register_console(struct console *newcon)
 	}
 
 	newcon->dropped = 0;
-	console_init_seq(newcon);
+	console_lock();
+	console_init_seq(newcon, bootcon_registered);
 
 	/*
 	 * Put this console in the list - keep the
 	 * preferred driver at the head of the list.
 	 */
-	console_lock();
 	if (hlist_empty(&console_list)) {
 		/* Ensure CON_CONSDEV is always set for the head. */
 		newcon->flags |= CON_CONSDEV;