main loop: was: Re: [PATCH printk v1 10/13] printk: add kthread console printers

[Petr Mladek]

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
> @@ -3345,6 +3390,116 @@ bool pr_flush(int timeout_ms, bool reset_on_progress)
>  }
>  EXPORT_SYMBOL(pr_flush);
>  
> +static bool printer_should_wake(struct console *con, u64 seq)
> +{
> +	short flags;
> +
> +	if (kthread_should_stop())
> +		return true;
> +
> +	if (console_suspended)
> +		return false;
> +
> +	/*
> +	 * This is an unsafe read to con->flags, but false positives
> +	 * are not an issue as long as they are rare.

This is not convincing. Anyway, it looks like an optimization. I
suggest to do the check under console_lock in the main loop.
printk kthread is a slow path anyway.

Also we should call there the entire console_is_usable().
It should do the same checks as in console_flush_all()
and the direct mode.

> +	 */
> +	flags = data_race(READ_ONCE(con->flags));
> +	if (!(flags & CON_ENABLED))
> +		return false;
> +
> +	return prb_read_valid(prb, seq, NULL);
> +}
> +
> +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);
> +
[...]
> +
> +	for (;;) {
> +		error = wait_event_interruptible(log_wait, printer_should_wake(con, seq));
> +
> +		if (kthread_should_stop())
> +			break;
> +
> +		if (error)
> +			continue;
> +
> +		do {
> +			console_lock();
> +			if (console_suspended) {
> +				console_unlock();
> +				break;
> +			}
> +

We should check here also console_is_usable() to be in sync with
console_flush_all().

> +			/*
> +			 * Even though the printk kthread is always preemptible, it is
> +			 * still not allowed to call cond_resched() from within
> +			 * console drivers. The task may become non-preemptible in the
> +			 * console driver call chain. For example, vt_console_print()
> +			 * takes a spinlock and then can call into fbcon_redraw(),
> +			 * which can conditionally invoke cond_resched().
> +			 */
> +			console_may_schedule = 0;

I do not understand the reason. How is this different from console_unlock()
and console_flush_all(), please?

> +			progress = console_emit_next_record(con, text, ext_text,
> +							    dropped_text, &handover);
> +			if (handover)
> +				break;
> +
> +			seq = con->seq;
> +
> +			/* Unlock console without invoking direct printing. */
> +			__console_unlock();
> +		} while (progress);
> +	}
> +out:
> +	kfree(dropped_text);
> +	kfree(ext_text);
> +	kfree(text);
> +	pr_info("%sconsole [%s%d]: printing thread stopped\n",
> +		(con->flags & CON_BOOT) ? "boot" : "",
> +		con->name, con->index);
> +	return 0;
> +}
> +

Best Regards,
Petr