Re: [PATCH printk v5 15/17] printk: Implement legacy printer kthread for PREEMPT_RT

[Petr Mladek <pmladek@suse.com>]

On Fri 2024-08-30 17:35:14, John Ogness wrote:
> The write() callback of legacy consoles usually makes use of
> spinlocks. This is not permitted with PREEMPT_RT in atomic
> contexts.
> 
> For PREEMPT_RT, create a new kthread to handle printing of all
> the legacy consoles (and nbcon consoles if boot consoles are
> registered). This allows legacy consoles to work on PREEMPT_RT
> without requiring modification. (However they will not have
> the reliability properties guaranteed by nbcon atomic
> consoles.)
> 
> Use the existing printk_kthreads_check_locked() to start/stop
> the legacy kthread as needed.
> 
> Introduce the macro force_legacy_kthread() to query if the
> forced threading of legacy consoles is in effect. Although
> currently only enabled for PREEMPT_RT, this acts as a simple
> mechanism for the future to allow other preemption models to
> easily take advantage of the non-interference property provided
> by the legacy kthread.
> 
> When force_legacy_kthread() is true, the legacy kthread
> fulfills the role of the console_flush_type @legacy_offload by
> waking the legacy kthread instead of printing via the
> console_lock in the irq_work. If the legacy kthread is not
> yet available, no legacy printing takes place (unless in
> panic).
> 
> If for some reason the legacy kthread fails to create, any
> legacy consoles are unregistered. With force_legacy_kthread(),
> the legacy kthread is a critical component for legacy consoles.
> 
> These changes only affect CONFIG_PREEMPT_RT.
> 
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -3459,6 +3475,87 @@ static int unregister_console_locked(struct console *console);
>  /* True when system boot is far enough to create printer threads. */
>  static bool printk_kthreads_ready __ro_after_init;
>  
> +static struct task_struct *printk_legacy_kthread;
> +
> +static bool legacy_kthread_should_wakeup(void)
> +{
> +	struct console_flush_type ft;
> +	struct console *con;
> +	bool ret = false;
> +	int cookie;
> +
> +	if (kthread_should_stop())
> +		return true;
> +
> +	printk_get_console_flush_type(&ft);
> +
> +	cookie = console_srcu_read_lock();
> +	for_each_console_srcu(con) {
> +		short flags = console_srcu_read_flags(con);
> +		u64 printk_seq;
> +
> +		/*
> +		 * The legacy printer thread is only for legacy consoles when
> +		 * the nbcon consoles have their printer threads.
> +		 */
> +		if ((flags & CON_NBCON) && ft.nbcon_offload)
> +			continue;

I am still scratching my head about the fact that the legacy loop
probably should not handle the nbcon consoles also when
printk_get_console_flush_type() returns ft.nbcon_atomic().

We probably does not have to take care of it here because this
code is called only when the legacy kthread is running.
It means that nbcon consoles should have their kthreads as well
when they can be handled outside the legacy loop. I mean
that we should never see ft.nbcon_atomic set here.

Sigh, the logic is so complicated.

Do I get it correctly, please?

> +		if (!console_is_usable(con, flags, false))
> +			continue;
> +
> +		if (flags & CON_NBCON) {
> +			printk_seq = nbcon_seq_read(con);
> +		} else {
> +			/*
> +			 * It is safe to read @seq because only this
> +			 * thread context updates @seq.
> +			 */
> +			printk_seq = con->seq;
> +		}
> +
> +		if (prb_read_valid(prb, printk_seq, NULL)) {
> +			ret = true;
> +			break;
> +		}
> +	}
> +	console_srcu_read_unlock(cookie);
> +
> +	return ret;
> +}

> --- a/kernel/printk/printk_safe.c
> +++ b/kernel/printk/printk_safe.c
> @@ -44,7 +44,9 @@ bool is_printk_legacy_deferred(void)
>  	 * The per-CPU variable @printk_context can be read safely in any
>  	 * context. CPU migration is always disabled when set.
>  	 */
> -	return (this_cpu_read(printk_context) || in_nmi());
> +	return (force_legacy_kthread() ||

This is not correct when used in panic(). force_legacy_kthread()
is not a reason for offload in that case.

IMHO, we should keep is_printk_legacy_deferred() as is.
Instead, we should check force_legacy_kthread() explicitly in
printk_get_console_flush_type(). It should cause the offload only
in NBCON_PRIO_NORMAL/EMERGENCY.

In fact, the legacy kthread should be used only in NBCON_PRIO_NORMAL.
The legacy loop should be called directly even in NBCON_PRIO_EMERGENCY.

> +		this_cpu_read(printk_context) ||
> +		in_nmi());
>  }
>  
>  asmlinkage int vprintk(const char *fmt, va_list args)

Best Regards,
Petr