Console drivers typically must deal with access to the hardware
via user input/output (such as an interactive login shell) and
output of kernel messages via printk() calls.

Follow-up commits require that the printk subsystem is able to
synchronize with the driver. Require nbcon consoles to implement
two new callbacks (device_lock(), device_unlock()) that will
use whatever synchronization mechanism the driver is using for
itself (for example, the port lock for uart serial consoles).

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 include/linux/console.h | 42 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/include/linux/console.h b/include/linux/console.h
index e4028d4079e1..ad85594e070e 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -352,6 +352,48 @@ struct console {
 	 */
 	void (*write_atomic)(struct console *con, struct nbcon_write_context *wctxt);
 
+	/**
+	 * @device_lock:
+	 *
+	 * NBCON callback to begin synchronization with driver code.
+	 *
+	 * Console drivers typically must deal with access to the hardware
+	 * via user input/output (such as an interactive login shell) and
+	 * output of kernel messages via printk() calls. This callback is
+	 * called by the printk-subsystem whenever it needs to synchronize
+	 * with hardware access by the driver. It should be implemented to
+	 * use whatever synchronization mechanism the driver is using for
+	 * itself (for example, the port lock for uart serial consoles).
+	 *
+	 * This callback is always called from task context. It may use any
+	 * synchronization method required by the driver. BUT this callback
+	 * MUST also disable migration. The console driver may be using a
+	 * synchronization mechanism that already takes care of this (such as
+	 * spinlocks). Otherwise this function must explicitly call
+	 * migrate_disable().
+	 *
+	 * The flags argument is provided as a convenience to the driver. It
+	 * will be passed again to device_unlock(). It can be ignored if the
+	 * driver does not need it.
+	 */
+	void (*device_lock)(struct console *con, unsigned long *flags);
+
+	/**
+	 * @device_unlock:
+	 *
+	 * NBCON callback to finish synchronization with driver code.
+	 *
+	 * It is the counterpart to device_lock().
+	 *
+	 * This callback is always called from task context. It must
+	 * appropriately re-enable migration (depending on how device_lock()
+	 * disabled migration).
+	 *
+	 * The flags argument is the value of the same variable that was
+	 * passed to device_lock().
+	 */
+	void (*device_unlock)(struct console *con, unsigned long flags);
+
 	atomic_t		__private nbcon_state;
 	atomic_long_t		__private nbcon_seq;
 	struct printk_buffers	*pbufs;
-- 
2.39.2

On Wed 2024-04-03 00:17:08, John Ogness wrote:
> Console drivers typically must deal with access to the hardware
> via user input/output (such as an interactive login shell) and
> output of kernel messages via printk() calls.
> 
> Follow-up commits require that the printk subsystem is able to
> synchronize with the driver. Require nbcon consoles to implement
> two new callbacks (device_lock(), device_unlock()) that will
> use whatever synchronization mechanism the driver is using for
> itself (for example, the port lock for uart serial consoles).

We should explain here the bigger picture, see my comment
of the word "whenever" below.

<proposal>
Console drivers typically have to deal with access to the hardware
via user input/output (such as an interactive login shell) and
output of kernel messages via printk() calls.

They use some classic locking mechanism in most situations. But
console->write_atomic() callbacks, used by nbcon consoles,
must be synchronized only by acquiring the console context.

The synchronization via the console context ownership is possible
only when the console driver is registered. It is when a particular
device driver is connected with a particular console driver.

The two synchronization mechanisms must be synchronized between
each other. It is tricky because the console context ownership
is quite special. It might be taken over by a higher priority
context. Also CPU migration has to be disabled.

The most tricky part is to (dis)connect these two mechanism during
the console (un)registration. Let's make it easier by adding two new
callbacks: device_lock(), device_unlock(). They would allow
to take the device-specific lock while the device is being
(un)registered by the related console driver.

For example, these callbacks would lock/unlock the port lock
for serial port drivers.
</proposal>

> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> ---
>  include/linux/console.h | 42 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/include/linux/console.h b/include/linux/console.h
> index e4028d4079e1..ad85594e070e 100644
> --- a/include/linux/console.h
> +++ b/include/linux/console.h
> @@ -352,6 +352,48 @@ struct console {
>  	 */
>  	void (*write_atomic)(struct console *con, struct nbcon_write_context *wctxt);
>  
> +	/**
> +	 * @device_lock:
> +	 *
> +	 * NBCON callback to begin synchronization with driver code.
> +	 *
> +	 * Console drivers typically must deal with access to the hardware
> +	 * via user input/output (such as an interactive login shell) and
> +	 * output of kernel messages via printk() calls. This callback is
> +	 * called by the printk-subsystem whenever it needs to synchronize
> +	 * with hardware access by the driver.

The word "whenever" can be translated as "always" (by dictionary
and by my head ;-) Then the description sounds like this would be
the primary synchronization mechanism between printk and the driver.

In fact, this API would have only one purpose to synchronize
the console registration/unregistration.

IMHO, we should explain here the relation between the driver-specific
locking and nbcon console context locking. It would describe the big
picture and hopefully reduce confusion and eventual misuse.


> + It should be implemented to
> +	 * use whatever synchronization mechanism the driver is using for
> +	 * itself (for example, the port lock for uart serial consoles).
> +	 *
> +	 * This callback is always called from task context. It may use any
> +	 * synchronization method required by the driver. BUT this callback
> +	 * MUST also disable migration. The console driver may be using a
> +	 * synchronization mechanism that already takes care of this (such as
> +	 * spinlocks). Otherwise this function must explicitly call
> +	 * migrate_disable().
> +	 *
> +	 * The flags argument is provided as a convenience to the driver. It
> +	 * will be passed again to device_unlock(). It can be ignored if the
> +	 * driver does not need it.
> +	 */

<proposal>
	/**
	 * @device_lock:
	 *
	 * NBCON callback to serialize registration of a device driver
	 * for a console driver.
	 *
	 * Console drivers typically have to deal with access to the hardware
	 * via user input/output (such as an interactive login shell) and
	 * Output of kernel messages via printk() calls.
	 *
	 * They use some classic locking mechanism in most situations. But
	 * console->write_atomic() callbacks, used by nbcon consoles,
	 * must be synchronized only by acquiring the console context.
	 *
	 * The synchronization via the console context ownership is possible
	 * only when the console driver is registered. It is when a particular
	 * device driver is connected with a particular console driver.
	 *
	 * The device_lock() callback must block operations on the device
	 * while it is being (un)registered as a console driver. It will
	 * make sure that the classic device locking is aware of the console
	 * context locking when it might be acquired by the related nbcon
	 * console driver.
	 *
	 * This callback is always called from task context. It may use any
	 * synchronization method required by the driver, for example
	 * port lock for serial ports.
	 *
	 * IMPORTANT: This callback MUST also disable migration. The console
	 *	driver may be using a synchronization mechanism that already
	 *	takes care of this (such as spinlocks). Otherwise this function
	 *	must explicitly call migrate_disable().
	 *
	 *	The flags argument is provided as a convenience to the driver.
	 *	It will be passed again to device_unlock(). It can be ignored
	 *	if the driver does not need it.
	 */
</proposal>


> +	void (*device_lock)(struct console *con, unsigned long *flags);
> +
> +	/**
> +	 * @device_unlock:
> +	 *
> +	 * NBCON callback to finish synchronization with driver code.
> +	 *
> +	 * It is the counterpart to device_lock().
> +	 *
> +	 * This callback is always called from task context. It must
> +	 * appropriately re-enable migration (depending on how device_lock()
> +	 * disabled migration).
> +	 *
> +	 * The flags argument is the value of the same variable that was
> +	 * passed to device_lock().
> +	 */
> +	void (*device_unlock)(struct console *con, unsigned long flags);
> +
>  	atomic_t		__private nbcon_state;
>  	atomic_long_t		__private nbcon_seq;
>  	struct printk_buffers	*pbufs;

With the updated commit message and comment:

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

Best Regards,
Petr

On 2024-04-09, Petr Mladek <pmladek@suse.com> wrote:
>> Console drivers typically must deal with access to the hardware
>> via user input/output (such as an interactive login shell) and
>> output of kernel messages via printk() calls.
>> 
>> Follow-up commits require that the printk subsystem is able to
>> synchronize with the driver. Require nbcon consoles to implement
>> two new callbacks (device_lock(), device_unlock()) that will
>> use whatever synchronization mechanism the driver is using for
>> itself (for example, the port lock for uart serial consoles).
>
> We should explain here the bigger picture, see my comment
> of the word "whenever" below.
>
> <proposal>
> Console drivers typically have to deal with access to the hardware
> via user input/output (such as an interactive login shell) and
> output of kernel messages via printk() calls.
>
> They use some classic locking mechanism in most situations. But
> console->write_atomic() callbacks, used by nbcon consoles,
> must be synchronized only by acquiring the console context.
>
> The synchronization via the console context ownership is possible
> only when the console driver is registered. It is when a particular
> device driver is connected with a particular console driver.
>
> The two synchronization mechanisms must be synchronized between
> each other. It is tricky because the console context ownership
> is quite special. It might be taken over by a higher priority
> context. Also CPU migration has to be disabled.
>
> The most tricky part is to (dis)connect these two mechanism during
> the console (un)registration. Let's make it easier by adding two new
> callbacks: device_lock(), device_unlock(). They would allow
> to take the device-specific lock while the device is being
> (un)registered by the related console driver.
>
> For example, these callbacks would lock/unlock the port lock
> for serial port drivers.
> </proposal>

I do not like this proposal. IMHO it is focussing only on nbcon
registration details (a corner case) rather than presenting the big
picture.

Yes, we will use these callbacks during registration/unregistration to
avoid some races, but that is just one usage (and not the primary
one). The main reason these callbacks exist is to provide driver
synchronization when printing.

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()

How about this for the commit message:

printk: nbcon: Add callbacks to synchronize with driver

Console drivers typically must deal with access to the hardware
via user input/output (such as an interactive login shell) and
output of kernel messages via printk() calls. To provide the
necessary synchronization, usually some driver-specific locking
mechanism is used (for example, the port spinlock for uart
serial consoles).

Until now, usage of this driver-specific locking has been hidden
from the printk-subsystem and implemented within the various
console callbacks. However, for nbcon consoles, it is necessary
that the printk-subsystem uses the driver-specific locking so
that nbcon console ownership can be acquired _after_ the
driver-specific locking has succeeded. This allows for lock
contention to exist on the more context-friendly driver-specific
locking rather than nbcon console ownership (for non-emergency
and non-panic cases).

Require nbcon consoles to implement two new callbacks
(device_lock(), device_unlock()) that will use whatever
synchronization mechanism the driver is using for itself.

John

P.S. I think it makes more sense to use your proposal for the commit
message of the follow-up patch "printk: nbcon: Use driver
synchronization while registering".

On Tue 2024-04-16 17:07:39, John Ogness wrote:
> On 2024-04-09, Petr Mladek <pmladek@suse.com> wrote:
> >> Console drivers typically must deal with access to the hardware
> >> via user input/output (such as an interactive login shell) and
> >> output of kernel messages via printk() calls.
> >> 
> >> Follow-up commits require that the printk subsystem is able to
> >> synchronize with the driver. Require nbcon consoles to implement
> >> two new callbacks (device_lock(), device_unlock()) that will
> >> use whatever synchronization mechanism the driver is using for
> >> itself (for example, the port lock for uart serial consoles).
> >
> > We should explain here the bigger picture, see my comment
> > of the word "whenever" below.
> >
> > <proposal>
> > Console drivers typically have to deal with access to the hardware
> > via user input/output (such as an interactive login shell) and
> > output of kernel messages via printk() calls.
> >
> > They use some classic locking mechanism in most situations. But
> > console->write_atomic() callbacks, used by nbcon consoles,
> > must be synchronized only by acquiring the console context.
> >
> > The synchronization via the console context ownership is possible
> > only when the console driver is registered. It is when a particular
> > device driver is connected with a particular console driver.
> >
> > The two synchronization mechanisms must be synchronized between
> > each other. It is tricky because the console context ownership
> > is quite special. It might be taken over by a higher priority
> > context. Also CPU migration has to be disabled.
> >
> > The most tricky part is to (dis)connect these two mechanism during
> > the console (un)registration. Let's make it easier by adding two new
> > callbacks: device_lock(), device_unlock(). They would allow
> > to take the device-specific lock while the device is being
> > (un)registered by the related console driver.
> >
> > For example, these callbacks would lock/unlock the port lock
> > for serial port drivers.
> > </proposal>
> 
> I do not like this proposal. IMHO it is focussing only on nbcon
> registration details (a corner case) rather than presenting the big
> picture.

Fair enough.

> Yes, we will use these callbacks during registration/unregistration to
> avoid some races, but that is just one usage (and not the primary
> one). The main reason these callbacks exist is to provide driver
> synchronization when printing.
> 
> 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 ;-) I would like to better understand it [*] and
document it a clear way.

In principle, we do not need to take the full con->device_lock()
around nbcon_acquire() in the normal context. By other words,
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.

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.

     In the initial RFC, nbcon_try_acquire() allowed to take over
     the context with the same priority.

     The con->device() taken in the kthread might have prevented
     stealing the context by another "random" uart_port_lock() call from
     a code which would not continue emitting the messages.

     But the current nbcon_try_acquire() implementation does not take
     over the context with the same priority. So, this reason
     is not longet valid. But it probably has never been the 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.


I believe that the 2nd reason is that right one. But it is far
from obvious from the proposed comments.


[*] I am pretty sure that we have had the same discussion during
    Plumbers 2023. I am sorry I do not recall all the details now.
    Anyway, this should be explained in public anyway.


> How about this for the commit message:
> 
> printk: nbcon: Add callbacks to synchronize with driver
> 
> Console drivers typically must deal with access to the hardware
> via user input/output (such as an interactive login shell) and
> output of kernel messages via printk() calls. To provide the
> necessary synchronization, usually some driver-specific locking
> mechanism is used (for example, the port spinlock for uart
> serial consoles).
>
> Until now, usage of this driver-specific locking has been hidden
> from the printk-subsystem and implemented within the various
> console callbacks.

So far, so good.

> However, for nbcon consoles, it is necessary
> that the printk-subsystem uses the driver-specific locking so
> that nbcon console ownership can be acquired _after_ the
> driver-specific locking has succeeded. This allows for lock
> contention to exist on the more context-friendly driver-specific
> locking rather than nbcon console ownership (for non-emergency
> and non-panic cases).

I guess that the part:

   This allows for lock contention to exist on the more
   context-friendly driver-specific locking

tries to explain the 2nd reason that I have described above.
But it looks too cryptic to me. I have got it only because
I knew what I was looking for.

> Require nbcon consoles to implement two new callbacks
> (device_lock(), device_unlock()) that will use whatever
> synchronization mechanism the driver is using for itself.


Honestly, I still think that we need con->device_lock() primary
to synchronize the console registration and unregistration.

In all other cases, we only need to know whether we have
to call nbcon_try_acquire() with interrupts disabled or not[**].
And we need to know this for any nbcon_try_acquire() access,
including the emergency context.

Maybe, we should pass this information another way because
we do not want to call con->device_lock() in
nbcon_cpu_emergency_exit().

[**] According to my last findings, we always need to disable
     preemption, see
     https://lore.kernel.org/r/Zhj5uQ-JJnlIGUXK@localhost.localdomain


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.

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

   + 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.


I am sure that it will be possible to make nbcon_try_acquire()
preemption-safe but it will need some more magic. Maybe, we could
do it later when really needed. The primary target are the slow
serial consoles anyway.


> P.S. I think it makes more sense to use your proposal for the commit
> message of the follow-up patch "printk: nbcon: Use driver
> synchronization while registering".

Yup, feel free to use it.

Best Regards,
Petr

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

On Wed 2024-04-17 17:00:42, John Ogness wrote:
> 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

This explains it!

I think that a lot of misunderstanding here is caused because
your brain is trained primary in "RT mode" ;-) While I am not
that familiar with the RT tricks and my brain is thinking
in classic preemption mode :-)

I am not sure how it is done in other parts of kernel code where
RT needed to introduce some tricks. But I think that we should
really start mentioning RT behavior in the commit messages and
and comments where the RT mode makes huge changes.


> > 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 explains everything. It is a huge game changer.

Sigh, I remember that you told me this on Plumbers. But my
non-RT-trained  brain forgot this "detail". Well, I hope that
I am not the only one and we should mention this in the comments.

> > 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.

Got it! I have completely forgot that spin_lock() is a mutex in RT.

> >    + 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.

The race scenario has been mentioned in
https://lore.kernel.org/r/Zhj5uQ-JJnlIGUXK@localhost.localdomain

CPU0				CPU1

 [ task A ]

 nbcon_context_try_acquire()
   # success with NORMAL prio
   # .unsafe == false;  // safe for takeover

 [ schedule: task A -> B ]


				WARN_ON()
				  nbcon_atomic_flush_pending()
				    nbcon_context_try_acquire()
				      # success with EMERGENCY prio
				      # .unsafe == false;  // safe for takeover

				      # flushing
				      nbcon_context_release()

				      # HERE: con->nbcon_state is free
				      #       to take by anyone !!!


 nbcon_context_try_acquire()
   # success with NORMAL prio [ task B ]
   # .unsafe == false;  // safe for takeover

 [ schedule: task B -> A ]

 nbcon_enter_unsafe()
   nbcon_context_can_proceed()

BUG: nbcon_context_can_proceed() returns "true" because
     the console is owned by a context on CPU0 with
     NBCON_PRIO_NORMAL.

     But it should return "false". The console is owned
     by a context from task B and we do the check
     in a context from task A.


OK, let's look at it with the new RT perspective. Here, the
con->device_lock() plays important role.

The race could NOT happen in:

   + NBCON_PRIO_PANIC context because it does not schedule

   + NBCON_PRIO_EMERGENCY context because we explicitly disable preemption there

   + NBCON_NORMAL_PRIO context when we ALWAYS do nbcon_try_acquire() under
     con->device() lock. Here the con->device_lock() serializes
     nbcon_try_acquire() calls even between running tasks.


Everything makes sense now. And we are probable safe.

I have to double check that we really ALWAYS call nbcon_try_acquire()
under con->device() lock. And I have to think how to describe this
in the commit messages and comments.

Best Regards,
Petr

On 2024-04-18, Petr Mladek <pmladek@suse.com> wrote:
> I am not sure how it is done in other parts of kernel code where
> RT needed to introduce some tricks. But I think that we should
> really start mentioning RT behavior in the commit messages and
> and comments where the RT mode makes huge changes.

Yes, our motivation is RT. But these semantics are not RT-specific. They
apply to the general kernel locking model. For example, even for a !RT
system, it is semantically incorrect to take a spin_lock while holding a
raw_spin_lock.

In the full PREEMPT_RT series I have tried to be careful about only
mentioning PREEMPT_RT when it is really PREEMPT_RT-specific. For example
[0][1][2].

[0] https://git.kernel.org/pub/scm/linux/kernel/git/rt/linux-rt-devel.git/commit/?h=linux-6.9.y-rt-rebase&id=1564af55a92c32fe215af35cf55cb9359c5fff30

[1] https://git.kernel.org/pub/scm/linux/kernel/git/rt/linux-rt-devel.git/commit/?h=linux-6.9.y-rt-rebase&id=033b416ad25b17dc60d5f71c1a0b33a5fbc17639

[2] https://git.kernel.org/pub/scm/linux/kernel/git/rt/linux-rt-devel.git/commit/?h=linux-6.9.y-rt-rebase&id=7929ba9e5c110148a1fcd8bd93d6a4eff37aa265

> The race could NOT happen in:
>
>    + NBCON_PRIO_PANIC context because it does not schedule

Yes.

>    + NBCON_PRIO_EMERGENCY context because we explicitly disable
>      preemption there

Yes.

>    + NBCON_NORMAL_PRIO context when we ALWAYS do nbcon_try_acquire()
>      under con->device() lock. Here the con->device_lock() serializes
>      nbcon_try_acquire() calls even between running tasks.

The nbcon_legacy_emit_next_record() printing as NBCON_NORMAL_PRIO is a
special situation where write_atomic() is used. It is safe because it
disables hard interrupts and is never called from NMI context.

nbcon_atomic_flush_pending() as NBCON_NORMAL_PRIO is safe in !NMI
because it also disables hard interrupts. However,
nbcon_atomic_flush_pending() could be called in NMI with
NBCON_NORMAL_PRIO. I need to think about this case.

John

On Thu 2024-04-18 14:16:16, John Ogness wrote:
> On 2024-04-18, Petr Mladek <pmladek@suse.com> wrote:
> > I am not sure how it is done in other parts of kernel code where
> > RT needed to introduce some tricks. But I think that we should
> > really start mentioning RT behavior in the commit messages and
> > and comments where the RT mode makes huge changes.
> 
> Yes, our motivation is RT. But these semantics are not RT-specific. They
> apply to the general kernel locking model.

Yes, but RT is a nice example where it is clear what want to achieve.
IMHO, a clear example is always better then a scientific formulation
where every word might be important. Especially when different people
might understand some words different ways.


> For example, even for a !RT system, it is semantically incorrect to
> take a spin_lock while holding a raw_spin_lock.

Really? I am not aware of it. I know that lockdep complains even
in no-RT configuration. But I have expected that it only helps
to catch potential problems when the same code is used with
RT enabled.

Is there any difference between spin_lock() and raw_spin_lock()
when RT is disabled. I do not see any. This is from
include/linux/spinlock.h:

	/* Non PREEMPT_RT kernel, map to raw spinlocks: */
	#ifndef CONFIG_PREEMPT_RT
	[...]
	static __always_inline void spin_lock(spinlock_t *lock)
	{
		raw_spin_lock(&lock->rlock);
	}

Would raw_spinlock() API exist without CONFIG_PREEMPT_RT?

Maybe, you do not understand what I suggest. Let's talk about
particular comments in the code.


> In the full PREEMPT_RT series I have tried to be careful about only
> mentioning PREEMPT_RT when it is really PREEMPT_RT-specific. For example
> [0][1][2].
> 
> [0] https://git.kernel.org/pub/scm/linux/kernel/git/rt/linux-rt-devel.git/commit/?h=linux-6.9.y-rt-rebase&id=1564af55a92c32fe215af35cf55cb9359c5fff30
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/rt/linux-rt-devel.git/commit/?h=linux-6.9.y-rt-rebase&id=033b416ad25b17dc60d5f71c1a0b33a5fbc17639
> 
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/rt/linux-rt-devel.git/commit/?h=linux-6.9.y-rt-rebase&id=7929ba9e5c110148a1fcd8bd93d6a4eff37aa265
> 
> > The race could NOT happen in:
> >
> >    + NBCON_PRIO_PANIC context because it does not schedule
> 
> Yes.
> 
> >    + NBCON_PRIO_EMERGENCY context because we explicitly disable
> >      preemption there
> 
> Yes.
> 
> >    + NBCON_NORMAL_PRIO context when we ALWAYS do nbcon_try_acquire()
> >      under con->device() lock. Here the con->device_lock() serializes
> >      nbcon_try_acquire() calls even between running tasks.
> 
> The nbcon_legacy_emit_next_record() printing as NBCON_NORMAL_PRIO is a
> special situation where write_atomic() is used. It is safe because it
> disables hard interrupts and is never called from NMI context.
> 
> nbcon_atomic_flush_pending() as NBCON_NORMAL_PRIO is safe in !NMI
> because it also disables hard interrupts. However,
> nbcon_atomic_flush_pending() could be called in NMI with
> NBCON_NORMAL_PRIO. I need to think about this case.

It is safe. The race scenario requires _double_ scheduling (A->B->A):

 1. [CPU 0]: process A acquires the context and is scheduled (CPU 0)

 2. [CPU 1] The nbcon context is taken over and released in emergency.

 3. [CPU 0] process B acquires the context and is scheduled

 4. [CPU 0] process A thinks that it still owns the context
	    and continue when it ended.


This could not happen with the current code when:

   + nbcon_try_acquire() is serialized by con->device_lock()
     because process B would get blocked on this lock.

   + nbcon_try_acquire() is called in atomic context
     because the context is always released before scheduling.


I would say that this is far from obvious and we really need
to document this somehow. I would mention these details above
nbcon_context_try_acquire().

Best Regards,
Petr