Re: [PATCH printk v2 03/38] printk: Prepare for SRCU console list protection

From: Petr Mladek
Date: Mon Oct 31 2022 - 10:09:22 EST


On Mon 2022-10-31 14:12:36, John Ogness wrote:
> On 2022-10-21, Petr Mladek <pmladek@xxxxxxxx> wrote:
> >> @@ -3179,6 +3214,17 @@ void register_console(struct console *newcon)
> >> newcon->flags &= ~CON_PRINTBUFFER;
> >> }
> >>
> >> + newcon->dropped = 0;
> >> + if (newcon->flags & CON_PRINTBUFFER) {
> >> + /* Get a consistent copy of @syslog_seq. */
> >> + mutex_lock(&syslog_lock);
> >> + newcon->seq = syslog_seq;
> >> + mutex_unlock(&syslog_lock);
> >> + } else {
> >> + /* Begin with next message. */
> >> + newcon->seq = prb_next_seq(prb);
> >
> > Hmm, prb_next_seq() is the next-to-be written message. It does not
> > guarantee that all the existing messages already reached the boot
> > console.
> >
> > Ideally, we should set it to con->seq from the related boot
> > consoles. But we do not know which one it is.
>
> Yes. It is really sad that we do not know this. We need to fix this boot
> console handover at some point in the future.
>
> > A good enough solution would be to set it to the minimal con->seq
> > of all registered boot consoles. They would most likely be on
> > the same sequence number. Anyway, con->seq has to be read under
> > console_lock() at least at this stage of the patchset.
>
> Well, for v3 I would do the following:
>
> - explicitly have boot consoles also behave like CON_PRINTBUFFER
>
> - use the oldest boot+enabled message
>
> The code would include the additional changes:
>
> - if (newcon->flags & CON_PRINTBUFFER) {
> + 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 the ringbuffer. */
> newcon->seq = prb_next_seq(prb);
> +
> + /*
> + * If an enabled boot console is not caught up, start with
> + * that message instead. That boot console will be
> + * unregistered shortly and may be the same device.
> + */
> + for_each_console(con) {
> + if ((con->flags & (CON_BOOT | CON_ENABLED)) == (CON_BOOT | CON_ENABLED) &&
> + con->seq < newcon->seq) {
> + newcon->seq = con->seq;
> + }
> + }
> }

Makes sense. Just please do it in a separate patch. It might
potentially change the behavior. And the problem have been
there since the global "console_seq" was moved to struct console_seq.

> >> + hlist_add_behind_rcu(&newcon->node, console_list.first);
> >> }
> >> console_unlock();
> >> +

All the other proposed changes look good.

Best Regards,
Petr