[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <871q298y9r.fsf@jogness.linutronix.de>
Date: Tue, 27 Aug 2024 23:19:36 +0206
From: John Ogness <john.ogness@...utronix.de>
To: Petr Mladek <pmladek@...e.com>
Cc: Sergey Senozhatsky <senozhatsky@...omium.org>, Steven Rostedt
<rostedt@...dmis.org>, Thomas Gleixner <tglx@...utronix.de>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH printk v4 09/17] printk: nbcon: Rely on kthreads for
normal operation
On 2024-08-27, Petr Mladek <pmladek@...e.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
Powered by blists - more mailing lists