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

[John Ogness]

On 2024-09-03, Petr Mladek <pmladek@xxxxxxxx> wrote:
>> +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().

The legacy loop kthread should never handle nbcon consoles unless a boot
console is registered. So it really needs to be:

if ((flags & CON_NBCON) && (ft.nbcon_offload || 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.

Remember that the nbcon kthreads are stopped on shutdown/reboot. For
those final messages, ft.nbcon_atomic=1. The legacy loop kthread should
not handled those printings.

> Sigh, the logic is so complicated.

It takes some getting used to. But I feel like the responsibilities and
transitions are clearer now than ever before.

>> --- 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.

If force_legacy_kthread() is true, legacy printing is _ALWAYS_ offloaded
except as a "hope and pray" last attempt via
console_flush_on_panic(). That is a requirement of PREEMPT_RT.

John