Re: [PATCH printk v4 09/17] printk: nbcon: Rely on kthreads for normal operation

[John Ogness <john.ogness@linutronix.de>]

On 2024-08-27, Petr Mladek <pmladek@suse.com> wrote:
>> --- a/kernel/printk/printk.c
>> +++ b/kernel/printk/printk.c
>> @@ -2749,6 +2753,10 @@ void resume_console(void)
>>  	 */
>>  	synchronize_srcu(&console_srcu);
>>  
>> +	printk_get_console_flush_type(&ft);
>> +	if (ft.nbcon_offload)
>> +		nbcon_kthreads_wake();
>
> Who would flush the nbcon consoles in the following case?
>
>   ft.nbcon_atomic == true && ft.legacy_direct == false
>
> pr_flush() won't call the legacy loop in this case.
>
> We should probably update pr_flush() to call nbcon_atomic_flush_pending()
> in this case.

Agreed.

>> @@ -3387,9 +3405,25 @@ EXPORT_SYMBOL(console_stop);
>>  
>>  void console_start(struct console *console)
>>  {
>> +	struct console_flush_type ft;
>> +	bool is_nbcon;
>> +
>>  	console_list_lock();
>>  	console_srcu_write_flags(console, console->flags | CON_ENABLED);
>> +	is_nbcon = console->flags & CON_NBCON;
>>  	console_list_unlock();
>> +
>> +	/*
>> +	 * Ensure that all SRCU list walks have completed. The related
>> +	 * printing context must be able to see it is enabled so that
>> +	 * it is guaranteed to wake up and resume printing.
>> +	 */
>> +	synchronize_srcu(&console_srcu);
>> +
>> +	printk_get_console_flush_type(&ft);
>> +	if (is_nbcon && ft.nbcon_offload)
>> +		nbcon_kthread_wake(console);
>
> Same here.
>
>
>>  	__pr_flush(console, 1000, true);

Agreed, via adding nbcon_atomic_flush_pending() within __pr_flush().

>> @@ -4629,8 +4665,13 @@ EXPORT_SYMBOL_GPL(kmsg_dump_rewind);
>>   */
>>  void console_try_replay_all(void)
>>  {
>> +	struct console_flush_type ft;
>> +
>> +	printk_get_console_flush_type(&ft);
>>  	if (console_trylock()) {
>>  		__console_rewind_all();
>> +		if (ft.nbcon_offload)
>> +			nbcon_kthreads_wake();
>
> And here. We in this case, we should likely call add here:
>
> 		if (ft.nbcon_atomic)
> 			nbcon_atomic_flush_pending()

Agreed.

You once mentioned having a generic flush function that simply evaluates
the flush types and then executes them appropriately. It would be nice
to get rid of all/most these individual eval/flush snippets.

At a quick glance, the significant variations are:

- for legacy_direct: console_trylock() vs. console_lock() usage

- only performing direct/atomic flushing

- only performing offload flushing

- vprintk_emit(), console_start(), nbcon_device_release() are special

The first three variations could all be covered by arguments to the
generic flush function:

void console_flush(bool do_direct, bool do_offload, bool may_sleep);

or by packing it into bits/macros for readability, with call examples
such as:

console_flush(CON_FLUSH_OFFLOAD);
console_flush(CON_FLUSH_DIRECT | CON_FLUSH_OFFLOAD);
console_flush(CON_FLUSH_DIRECT | CON_FLUSH_MAY_SLEEP);

Such a function would have avoided the issues you found in this review.

John Ogness