Re: replay log: Re: [PATCH printk v4 38/39] printk: relieve console_lock of list synchronization duties

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: replay log: Re: [PATCH printk v4 38/39] printk: relieve console_lock of list synchronization duties
Date: Tue, 15 Nov 2022 19:23:13 +0100	[thread overview]
Message-ID: <Y3PZEbx+40ZyN/79@alley> (raw)
In-Reply-To: <87tu30maqf.fsf@jogness.linutronix.de>

On Tue 2022-11-15 17:47:12, John Ogness wrote:
> On 2022-11-15, Petr Mladek <pmladek@suse.com> wrote:
> > On Mon 2022-11-14 17:35:31, John Ogness wrote:
> >> --- a/kernel/printk/printk.c
> >> +++ b/kernel/printk/printk.c
> >> @@ -3334,6 +3330,11 @@ void register_console(struct console *newcon)
> >>  		 * boot console that is the furthest behind.
> >>  		 */
> >>  		if (bootcon_registered && !keep_bootcon) {
> >> +			/*
> >> +			 * Hold the console_lock to guarantee safe access to
> >> +			 * console->seq.
> >> +			 */
> >> +			console_lock();
> >>  			for_each_console(con) {
> >>  				if ((con->flags & CON_BOOT) &&
> >>  				    (con->flags & CON_ENABLED) &&
> >> @@ -3341,6 +3342,7 @@ void register_console(struct console *newcon)
> >>  					newcon->seq = con->seq;
> >>  				}
> >>  			}
> >> +			console_unlock();
> >
> > Thinking more about it. This console_unlock() will actually cause
> > flushing the boot consoles. A solution would be to call
> > console_flush_all() here.
> 
> console_flush_all() requires the console_lock, so I don't think it would
> be different.

I meant to keep the console_lock(), something like:

		if (bootcon_registered && !keep_bootcon) {
			/*
			 * Hold the console_lock to guarantee safe access to
			 * console->seq.
			 */
			console_lock();
			/* Try to sync all consoles. */
			console_flush_all();
			/* Check if some boot console is still behind. */
			for_each_console(con) {
				if ((con->flags & CON_BOOT) &&
				    (con->flags & CON_ENABLED) &&
				    con->seq < newcon->seq) {
					newcon->seq = con->seq;
				}
			}
			console_unlock();
		}

It is not that easy because console_flush_all() might handover the
console_lock(). Also some new messages might appear so that we
should re-read prb_next_seq().

Maybe, the best solution would be to call pr_flush():

		if (bootcon_registered && !keep_bootcon) {
			/*
			 * Try to sync all consoles because we do not
			 * know which one is going to be replaced
			 */
			pr_flush();
			/*
			 * Hold the console_lock to guarantee safe access to
			 * console->seq.
			 */
			console_lock();
			/* Check if some boot console is still behind. */
			for_each_console(con) {
				if ((con->flags & CON_BOOT) &&
				    (con->flags & CON_ENABLED) &&
				    con->seq < newcon->seq) {
					newcon->seq = con->seq;
				}
			}
			console_unlock();
		}

It was always just the best effort. It does not need to be perfect.
On the other hand, we should not make it much worse because people
report duplicated messages from time to time.

> The correct solution would be to recognize if nextcon is taking over a
> bootcon. If yes, that bootcon could be unregistered right here with
> unregister_console_locked() and then seq for nextcon set appropriately
> to perfectly take over.
> 
> But we will need to think about how we could recognize the same
> device. I was thinking about if consoles hat some attribute showing
> their io-membase or something so that it could be clear that the two are
> the same hardware.

Interesting idea. Well, it is out of scope of this patchset.

Best Regards,
Petr