Re: [PATCH printk v3 6/7] printk: nbcon: Add emit function and callback function for atomic printing

[Petr Mladek]

On Sun 2023-09-03 17:11:38, John Ogness wrote:
> From: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> 
> Implement an emit function for nbcon consoles to output printk
> messages. It utilizes the lockless printk_get_next_message() and
> console_prepend_dropped() functions to retrieve/build the output
> message. The emit function includes the required safety points to
> check for handover/takeover and calls a new write_atomic callback
> of the console driver to output the message. It also includes
> proper handling for updating the nbcon console sequence number.
> 
> A new nbcon_write_context struct is introduced. This is provided
> to the write_atomic callback and includes only the information
> necessary for performing atomic writes.
> 
> Co-developed-by: John Ogness <john.ogness@xxxxxxxxxxxxx>
> Signed-off-by: John Ogness <john.ogness@xxxxxxxxxxxxx>
> Signed-off-by: Thomas Gleixner (Intel) <tglx@xxxxxxxxxxxxx>

Reviewed-by: Petr Mladek <pmladek@xxxxxxxx>

Well, I would prefer one small change, see below.

> --- a/kernel/printk/nbcon.c
> +++ b/kernel/printk/nbcon.c
> @@ -732,6 +756,102 @@ static bool nbcon_context_update_unsafe(struct nbcon_context *ctxt, bool unsafe)
>  	return nbcon_context_can_proceed(ctxt, &cur);
>  }
>  
> +/**
> + * nbcon_emit_next_record - Emit a record in the acquired context
> + * @wctxt:	The write context that will be handed to the write function
> + *
> + * Return:	True if this context still owns the console. False if
> + *		ownership was handed over or taken.
> + *
> + * When this function returns false then the calling context no longer owns
> + * the console and is no longer allowed to go forward. In this case it must
> + * back out immediately and carefully. The buffer content is also no longer
> + * trusted since it no longer belongs to the calling context. If the caller
> + * wants to do more it must reacquire the console first.
> + *
> + * When true is returned, @wctxt->ctxt.backlog indicates whether there are
> + * still records pending in the ringbuffer,
> + */
> +__maybe_unused
> +static bool nbcon_emit_next_record(struct nbcon_write_context *wctxt)
> +{
> +	struct nbcon_context *ctxt = &ACCESS_PRIVATE(wctxt, ctxt);
> +	struct console *con = ctxt->console;
> +	bool is_extended = console_srcu_read_flags(con) & CON_EXTENDED;
> +	struct printk_message pmsg = {
> +		.pbufs = ctxt->pbufs,
> +	};
> +	unsigned long con_dropped;
> +	struct nbcon_state cur;
> +	unsigned long dropped;
> +	bool done;
[...]
> +update_con:
> +	/*
> +	 * The dropped count and the sequence number are updated within an
> +	 * unsafe section. This limits update races to the panic context and
> +	 * allows the panic context to win.
> +	 */
> +
> +	if (!nbcon_context_update_unsafe(ctxt, true))
> +		return false;

I would prefer to use nbcon_enter_unsafe(). It is more self-explaining.
And it will make it easier to see the "all" enter/exit points using cscope.

> +
> +	if (dropped != con_dropped) {
> +		/* Counterpart to the READ_ONCE() above. */
> +		WRITE_ONCE(con->dropped, dropped);
> +	}
> +
> +	nbcon_seq_try_update(ctxt, pmsg.seq + 1);
> +
> +	return nbcon_context_update_unsafe(ctxt, false);
> +}

Best Regards,
Petr