start&stop: was: Re: [PATCH printk v1 10/13] printk: add kthread console printers

[Petr Mladek <pmladek@suse.com>]

On Mon 2022-02-07 20:49:20, John Ogness wrote:
> Create a kthread for each console to perform console printing. During
> normal operation (@system_state == SYSTEM_RUNNING), the kthread
> printers are responsible for all printing on their respective
> consoles.
> 
> During non-normal operation, console printing is done as it has been:
> within the context of the printk caller or within irq work triggered
> by the printk caller.
> 
> Console printers synchronize against each other and against console
> lockers by taking the console lock for each message that is printed.
> ---
>  include/linux/console.h |   2 +
>  kernel/printk/printk.c  | 159 +++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 159 insertions(+), 2 deletions(-)
> 
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -3114,6 +3145,8 @@ void register_console(struct console *newcon)
>  		/* Begin with next message. */
>  		newcon->seq = prb_next_seq(prb);
>  	}
> +	if (kthreads_started)
> +		start_printk_kthread(newcon);

It is a corner case but we should handle possible error here.

The easiest solution is to stop all other printk kthreads and
use direct printing all the time. It should be enough because
it is a corner case. If kthreads could not be created then
the system is in big troubles anyway.

>  	console_unlock();
>  	console_sysfs_notify();
>  
> @@ -3170,6 +3203,11 @@ int unregister_console(struct console *console)
>  		}
>  	}
>  
> +	if (console->thread) {
> +		kthread_stop(console->thread);

I think that must not be called under console_lock or any other
locks that might be used by the kthread.

kthread_stop() waits until the kthread really exits. It means
that kthread_func() must check kthread_should_stop() and return.

There might be a deadlock when kthread_func() need to take some
locks in the main loop between kthread_should_stop() checks.

> +		console->thread = NULL;
> +	}
> +
>  	if (res)
>  		goto out_disable_unlock;
>  
> @@ -3345,6 +3390,116 @@ bool pr_flush(int timeout_ms, bool
> reset_on_progress)
[...]
> +}
> +
> +static int printk_kthread_func(void *data)
> +{
> +	struct console *con = data;
> +	char *dropped_text = NULL;
> +	char *ext_text = NULL;
> +	bool progress;
> +	bool handover;
> +	u64 seq = 0;
> +	char *text;
> +	int error;
> +
> +	pr_info("%sconsole [%s%d]: printing thread started\n",
> +		(con->flags & CON_BOOT) ? "boot" : "",
> +		con->name, con->index);

This should be printed after the allocations succeeded before entering
the main loop.

> +	text = kmalloc(CONSOLE_LOG_MAX, GFP_KERNEL);
> +	if (!text)
> +		goto out;
> +
> +	if (con->flags & CON_EXTENDED) {
> +		ext_text = kmalloc(CONSOLE_EXT_LOG_MAX, GFP_KERNEL);
> +		if (!ext_text)
> +			goto out;
> +	} else {
> +		dropped_text = kmalloc(DROPPED_TEXT_MAX, GFP_KERNEL);
> +		if (!dropped_text)
> +			goto out;
> +	}

here

> +	for (;;) {
> +		error = wait_event_interruptible(log_wait, printer_should_wake(con, seq));
> +
> +		if (kthread_should_stop())
> +			break;
> +
> +		if (error)
> +			continue;
> +
> +		do {

[...]

> +
> +/* Must be called within console_lock(). */
> +static void start_printk_kthread(struct console *con)
> +{
> +	con->thread = kthread_run(printk_kthread_func, con,
> +				  "pr/%s%d", con->name, con->index);
> +	if (IS_ERR(con->thread)) {
> +		con->thread = NULL;
> +		pr_err("%sconsole [%s%d]: unable to start printing thread\n",
> +			(con->flags & CON_BOOT) ? "boot" : "",
> +			con->name, con->index);
> +		return;

We need to pass the error down. The system has to switch to
direct printing off when any printk kthread could not be started.
Otherwise, the related console would not work.

> +	}
> +}
> +
>  /*
>   * Delayed printk version, for scheduler-internal messages:
>   */

Best Regards,
Petr