Received-SPF: pass (google.com: domain of linux-kernel+bounces-148738-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
From: John Ogness <john.ogness@linutronix.de>
To: Petr Mladek <pmladek@suse.com>
Cc: Sergey Senozhatsky <senozhatsky@chromium.org>, Steven Rostedt
 <rostedt@goodmis.org>, Thomas Gleixner <tglx@linutronix.de>,
 linux-kernel@vger.kernel.org, Greg Kroah-Hartman
 <gregkh@linuxfoundation.org>
Subject: Re: [PATCH printk v4 06/27] printk: nbcon: Add callbacks to
 synchronize with driver
In-Reply-To: <Zh_IrB4MyHwU8OJE@pathway.suse.cz>
References: <20240402221129.2613843-1-john.ogness@linutronix.de>
 <20240402221129.2613843-7-john.ogness@linutronix.de>
 <ZhUIatzxietD4F-m@localhost.localdomain>
 <87y19djqr0.fsf@jogness.linutronix.de> <Zh_IrB4MyHwU8OJE@pathway.suse.cz>
Date: Wed, 17 Apr 2024 17:00:42 +0206
Message-ID: <87bk68niod.fsf@jogness.linutronix.de>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain

On 2024-04-17, Petr Mladek <pmladek@suse.com> wrote:
>> We want to avoid using nbcon console ownership contention whenever
>> possible. In fact, there should _never_ be nbcon console owership
>> contention except for in emergency or panic situations.
>>
>> In the normal case, printk will use the driver-specific locking for
>> synchronization. Previously this was achieved by implementing the
>> lock/unlock within the write() callback. But with nbcon consoles that
>> is not possible because the nbcon ownership must be taken outside of
>> the write callback:
>> 
>> con->device_lock()
>> nbcon_acquire()
>> con->write_atomic() or con->write_thread()
>> nbcon_release()
>> con->device_unlock()
>
> This sounds like a strong requirement. So there should be a strong
> reason

There is: PREEMPT_RT

> when nbcon_acquire() is safe enough in emergency context
> then it should be safe enough in the normal context either.
> Otherwise, we would have a problem.

Of course. That is not the issue.

> My understanding is that we want to take con->device_lock()
> in the normal context from two reasons:
>
>   1. This is historical, king of speculation, and probably
>      not the real reason.

Correct. Not a reason.

>   2. The con->device() defines the context in which nbcon_acquire()
>      will be taken and con->write_atomic() called to make it
>      safe against other operations with the device driver.
>
>      For example, con->device() from uart serial consoles would
>      disable interrupts to prevent deadlocks with the serial
>      port IRQ handlers.
>
>      Some other drivers might just need to disable preemption.
>      And some (future) drivers might even allow to keep
>      the preemption enabled.

(Side note: In PREEMPT_RT, all drivers keep preemption enabled.)

This 2nd reason is almost correct.

Drivers are implemented using their own locking mechanisms. For UART it
will be spinlocks. For VT it will be mutexes. Whatever these mechanisms
are, that is what printk also wants to use. And since (for the normal
case) printk will always print console messages from task context,
drivers are free to use whatever locking mechanism fits them best. By
using the locking choice of the driver, printk will always do the right
thing and the author of that driver will always be in control of the
context.

Unfortunately printk also needs to deal with the non-normal case
(emergencies, panic, shutdown) when no printing threads are
available. For this (and only for this case) the nbcon console ownership
was introduced. It functions as a special[*] inner lock. This inner lock
will never be contended in the normal case. It exists only so that the
non-normal case can takeover the console for printing.

[*] Special = NMI-safe with priority and handover/takeover semantics.

Generally speaking, driver authors should not be concerned about this
special inner lock. It should be hidden (such as in the port lock
wrapper).

The special lock is interesting _only_ for drivers implementing
write_atomic(). And even then, it is only interesting for the
write_thread() and write_atomic() callback implementations. These
require some special handling to make sure they will print sane output
during handover/takeovers. But no other functions need to be concerned
about it.


> I still have to shake my head around this. But I would first like
> to know whether:
>
>    + You agree that nbcon_try_acquire() always have to be called with
>      preemption disabled.

No, it must not. PREEMPT_RT requires preemption enabled. That has always
been the core of this whole rework.

>    + What do you think about explicitly disabling preemption
>      in nbcon_try_acquire().

We cannot do it.

>    + If it is acceptable for the big picture. It should be fine for
>      serial consoles. But I think that graphics consoles wanted to
>      be preemptive when called in the printk kthread.

In PREEMPT_RT, all are preemptive.

> I am sure that it will be possible to make nbcon_try_acquire()
> preemption-safe but it will need some more magic.

I am still investigating why you think it is not safe (as an inner lock
for the normal case). Note that for emergency and panic situations,
preemption _is_ disabled.

John