Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 31 Oct 2022 15:09:12 +0100
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, Miguel Ojeda <ojeda@kernel.org>,
        "Paul E. McKenney" <paulmck@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Subject: Re: [PATCH printk v2 03/38] printk: Prepare for SRCU console list
 protection
Message-ID: <Y1/XCLSPBZtsTy0h@alley>
References: <20221019145600.1282823-1-john.ogness@linutronix.de>
 <20221019145600.1282823-4-john.ogness@linutronix.de>
 <Y1JZqyIF5rf4sal2@alley>
 <87leow9m77.fsf@jogness.linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87leow9m77.fsf@jogness.linutronix.de>
Precedence: bulk

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