Messages in this thread | | | Date | Mon, 31 Oct 2022 15:09:12 +0100 | From | Petr Mladek <> | Subject | Re: [PATCH printk v2 03/38] printk: Prepare for SRCU console list protection |
| |
On Mon 2022-10-31 14:12:36, John Ogness wrote: > On 2022-10-21, Petr Mladek <pmladek@suse.com> 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
| |