Received-SPF: pass (google.com: domain of linux-kernel+bounces-78810-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Fri, 23 Feb 2024 18:15:21 +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
Subject: Re: [PATCH printk v2 16/26] printk: nbcon: Use nbcon consoles in
 console_flush_all()
Message-ID: <ZdjSqZH5ivCrIBpx@alley>
References: <20240218185726.1994771-1-john.ogness@linutronix.de>
 <20240218185726.1994771-17-john.ogness@linutronix.de>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240218185726.1994771-17-john.ogness@linutronix.de>

On Sun 2024-02-18 20:03:16, John Ogness wrote:
> Allow nbcon consoles to print messages in the legacy printk()
> caller context (printing via unlock) by integrating them into
> console_flush_all(). The write_atomic() callback is used for
> printing.
> 
> Provide nbcon_legacy_emit_next_record(), which acts as the
> nbcon variant of console_emit_next_record(). Call this variant
> within console_flush_all() for nbcon consoles. Since nbcon
> consoles use their own @nbcon_seq variable to track the next
> record to print, this also must be appropriately handled.
> 
> --- a/kernel/printk/nbcon.c
> +++ b/kernel/printk/nbcon.c
> @@ -960,6 +961,50 @@ static bool nbcon_atomic_emit_one(struct nbcon_write_context *wctxt)
>  	return ctxt->backlog;
>  }
>  
> +/**
> + * nbcon_legacy_emit_next_record - Print one record for an nbcon console
> + *					in legacy contexts
> + * @con:	The console to print on
> + * @handover:	Will be set to true if a printk waiter has taken over the
> + *		console_lock, in which case the caller is no longer holding
> + *		both the console_lock and the SRCU read lock. Otherwise it
> + *		is set to false.
> + * @cookie:	The cookie from the SRCU read lock.
> + *
> + * Context:	Any context which could not be migrated to another CPU.

This seems to be a relic after shufling the code. IMHO, it was meant
for nbcon_atomic_emit_one(). This function disables interrupts
to prevent CPU migration when calling nbcon_atomic_emit_one().

> + * Return:	True if a record could be printed, otherwise false.

A more precise might be to say "has been printed" instead of
"could be printed".

It is more problematic here. It could return false also when it was
not able to acquire nbcon console.

While console_emit_next_record() has a bit different semantic.
It returns false only when there is no new record.

Thinking loudly:

It probably is not a bit deal. console_flush_all() does not guarantee
that it flushed all messages. It might return early when
the console_lock was handovered. It means there there is
another legacy context flushing the messages.

And nbcon console could not be acquired here also when
there is another nbcon context already flushing the nbcon.

Another question is whether console_flush_all() should return "false"
when nbcon consoles were not flushed because they could not be acquired.
It might be needed so that console_unlock() does not wait until
the other context flushes the nbcon console.

> + *
> + * This function is meant to be called by console_flush_all() to print records
> + * on nbcon consoles from legacy context (printing via console unlocking).
> + * Essentially it is the nbcon version of console_emit_next_record().
> + */
> +bool nbcon_legacy_emit_next_record(struct console *con, bool *handover,
> +				   int cookie)
> +{
> +	struct nbcon_write_context wctxt = { };
> +	struct nbcon_context *ctxt = &ACCESS_PRIVATE(&wctxt, ctxt);
> +	bool progress = false;
> +	unsigned long flags;
> +
> +	*handover = false;
> +
> +	/* Use the same procedure as console_emit_next_record(). */
> +	printk_safe_enter_irqsave(flags);
> +	console_lock_spinning_enable();
> +	stop_critical_timings();
> +
> +	ctxt->console	= con;
> +	ctxt->prio	= NBCON_PRIO_NORMAL;
> +
> +	progress = nbcon_atomic_emit_one(&wctxt);
> +
> +	start_critical_timings();
> +	*handover = console_lock_spinning_disable_and_check(cookie);
> +	printk_safe_exit_irqrestore(flags);
> +
> +	return progress;
> +}
> +
>  /**
>   * __nbcon_atomic_flush_all - Flush all nbcon consoles using their
>   *					write_atomic() callback
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2948,13 +2948,22 @@ static bool console_flush_all(bool do_cond_resched, u64 *next_seq, bool *handove
>  		cookie = console_srcu_read_lock();
>  		for_each_console_srcu(con) {
>  			short flags = console_srcu_read_flags(con);
> +			u64 printk_seq;
>  			bool progress;
>  
>  			if (!console_is_usable(con, flags))
>  				continue;
>  			any_usable = true;
>  
> -			progress = console_emit_next_record(con, handover, cookie);
> +			if (flags & CON_NBCON) {
> +				progress = nbcon_legacy_emit_next_record(con, handover, cookie);
> +
> +				printk_seq = nbcon_seq_read(con);

It might looks better without the empty line. But it is just my view.
Feel free to keep it as is.

> +			} else {
> +				progress = console_emit_next_record(con, handover, cookie);
> +
> +				printk_seq = con->seq;
> +			}
>  
>  			/*
>  			 * If a handover has occurred, the SRCU read lock

Best Regards,
Petr