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

[Petr Mladek]

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