Message-ID: <ZQMuiDPabS3t5TtT@alley>
Date:   Thu, 14 Sep 2023 18:02:16 +0200
From:   Petr Mladek <pmladek@...e.com>
To:     John Ogness <john.ogness@...utronix.de>
Cc:     Sergey Senozhatsky <senozhatsky@...omium.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        linux-kernel@...r.kernel.org,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [PATCH printk v4 6/8] printk: nbcon: Add sequence handling

On Fri 2023-09-08 20:56:06, John Ogness wrote:
> From: Thomas Gleixner <tglx@...utronix.de>
> 
> Add an atomic_long_t field @nbcon_seq to the console struct to
> store the sequence number for nbcon consoles. For nbcon consoles
> this will be used instead of the non-atomic @seq field. The new
> field allows for safe atomic sequence number updates without
> requiring any locking.
> 
> On 64bit systems the new field stores the full sequence number.
> On 32bit systems the new field stores the lower 32 bits of the
> sequence number, which are expanded to 64bit as needed by
> folding the values based on the sequence numbers available in
> the ringbuffer.
> 
> For 32bit systems, having a 32bit representation in the console
> is sufficient. If a console ever gets more than 2^31 records
> behind the ringbuffer then this is the least of the problems.
> 
> diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
> index 644c4b9a4540..d23aa132fdcb 100644
> --- a/kernel/printk/nbcon.c
> +++ b/kernel/printk/nbcon.c
> +/**
> + * nbcon_seq_init - Helper function to initialize the console sequence
> + * @con:	Console to work on
> + *
> + * Set @con->nbcon_seq to the starting record (specified with con->seq).
> + * If the starting record no longer exists, the oldest available record
> + * is chosen. This is especially important on 32bit systems because only
> + * the lower 32 bits of the sequence number are stored. The upper 32 bits
> + * are derived from the sequence numbers available in the ringbuffer.
> + *
> + * For init only. Do not use for runtime updates.
> + */
> +static void nbcon_seq_init(struct console *con)
> +{
> +	u64 seq = max_t(u64, con->seq, prb_first_valid_seq(prb));
> +
> +	atomic_long_set(&ACCESS_PRIVATE(con, nbcon_seq), __seq_to_nbcon_seq(seq));
> +
> +	/* Clear con->seq since nbcon consoles use con->nbcon_seq instead. */
> +	con->seq = 0;
> +}
> +
> +/**
> + * nbcon_seq_read - Read the current console sequence
> + * @con:	Console to read the sequence of
> + *
> + * Return:	Sequence number of the next record to print on @con.
> + */
> +u64 nbcon_seq_read(struct console *con)
> +{
> +	unsigned long nbcon_seq = atomic_long_read(&ACCESS_PRIVATE(con, nbcon_seq));
> +
> +	return __nbcon_seq_to_seq(nbcon_seq);
> +}
> +
> +/**
> + * nbcon_seq_force - Force console sequence to a specific value
> + * @con:	Console to work on
> + * @seq:	Sequence number value to set
> + *
> + * Only to be used in extreme situations (such as panic with
> + * CONSOLE_REPLAY_ALL).
> + */
> +void nbcon_seq_force(struct console *con, u64 seq)
> +{
> +	atomic_long_set(&ACCESS_PRIVATE(con, nbcon_seq), __seq_to_nbcon_seq(seq));

We should actually do the same trick as in nbcon_seq_init() to make
sure that the 32-bit seq is shrinked against the prb_first_valid_seq().
I mean to do:

	/* If the starting record no longer exists, the oldest available record
	 * is chosen. This is especially important on 32bit systems because only
	 * the lower 32 bits of the sequence number are stored. The upper 32 bits
	 * are derived from the sequence numbers available in the ringbuffer.
	 */
	u64 valid_seq = max_t(u64, seq, prb_first_valid_seq(prb));

	atomic_long_set(&ACCESS_PRIVATE(con, nbcon_seq), __seq_to_nbcon_seq(valid));

> +}

And we might implement nbcon_seq_init() using nbcon_seq_force(). I mean:

static void nbcon_seq_init(struct console *con)
{
	nbcon_seq_force(con->seq);

	/* Clear con->seq since nbcon consoles use con->nbcon_seq instead. */
	con->seq = 0;
}

> @@ -540,11 +649,14 @@ static bool nbcon_context_can_proceed(struct nbcon_context *ctxt, struct nbcon_s
>  	nbcon_context_release(ctxt);
>  
>  	/*
> -	 * It is not known whether the handover succeeded. The outermost
> -	 * callsite has to make the final decision whether printing
> -	 * should proceed or not (via reacquire, possibly hostile). The
> -	 * console is now unlocked so go back all the way instead of
> -	 * trying to implement heuristics in tons of places.
> +	 * It is not clear whether the waiter really took over ownership. The
> +	 * outermost callsite must make the final decision whether console
> +	 * ownership is needed for it to proceed. If yes, it must reacquire
> +	 * ownership (possibly hostile) before carefully proceeding.
> +	 *
> +	 * The calling context no longer owns the console so go back all the
> +	 * way instead of trying to implement reacquire heuristics in tons of
> +	 * places.
>  	 */
>  	return false;
>  }

This change probably should have been done in the patch introducing
nbcon_context_can_proceed().

> @@ -636,6 +748,8 @@ bool nbcon_alloc(struct console *con)
>   *
>   * nbcon_alloc() *must* be called and succeed before this function
>   * is called.
> + *
> + * This function expects that the legacy @con->seq has been set.
>   */
>  void nbcon_init(struct console *con)
>  {

Best Regards,
Petr

Powered by blists - more mailing lists