Re: [PATCH printk v4 06/17] printk: nbcon: Introduce printer kthreads

[Petr Mladek <pmladek@suse.com>]

On Tue 2024-08-27 06:49:22, John Ogness wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> Provide the main implementation for running a printer kthread
> per nbcon console that is takeover/handover aware. This
> includes:
> 
> - new mandatory write_thread() callback
> - kthread creation
> - kthread main printing loop
> - kthread wakeup mechanism
> - kthread shutdown
> 
> --- a/kernel/printk/nbcon.c
> +++ b/kernel/printk/nbcon.c
> @@ -1036,6 +1042,219 @@ static bool nbcon_emit_next_record(struct nbcon_write_context *wctxt, bool use_a
[...]
> +/*
> + * nbcon_kthread_stop - Stop a console printer thread
> + * @con:	Console to operate on
> + */
> +void nbcon_kthread_stop(struct console *con)
> +{
> +	lockdep_assert_console_list_lock_held();
> +
> +	if (!con->kthread)
> +		return;
> +
> +	kthread_stop(con->kthread);
> +	con->kthread = NULL;
> +}
> +
> +/**
> + * nbcon_kthread_create - Create a console printer thread
> + * @con:	Console to operate on
> + *
> + * Return:	True if the kthread was started or already exists.
> + *		Otherwise false and @con must not be registered.
> + *
> + * If @con was already registered, it must be unregistered before
> + * the global state variable @printk_kthreads_running can be set.

This paragraph is quite confusing without more context. I would
either remove it completely or write something like:

<proposal>
 * This function is called when nbcon consoles are supposed to be flushed
 * using the kthread. The messages printed with NBCON_PRIO_NORMAL are not
 * longer flushed by the legacy loop. This is why the failure is considered
 * fatal leading to the console unregistration.
</proposal>

> + */
> +bool nbcon_kthread_create(struct console *con)
> +{
> +	struct task_struct *kt;
> +
> +	lockdep_assert_console_list_lock_held();
> +
> +	if (con->kthread)
> +		return true;
> +
> +	kt = kthread_run(nbcon_kthread_func, con, "pr/%s%d", con->name, con->index);
> +	if (WARN_ON(IS_ERR(kt))) {
> +		con_printk(KERN_ERR, con, "failed to start printing thread\n");
> +		return false;
> +	}
> +
> +	con->kthread = kt;
> +
> +	return true;
> +}
> +
>  /* Track the nbcon emergency nesting per CPU. */
>  static DEFINE_PER_CPU(unsigned int, nbcon_pcpu_emergency_nesting);
>  static unsigned int early_nbcon_pcpu_emergency_nesting __initdata;
> @@ -1419,6 +1644,13 @@ bool nbcon_alloc(struct console *con)
>  			con_printk(KERN_ERR, con, "failed to allocate printing buffer\n");
>  			return false;
>  		}
> +
> +		if (printk_kthreads_running) {
> +			if (!nbcon_kthread_create(con)) {
> +				kfree(con->pbufs);

It probably is not much important but I would rather do here:

				con->pbufs = NULL;

> +				return false;
> +			}
> +		}
>  	}
>  
>  	return true;

Otherwise, it looks good. With the two proposed changes:

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr