Re: [PATCH printk v3 09/19] printk: nbcon: Introduce printer kthreads

[John Ogness]

On 2024-07-22, John Ogness <john.ogness@xxxxxxxxxxxxx> wrote:
> +/**
> + * nbcon_kthread_should_wakeup - Check whether a printer thread should wakeup
> + * @con:	Console to operate on
> + * @ctxt:	The nbcon context from nbcon_context_try_acquire()
> + *
> + * Return:	True if the thread should shutdown or if the console is
> + *		allowed to print and a record is available. False otherwise.
> + *
> + * After the thread wakes up, it must first check if it should shutdown before
> + * attempting any printing.
> + */
> +static bool nbcon_kthread_should_wakeup(struct console *con, struct nbcon_context *ctxt)
> +{
> +	bool ret = false;
> +	short flags;
> +	int cookie;
> +
> +	if (kthread_should_stop())
> +		return true;
> +
> +	cookie = console_srcu_read_lock();
> +
> +	flags = console_srcu_read_flags(con);
> +	if (console_is_usable(con, flags, false)) {
> +		/* Bring the sequence in @ctxt up to date */
> +		ctxt->seq = nbcon_seq_read(con);
> +
> +		ret = prb_read_valid(prb, ctxt->seq, NULL);

With this v3 series, the kthreads could be started before @nbcon_seq has
been set to the correct initial value. This will cause it to start
printing immediately from 0.

To fix this, I would change nbcon_alloc() to initialize @nbcon_seq to
the highest possible value:

	/*
	 * Initialize @nbcon_seq to the highest possible sequence number so
	 * that practically speaking it will have nothing to print until a
	 * desired initial sequence number has been set via nbcon_seq_force().
	 */
	atomic_long_set(&ACCESS_PRIVATE(con, nbcon_seq), ULSEQ_MAX(prb));

With the following ULSEQ_MAX macro added to printk_ringbuffer.h:

#ifdef CONFIG_64BIT
#define ULSEQ_MAX(rb) (-1)
#else
#define ULSEQ_MAX(rb) (prb_first_seq(rb) + 0x80000000)
#endif

This will allow the prb_read_valid() in nbcon_kthread_should_wakeup() to
return false and the kthread will sleep until @nbcon_seq has been
correctly setup and the kthread is woken when the "enabled" printk() is
called.

> +	}
> +
> +	console_srcu_read_unlock(cookie);
> +	return ret;
> +}

Other options would be to add extra checks to
nbcon_kthread_should_wakeup() or add some wait/notify code before
entering the kthread main loop. But both are overkill for this simple
startup scenario.

Thoughts?

John Ogness