Re: [PATCH printk v1 1/1] printk: Avoid scheduling irq_work on suspend

[Petr Mladek <pmladek@suse.com>]

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();
>  		}
>  	}
>  	console_srcu_read_unlock(cookie);

Looks good.

> Can I fold all that into a new patch?

Go ahead.

Best Regards,
Petr