Message-ID: <ZnBUe0ZJgjbZXlAL@pathway.suse.cz>
Date: Mon, 17 Jun 2024 17:21:31 +0200
From: Petr Mladek <pmladek@...e.com>
To: John Ogness <john.ogness@...utronix.de>
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 v2 08/18] printk: nbcon: Stop threads on
 shutdown/reboot

On Tue 2024-06-04 01:30:43, John Ogness wrote:
> Register a syscore_ops shutdown function to stop all threaded
> printers on shutdown/reboot. This allows printk to cleanly
> transition back to atomic printing in order to provide a robust
> mechanism for outputting the final messages.

It looks like a simple straightforward patch. But there are some
hidden questions.

> --- a/kernel/printk/nbcon.c
> +++ b/kernel/printk/nbcon.c
> @@ -1703,3 +1704,33 @@ void nbcon_device_release(struct console *con)
>  	console_srcu_read_unlock(cookie);
>  }
>  EXPORT_SYMBOL_GPL(nbcon_device_release);
> +
> +/**
> + * printk_kthread_shutdown - shutdown all threaded printers
> + *
> + * On system shutdown all threaded printers are stopped. This allows printk
> + * to transition back to atomic printing, thus providing a robust mechanism
> + * for the final shutdown/reboot messages to be output.
> + */
> +static void printk_kthread_shutdown(void)

I would rename this to "printk_shutdown_threads() because it
is symmetric to printk_setup_threads().


> +{
> +	struct console *con;
> +
> +	console_list_lock();

First ideas (how the waterfall of questions started):

    + I though that we should do here:

	    printk_threads_enabled = false;

      It would tell vprintk_emit() to call nbcon_atomic_flush_pending().
      And it would be symetric with printk_setup_threads().


    + Also I though whether to call it before or after stopping the kthreads.

      If we set the variable before stopping kthreads then the kthreads
      might try to flush the messages in parallel with the flush from
      vprintk_emit(). It should be safe because it would be serialized
      via the nbcon console context. But it would be the first scenario
      where this happen => not much tested.

      Well, we should start the direct flush before stopping kthreads.
      So that we could see the messages when the stopping fails.


But then I realized:

Ha, vprintk_emit() would call nbcon_atomic_flush_pending() anyway
because these notifiers are called with:

	system_state > SYSTEM_RUNNING

This makes me wonder whether we need to stop the kthreads at all.
How exactly does this make printk more robust during shutdown?

Well, there is one differece. The kthreads could not interferre with
the direct flush when they are stopped.

But we have the same problem also with suspend/resume. And we do not
stop the kthreads in this case.

Maybe, we should just add a check of

       system_state > SYSTEM_RUNNING

also into the nbcon_kthread_func(). Maybe, the kthread should not try
to flush the messages when they are flushed directly by
vprintk_emit().

But then there is the problem with "the current owner is responsible
for flushing everything". vprintk_emit() could not flush the messages
when nbcon_atomic_flush_pending() races with the kthread. And if
the kthread stops flushing then nobody would flush the rest
of the pending messages.

Maybe, the notifier could just call pr_flush() like in suspend_console().
Maybe it is not important to stop the kthreads.

But maybe, the messages get flushed before the kthread is stopped.
Also the kthread could not interfere with direct flush once stopped.
Mabye, this is the idea behind "more robust during suspend".

But why is suspend solved another way then?
Is the suspend less important than shutdown?

My opinion:

I am not against stopping the kthreads. But the mechanism of switching
between kthreads and direct flush should be the same for both suspend
and shutdown. And I am not sure if we need to stop the kthreads then.

> +	for_each_console(con) {
> +		if (con->flags & CON_NBCON)
> +			nbcon_kthread_stop(con);
> +	}
> +	console_list_unlock();
> +}
> +
> +static struct syscore_ops printk_syscore_ops = {
> +	.shutdown = printk_kthread_shutdown,
> +};
> +
> +static int __init printk_init_ops(void)
> +{
> +	register_syscore_ops(&printk_syscore_ops);
> +	return 0;
> +}
> +device_initcall(printk_init_ops);

I guess that "device_initcall" is cut&pasted ;-) IMHO, it does not
fit much. That said, I do not have strong opinion where
it belongs.

IMHO, the best solution would be to register the notifier in
printk_setup_threads() before we actually allow creating them.
(Later update: if we really need to stop them).

Best Regards,
Petr

Powered by blists - more mailing lists