Re: [PATCH printk v2 04/18] printk: nbcon: Introduce printing kthreads

[John Ogness]

On 2024-06-11, Petr Mladek <pmladek@xxxxxxxx> wrote:
>> During normal operation, the inner safety (console context) will never
>> be contended. That really only exists to synchronize the atomic case.
>
> I see. For example, the spin_lock_irqsave() in __uart_port_lock_irqsave()
> will be sleeping lock in RT => preserving RT guarantees. And it will
> make sure that the inner nbcon_context_try_acquire() would never
> have to spin => never break the RT guarantees.
>
> It makes perfect sense. Do I get it correctly?

Yes.

> Well, I would still like to describe the role of device_lock()
> for write_kthread(). It would help even me to create a better
> mental model ;-)
>
> What about the following?
>
> <proposal-2>
> 	/**
> 	 * @write_thread:
> 	 *
> 	 * NBCON callback to write out text in task context.
> 	 *
> 	 * This callback must be called only in task context with both
> 	 * device_lock() and the nbcon console acquired with
> 	 * NBCON_PRIO_NORMAL.
> 	 *
> 	 * The same rules for console ownership verification and unsafe
> 	 * sections handling applies as with write_atomic().
> 	 *
> 	 * The console ownership handling is necessary for synchronization
> 	 * against write_atomic() which is synchronized only via the context.
> 	 *
> 	 * The device_lock() provides the primary serialization for operations
> 	 * on the device. It might be as relaxed (mutex)[*] or as tight
> 	 * (disabled preemption and interrupts) as needed. It allows
> 	 * the kthread to operate in the least restrictive mode[**].
> 	 *
> 	 * [*] Standalone nbcon_context_try_acquire() is not safe with
> 	 *     the preemption enabled, see nbcon_owner_matches(). But it
> 	 *     can be safe when always called in the preemptive context
> 	 *     under the device_lock().
> 	 *
> 	 * [**] The device_lock() makes sure that nbcon_context_try_acquire()
> 	 *      would never need to spin which is important especially with
> 	 *      PREEMPT_RT.
> 	 */
> </proposal-2>

OK. I will use this.

>> --- a/kernel/printk/printk.c
>> +++ b/kernel/printk/printk.c
>> @@ -3995,6 +3995,8 @@ static int unregister_console_locked(struct console *console)
>>  	if (res > 0)
>>  		return 0;
>>  
>> +	__pr_flush(console, 1000, true);
>> +
>>  	/* Disable it unconditionally */
>>  	console_srcu_write_flags(console, console->flags & ~CON_ENABLED);
>
> Makes sense.
>
> And it actually means that it is too late to flush messages
> when kthread_should_stop() returns true. So, it does not matter if
> we check it in the while(backlog) loop or not. Well, it might be
> worth a comment in the code.

I will add a comment for this.

>> The thread failing to start is a serious issue. Particularly for
>> PREEMPT_RT.
>
> I agree.
>
>> Probably it should be something like:
>> 
>> 	if (WARN_ON(IS_ERR(kt))) {
>
> Might make sense.

I will add this for v2.

> Honestly, if the system is not able to start the kthread then
> it is probably useless anyway. I would prefer if printk keeps working
> so that people know what is going on ;-)

OK. For v2 I will change it to fallback to the legacy printing for those
consoles that do not have a kthread.

> After all, I would add two comments, like these:
>
> <proposal-2>
> 	/*
> 	 * Any access to the console device is serialized either by
> 	 * device_lock() or console context or both.
> 	 */
> 	kt = kthread_run(nbcon_kthread_func, con, "pr/%s%d", con->name,
> con->index);
> [...]
>
> 	/*
> 	 * Some users check con->kthread to decide whether to flush
> 	 * the messages directly using con->write_atomic(). But they
> 	 * do so only when the console is already in @console_list.
> 	 */

I do not understand how @console_list is related to racing between
non-thread and thread. kthreads are not only created during
registration. For example, they can be created much later when the last
boot console unregisters.

I am OK with the first comment of this proposal. I do not understand the
second comment.

> 	 con->kthread = kt;
> </proposal-2>

John