On Thu 2025-11-13 11:17:42, John Ogness wrote:
On 2025-11-13, Petr Mladek pmladek@suse.com wrote:
I would prefer to keep all the printk_get_console_flush_type() changes since it returns proper available flush type information. If you would like to _additionally_ short-circuit __wake_up_klogd() and nbcon_kthreads_wake() in order to avoid all possible irq_work queueing, I would be OK with that.
Combining both approaches might be a bit messy. Some code paths might work correctly because of the explicit check and some just by chance.
But I got an idea. We could add a warning into __wake_up_klogd() and nbcon_kthreads_wake() to report when they are called unexpectedly.
And we should also prevent calling it from lib/nmi_backtrace.c.
I played with it and came up with the following changes on top of this patch:
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c index 334b4edff08c..e9290c418d12 100644 --- a/kernel/printk/printk.c +++ b/kernel/printk/printk.c @@ -4637,6 +4644,21 @@ void defer_console_output(void) __wake_up_klogd(PRINTK_PENDING_WAKEUP | PRINTK_PENDING_OUTPUT); } +void printk_try_flush(void) +{
- struct console_flush_type ft;
- printk_get_console_flush_type(&ft);
- if (ft.nbcon_atomic)
nbcon_atomic_flush_pending();For completeness, we should probably also have here:
if (ft.nbcon_offload) nbcon_kthreads_wake();
Makes sense. I think that did not add it because I got scared by the _wake suffix. I forgot that it just queued the irq_work.
- if (ft.legacy_direct) {
if (console_trylock())console_unlock();- }
- if (ft.legacy_offload)
defer_console_output();+}
void printk_trigger_flush(void) { defer_console_output(); diff --git a/lib/nmi_backtrace.c b/lib/nmi_backtrace.c index 33c154264bfe..632bbc28cb79 100644 --- a/lib/nmi_backtrace.c +++ b/lib/nmi_backtrace.c @@ -78,10 +78,10 @@ void nmi_trigger_cpumask_backtrace(const cpumask_t *mask, nmi_backtrace_stall_check(to_cpumask(backtrace_mask)); /*
* Force flush any remote buffers that might be stuck in IRQ context
* Try flushing messages added CPUs which might be stuck in IRQ context*/
- and therefore could not run their irq_work.
- printk_trigger_flush();
- printk_try_flush();
clear_bit_unlock(0, &backtrace_flag); put_cpu();
How does it look, please?
I like this. But I think the printk_try_flush() implementation should simply replace the implementation of printk_trigger_flush().
Make sense.
For the arch/powerpc/kernel/watchdog.c:watchdog_timer_interrupt() and lib/nmi_backtrace.c:nmi_trigger_cpumask_backtrace() sites I think it is appropriate.
Yup.
For the kernel/printk/nbcon.c:nbcon_device_release() site I think the call should be changed to defer_console_output():
diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c index 558ef31779760..73f315fd97a3e 100644 --- a/kernel/printk/nbcon.c +++ b/kernel/printk/nbcon.c @@ -1849,7 +1849,7 @@ void nbcon_device_release(struct console *con) if (console_trylock()) console_unlock(); } else if (ft.legacy_offload) {
printk_trigger_flush();
defer_console_output();
Looks good.
Can I fold all that into a new patch?
Go ahead.
Best Regards, Petr