linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] printk: Skip console drivers on PREEMPT_RT.
@ 2022-07-20 15:48 Sebastian Andrzej Siewior
  2022-07-20 16:26 ` John Ogness
  0 siblings, 1 reply; 15+ messages in thread
From: Sebastian Andrzej Siewior @ 2022-07-20 15:48 UTC (permalink / raw)
  To: linux-kernel
  Cc: Petr Mladek, Sergey Senozhatsky, Steven Rostedt, John Ogness,
	Thomas Gleixner

printk might be invoked in a context with disabled interrupts and or
preemption and additionally disables interrupts before it invokes the
console drivers. This is behaviour is not compatible with PREEMPT_RT.

Disable console printing until the return of atomic consoles and the
printing thread. This allows to retrieve the log buffer from user space
which is not possible by disable printk.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 kernel/printk/printk.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2843,6 +2843,16 @@ void console_unlock(void)
 	}
 
 	/*
+	 * On PREEMPT_RT it is not possible to invoke console drivers with
+	 * disabled interrupts and or preemption. Therefore all drivers are
+	 * skipped and the output can be retrieved from the buffer.
+	 */
+	if (IS_ENABLED(CONFIG_PREEMPT_RT)) {
+		up_console_sem();
+		return;
+	}
+
+	/*
 	 * Console drivers are called with interrupts disabled, so
 	 * @console_may_schedule should be cleared before; however, we may
 	 * end up dumping a lot of lines, for example, if called from

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] printk: Skip console drivers on PREEMPT_RT.
  2022-07-20 15:48 [PATCH] printk: Skip console drivers on PREEMPT_RT Sebastian Andrzej Siewior
@ 2022-07-20 16:26 ` John Ogness
  2022-07-20 16:35   ` Sebastian Andrzej Siewior
  0 siblings, 1 reply; 15+ messages in thread
From: John Ogness @ 2022-07-20 16:26 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior, linux-kernel
  Cc: Petr Mladek, Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner

Hi Sebastian,

On 2022-07-20, Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
> printk might be invoked in a context with disabled interrupts and or
> preemption and additionally disables interrupts before it invokes the
> console drivers. This is behaviour is not compatible with PREEMPT_RT.
>
> Disable console printing until the return of atomic consoles and the
> printing thread. This allows to retrieve the log buffer from user space
> which is not possible by disable printk.
>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  kernel/printk/printk.c |   10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2843,6 +2843,16 @@ void console_unlock(void)
>  	}
>  
>  	/*
> +	 * On PREEMPT_RT it is not possible to invoke console drivers with
> +	 * disabled interrupts and or preemption. Therefore all drivers are
> +	 * skipped and the output can be retrieved from the buffer.
> +	 */
> +	if (IS_ENABLED(CONFIG_PREEMPT_RT)) {
> +		up_console_sem();

This should be:

                __console_unlock();

> +		return;
> +	}

Note that if @console_may_schedule is 1, then we are in a sleepable
context and could print. But since that is not very often, it is
probably better to just have it off all the time as you propose.

John Ogness

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] printk: Skip console drivers on PREEMPT_RT.
  2022-07-20 16:26 ` John Ogness
@ 2022-07-20 16:35   ` Sebastian Andrzej Siewior
  2022-07-20 17:50     ` John Ogness
  0 siblings, 1 reply; 15+ messages in thread
From: Sebastian Andrzej Siewior @ 2022-07-20 16:35 UTC (permalink / raw)
  To: John Ogness
  Cc: linux-kernel, Petr Mladek, Sergey Senozhatsky, Steven Rostedt,
	Thomas Gleixner

On 2022-07-20 18:32:43 [+0206], John Ogness wrote:
> Hi Sebastian,
Hi,

> > --- a/kernel/printk/printk.c
> > +++ b/kernel/printk/printk.c
> > @@ -2843,6 +2843,16 @@ void console_unlock(void)
> >  	}
> >  
> >  	/*
> > +	 * On PREEMPT_RT it is not possible to invoke console drivers with
> > +	 * disabled interrupts and or preemption. Therefore all drivers are
> > +	 * skipped and the output can be retrieved from the buffer.
> > +	 */
> > +	if (IS_ENABLED(CONFIG_PREEMPT_RT)) {
> > +		up_console_sem();
> 
> This should be:
> 
>                 __console_unlock();

Why?

> > +		return;
> > +	}
> 
> Note that if @console_may_schedule is 1, then we are in a sleepable
> context and could print. But since that is not very often, it is
> probably better to just have it off all the time as you propose.

No we can't coz printk disables interrupts before invoking the console
drivers.

> John Ogness

Sebastian

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] printk: Skip console drivers on PREEMPT_RT.
  2022-07-20 16:35   ` Sebastian Andrzej Siewior
@ 2022-07-20 17:50     ` John Ogness
  2022-07-21  6:50       ` [PATCH v2] " Sebastian Andrzej Siewior
  0 siblings, 1 reply; 15+ messages in thread
From: John Ogness @ 2022-07-20 17:50 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior
  Cc: linux-kernel, Petr Mladek, Sergey Senozhatsky, Steven Rostedt,
	Thomas Gleixner

On 2022-07-20, Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
>> > --- a/kernel/printk/printk.c
>> > +++ b/kernel/printk/printk.c
>> > @@ -2843,6 +2843,16 @@ void console_unlock(void)
>> >  	}
>> >  
>> >  	/*
>> > +	 * On PREEMPT_RT it is not possible to invoke console drivers with
>> > +	 * disabled interrupts and or preemption. Therefore all drivers are
>> > +	 * skipped and the output can be retrieved from the buffer.
>> > +	 */
>> > +	if (IS_ENABLED(CONFIG_PREEMPT_RT)) {
>> > +		up_console_sem();
>> 
>> This should be:
>> 
>>                 __console_unlock();
>
> Why?

Because that is the official function that performs the console
unlocking. With upcoming patches, that function does more. But even
without those patches, by directly calling up_console_sem() you are
leaving @console_locked set. The @console_locked variable is used by the
vt code to validate correct console locking.

I realize you are just copy/pasting from the condition above (checking
@console_suspended), but that condition is special. When consoles are
suspended, we still allow @console_sem to be locked/unlocked, but
without changing the locked status of the console. See suspend_console()
and resume_console() if you really want to see the gory details.

John

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v2] printk: Skip console drivers on PREEMPT_RT.
  2022-07-20 17:50     ` John Ogness
@ 2022-07-21  6:50       ` Sebastian Andrzej Siewior
  2022-07-22 12:39         ` Petr Mladek
  0 siblings, 1 reply; 15+ messages in thread
From: Sebastian Andrzej Siewior @ 2022-07-21  6:50 UTC (permalink / raw)
  To: John Ogness
  Cc: linux-kernel, Petr Mladek, Sergey Senozhatsky, Steven Rostedt,
	Thomas Gleixner

printk might be invoked in a context with disabled interrupts and or
preemption and additionally disables interrupts before it invokes the
console drivers. This is behaviour is not compatible with PREEMPT_RT.

Disable console printing until the return of atomic consoles and the
printing thread. This allows to retrieve the log buffer from user space
which is not possible by disable printk.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
v1…v2:
   - Use __console_unlock() as suggested by John.

 kernel/printk/printk.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2843,6 +2843,16 @@ void console_unlock(void)
 	}
 
 	/*
+	 * On PREEMPT_RT it is not possible to invoke console drivers with
+	 * disabled interrupts and or preemption. Therefore all drivers are
+	 * skipped and the output can be retrieved from the buffer.
+	 */
+	if (IS_ENABLED(CONFIG_PREEMPT_RT)) {
+		__console_unlock();
+		return;
+	}
+
+	/*
 	 * Console drivers are called with interrupts disabled, so
 	 * @console_may_schedule should be cleared before; however, we may
 	 * end up dumping a lot of lines, for example, if called from

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2] printk: Skip console drivers on PREEMPT_RT.
  2022-07-21  6:50       ` [PATCH v2] " Sebastian Andrzej Siewior
@ 2022-07-22 12:39         ` Petr Mladek
  2022-07-22 17:03           ` Sebastian Andrzej Siewior
  0 siblings, 1 reply; 15+ messages in thread
From: Petr Mladek @ 2022-07-22 12:39 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior
  Cc: John Ogness, linux-kernel, Sergey Senozhatsky, Steven Rostedt,
	Thomas Gleixner

On Thu 2022-07-21 08:50:38, Sebastian Andrzej Siewior wrote:
> printk might be invoked in a context with disabled interrupts and or
> preemption and additionally disables interrupts before it invokes the
> console drivers. This is behaviour is not compatible with PREEMPT_RT.

Maybe I do not understand it correctly. It sounds like we could not
disable interrupts when interrupts or preemption is already disabled.
Like nested disablement of interrupts is bad.

Is this a generic rule? Is is about the nesting?

Or is is somehow specific to the console drivers called from printk()
directly? Do you always want to disable here because it might
be an atomic context and they might take too long?

I guess that the sentence "additionally disables interrupts before
it invokes the console drivers" is not really important" and it confused me.


> Disable console printing until the return of atomic consoles and the
> printing thread. This allows to retrieve the log buffer from user space
> which is not possible by disable printk.

I guess that this is for RT tree because the kthreads and the atomic
consoles are still not in the mainline.

> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2843,6 +2843,16 @@ void console_unlock(void)
>  	}
>  
>  	/*
> +	 * On PREEMPT_RT it is not possible to invoke console drivers with
> +	 * disabled interrupts and or preemption. Therefore all drivers are
> +	 * skipped and the output can be retrieved from the buffer.
> +	 */
> +	if (IS_ENABLED(CONFIG_PREEMPT_RT)) {
> +		__console_unlock();
> +		return;
> +	}

Do you want this even in panic() or early boot?

AFAIK, only the serial console has atomic write() callback in the RT
tree. Is this the only console used by RT kernel users in practice?


> +	/*
>  	 * Console drivers are called with interrupts disabled, so
>  	 * @console_may_schedule should be cleared before; however, we may
>  	 * end up dumping a lot of lines, for example, if called from

Best Regards,
Petr

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2] printk: Skip console drivers on PREEMPT_RT.
  2022-07-22 12:39         ` Petr Mladek
@ 2022-07-22 17:03           ` Sebastian Andrzej Siewior
  2022-07-25 12:30             ` Petr Mladek
  0 siblings, 1 reply; 15+ messages in thread
From: Sebastian Andrzej Siewior @ 2022-07-22 17:03 UTC (permalink / raw)
  To: Petr Mladek
  Cc: John Ogness, linux-kernel, Sergey Senozhatsky, Steven Rostedt,
	Thomas Gleixner

On 2022-07-22 14:39:44 [+0200], Petr Mladek wrote:
> On Thu 2022-07-21 08:50:38, Sebastian Andrzej Siewior wrote:
> > printk might be invoked in a context with disabled interrupts and or
> > preemption and additionally disables interrupts before it invokes the
> > console drivers. This is behaviour is not compatible with PREEMPT_RT.
> 
> Maybe I do not understand it correctly. It sounds like we could not
> disable interrupts when interrupts or preemption is already disabled.
> Like nested disablement of interrupts is bad.
> 
> Is this a generic rule? Is is about the nesting?

You must not invoke the console drivers with disabled interrupts. This is
bad. So even if the context you were called from has interrupts enabled
then in console_emit_next_record() you have printk_safe_enter_irqsave()
before call_console_driver() which disables interrupts and this is bad.
More below…

> Or is is somehow specific to the console drivers called from printk()
> directly? Do you always want to disable here because it might
> be an atomic context and they might take too long?

You can't acquire a sleeping lock with disabled interrupts and or
preemption. Therefore the console drivers must not be invoked because
they need to acquire a sleeping lock(s).

> I guess that the sentence "additionally disables interrupts before
> it invokes the console drivers" is not really important" and it confused me.

This refers to printk_safe_enter_irqsave(). You could argue that it is
safe to invoke the console drivers if the context, in which printk() is
invoked, is safe. However this is not possible because printk disables
interrupts prio invoking the console drivers as just explained.
Therefore I don't see a way how to invoke the console drivers on RT as
of v5.19-rc7.

> 
> > Disable console printing until the return of atomic consoles and the
> > printing thread. This allows to retrieve the log buffer from user space
> > which is not possible by disable printk.
> 
> I guess that this is for RT tree because the kthreads and the atomic
> consoles are still not in the mainline.

I would like to have this applied to the v5.20 upstream tree and then
revoked once the missing bits have been  merged. Based on what I see,
there shouldn't be any road blocks.

> > --- a/kernel/printk/printk.c
> > +++ b/kernel/printk/printk.c
> > @@ -2843,6 +2843,16 @@ void console_unlock(void)
> >  	}
> >  
> >  	/*
> > +	 * On PREEMPT_RT it is not possible to invoke console drivers with
> > +	 * disabled interrupts and or preemption. Therefore all drivers are
> > +	 * skipped and the output can be retrieved from the buffer.
> > +	 */
> > +	if (IS_ENABLED(CONFIG_PREEMPT_RT)) {
> > +		__console_unlock();
> > +		return;
> > +	}
> 
> Do you want this even in panic() or early boot?

yes. I don't see a way to invoke the console drivers without the
print-kthread.

> AFAIK, only the serial console has atomic write() callback in the RT
> tree. Is this the only console used by RT kernel users in practice?

The atomic console is made for emergencies. Everything else should be
written using the printk thread.

> 
> > +	/*
> >  	 * Console drivers are called with interrupts disabled, so
> >  	 * @console_may_schedule should be cleared before; however, we may
> >  	 * end up dumping a lot of lines, for example, if called from
> 
> Best Regards,
> Petr

Sebastian

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2] printk: Skip console drivers on PREEMPT_RT.
  2022-07-22 17:03           ` Sebastian Andrzej Siewior
@ 2022-07-25 12:30             ` Petr Mladek
  2022-07-25 12:51               ` John Ogness
  2022-07-25 13:55               ` Sebastian Andrzej Siewior
  0 siblings, 2 replies; 15+ messages in thread
From: Petr Mladek @ 2022-07-25 12:30 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior
  Cc: John Ogness, linux-kernel, Sergey Senozhatsky, Steven Rostedt,
	Thomas Gleixner

On Fri 2022-07-22 19:03:49, Sebastian Andrzej Siewior wrote:
> On 2022-07-22 14:39:44 [+0200], Petr Mladek wrote:
> > On Thu 2022-07-21 08:50:38, Sebastian Andrzej Siewior wrote:
> > > printk might be invoked in a context with disabled interrupts and or
> > > preemption and additionally disables interrupts before it invokes the
> > > console drivers. This is behaviour is not compatible with PREEMPT_RT.
> > 
> > Maybe I do not understand it correctly.
> 
> > Or is is somehow specific to the console drivers called from printk()
> > directly?
> 
> You can't acquire a sleeping lock with disabled interrupts and or

This is what I have missed. It might be obvious for people living by RT
kernel. But spinlocks do not sleep in normal kernel. So I did not get
that the spinlocks are the culprit. Please, make it more obvious
in the commit message, for example:

--- cut ---
Console drivers use spinlocks that might sleep in PREEMPT_RT. As a
result they must not be called with interrupts enabled...
--- cut ---

> > > Disable console printing until the return of atomic consoles and the
> > > printing thread. This allows to retrieve the log buffer from user space
> > > which is not possible by disable printk.
> > 
> > I guess that this is for RT tree because the kthreads and the atomic
> > consoles are still not in the mainline.
> 
> I would like to have this applied to the v5.20 upstream tree and then
> revoked once the missing bits have been  merged. Based on what I see,
> there shouldn't be any road blocks.

Huh, I do not think that it is a good idea. There are neither atomic
consoles nor printk kthreads in upstream. The patch effectively
completely disables the consoles in PREEMPT_RT. All consoles
will be _empty all the time_.

Also the consoles were never tested with interrupts enabled. I am
pretty sure that interrupts has to be disabled in some locations,
for example, when sending some data sequences on the ports. Are we
sure that they are always explicitly disabled inside the drivers?
This is one reason why we reverted the console kthreads in 5.19-rc4.
AFAIK, John is doing some inspection of all drivers.

That said, I am going to leave the decision on John. I am not sure
what are the expectations of RT users in mainline.

From my point, this patch does not make much sense. IMHO, it will
not make mainline usable with PREEMPT_RT. Any serious RT user will
need to revert it and apply a better printk solution from
the out-of-tree RT patchset.

Best Regards,
Petr

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2] printk: Skip console drivers on PREEMPT_RT.
  2022-07-25 12:30             ` Petr Mladek
@ 2022-07-25 12:51               ` John Ogness
  2022-07-25 13:55               ` Sebastian Andrzej Siewior
  1 sibling, 0 replies; 15+ messages in thread
From: John Ogness @ 2022-07-25 12:51 UTC (permalink / raw)
  To: Petr Mladek, Sebastian Andrzej Siewior
  Cc: linux-kernel, Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner

Hi Petr,

On 2022-07-25, Petr Mladek <pmladek@suse.com> wrote:
> From my point, this patch does not make much sense. IMHO, it will
> not make mainline usable with PREEMPT_RT. Any serious RT user will
> need to revert it and apply a better printk solution from
> the out-of-tree RT patchset.

The problem is that direct console printing cannot work with
PREEMPT_RT, even in a panic situation. And it never will (using "normal"
console drivers).

PREEMPT_RT is basically mainline now except for the kthread printers and
atomic consoles. But these features will not be available in mainline so
soon. (Atomic consoles have not gone through the LKML review process at
all yet.)

We see value in allowing PREEMPT_RT to be available now, even if it
means no console printing. You claim "any serious RT user" needs console
printing, but for production RT systems, the console is probably
disabled anyway.

We could solve this with kconfig tricks, making console kconfig options
depend on !PREEMPT_RT. But that would cause the console config of users
to be disabled, making it more inconvenient for them to turn it back on
once these features are available.

A software switch (as implemented by Sebastian's patch) may provide the
simplest transition. A 5.20 PREEMPT_RT would not have console printing,
but a later version (using the same config) would.

John

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2] printk: Skip console drivers on PREEMPT_RT.
  2022-07-25 12:30             ` Petr Mladek
  2022-07-25 12:51               ` John Ogness
@ 2022-07-25 13:55               ` Sebastian Andrzej Siewior
  2022-07-25 14:24                 ` Petr Mladek
  1 sibling, 1 reply; 15+ messages in thread
From: Sebastian Andrzej Siewior @ 2022-07-25 13:55 UTC (permalink / raw)
  To: Petr Mladek
  Cc: John Ogness, linux-kernel, Sergey Senozhatsky, Steven Rostedt,
	Thomas Gleixner

On 2022-07-25 14:30:04 [+0200], Petr Mladek wrote:
> This is what I have missed. It might be obvious for people living by RT
> kernel. But spinlocks do not sleep in normal kernel. So I did not get
> that the spinlocks are the culprit. Please, make it more obvious
> in the commit message, for example:
> 
> --- cut ---
> Console drivers use spinlocks that might sleep in PREEMPT_RT. As a
> result they must not be called with interrupts enabled...
> --- cut ---

It is not only the sleeping lock by itself but also the fact that
polling on 115200 or 9600 to print a line is simply takes too much time.
Even if the line consists of four letters.

> > > > Disable console printing until the return of atomic consoles and the
> > > > printing thread. This allows to retrieve the log buffer from user space
> > > > which is not possible by disable printk.
> > > 
> > > I guess that this is for RT tree because the kthreads and the atomic
> > > consoles are still not in the mainline.
> > 
> > I would like to have this applied to the v5.20 upstream tree and then
> > revoked once the missing bits have been  merged. Based on what I see,
> > there shouldn't be any road blocks.
> 
> Huh, I do not think that it is a good idea. There are neither atomic
> consoles nor printk kthreads in upstream. The patch effectively
> completely disables the consoles in PREEMPT_RT. All consoles
> will be _empty all the time_.

I am aware of that, I tested that patch (as in I didn't just send it).
This is what I see on bare metal's UART:

|  Booting `Debian GNU/Linux'
|
| Loading Linux RT ...
| Loading initial ramdisk ...
| No EFI environment detected.
| early console in extract_kernel
| input_data: 0x00000000031d12e0
| input_len: 0x0000000000c283e1
| output: 0x0000000001000000
| output_len: 0x00000000025f49f8
| kernel_total_size: 0x0000000002e26000
| needed_size: 0x0000000003000000
| trampoline_32bit: 0x0000000000099000
| 
| Decompressing Linux... Parsing ELF... done.
| Booting the kernel.
| Loading, please wait...
| Starting version 251.3-1
| Begin: Loading essential drivers ... done.
…

and this is okay for me at this point. This means that v5.20 could have
RT enabled for x86 and people can start test it without additional
patches. They can enable it and if it boots they can check dmesg's
output for anything odd. If it doesn't boot they can either wait for the
next release or grab the patchset with the missing bits for debugging.
This is way better than letting printk depend on !RT which would provide
no insight at all. The RT option is still hidden behind EXPERT.

> Also the consoles were never tested with interrupts enabled. I am
> pretty sure that interrupts has to be disabled in some locations,
> for example, when sending some data sequences on the ports. Are we
> sure that they are always explicitly disabled inside the drivers?

The 8250 did disable interrupts via local_irq_disable() and this is not
desired on RT since it *really* disables interrupts. This was long ago
addressed in commit
   ebade5e833eda ("serial: 8250: Clean up the locking for -rt")

The interrupts are enabled in general on RT and they have to. I did not
remove this pattern (local_irq_disable() + try_lock()) from all console
drivers because five years ago we were talking about explicit entry
points for magic-sysrq and oops_in_progress and I didn't want to patch
around until it is done. I don't know what John's plans here are but I
did mention this to him a while ago.
A few other (ARM related drivers) are patched as part of the RT queue.

> This is one reason why we reverted the console kthreads in 5.19-rc4.
> AFAIK, John is doing some inspection of all drivers.

The console drivers (or drivers in general) never *really* disable
interrupts on RT via spin_lock_irq*() and this is intended. 

> That said, I am going to leave the decision on John. I am not sure
> what are the expectations of RT users in mainline.
>
> From my point, this patch does not make much sense. IMHO, it will
> not make mainline usable with PREEMPT_RT. Any serious RT user will
> need to revert it and apply a better printk solution from
> the out-of-tree RT patchset.

The lack of the console probably makes RT not production ready as of
v5.20 but I was debugging systems without a UART so.
It might not be production ready without a console. Also ARM and PowerPC
can not be enabled so there will be a queue for a while unless people
device that they don't care about what is left. However it will
hopefully will attract x86 people to give it a spin. Then we may receive
bug reports and or patches which we did not before and so get better.

> Best Regards,
> Petr

Sebastian

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2] printk: Skip console drivers on PREEMPT_RT.
  2022-07-25 13:55               ` Sebastian Andrzej Siewior
@ 2022-07-25 14:24                 ` Petr Mladek
  2022-07-25 15:16                   ` [PATCH v3] " Sebastian Andrzej Siewior
  0 siblings, 1 reply; 15+ messages in thread
From: Petr Mladek @ 2022-07-25 14:24 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior
  Cc: John Ogness, linux-kernel, Sergey Senozhatsky, Steven Rostedt,
	Thomas Gleixner

On Mon 2022-07-25 15:55:19, Sebastian Andrzej Siewior wrote:
> On 2022-07-25 14:30:04 [+0200], Petr Mladek wrote:
> > This is what I have missed. It might be obvious for people living by RT
> > kernel. But spinlocks do not sleep in normal kernel. So I did not get
> > that the spinlocks are the culprit. Please, make it more obvious
> > in the commit message, for example:
> > 
> > --- cut ---
> > Console drivers use spinlocks that might sleep in PREEMPT_RT. As a
> > result they must not be called with interrupts enabled...
> > --- cut ---
> 
> It is not only the sleeping lock by itself but also the fact that
> polling on 115200 or 9600 to print a line is simply takes too much time.
> Even if the line consists of four letters.

I see.

> > Huh, I do not think that it is a good idea. There are neither atomic
> > consoles nor printk kthreads in upstream. The patch effectively
> > completely disables the consoles in PREEMPT_RT. All consoles
> > will be _empty all the time_.
> 
> I am aware of that, I tested that patch (as in I didn't just send it).
> This is what I see on bare metal's UART:
> 
> |  Booting `Debian GNU/Linux'
> |
> | Loading Linux RT ...
> | Loading initial ramdisk ...
> | No EFI environment detected.
> | early console in extract_kernel
> | input_data: 0x00000000031d12e0
> | input_len: 0x0000000000c283e1
> | output: 0x0000000001000000
> | output_len: 0x00000000025f49f8
> | kernel_total_size: 0x0000000002e26000
> | needed_size: 0x0000000003000000
> | trampoline_32bit: 0x0000000000099000
> | 
> | Decompressing Linux... Parsing ELF... done.
> | Booting the kernel.
> | Loading, please wait...
> | Starting version 251.3-1
> | Begin: Loading essential drivers ... done.

I see. These messages are pushed to the serial console directly.


> and this is okay for me at this point. This means that v5.20 could have
> RT enabled for x86 and people can start test it without additional
> patches. They can enable it and if it boots they can check dmesg's
> output for anything odd. If it doesn't boot they can either wait for the
> next release or grab the patchset with the missing bits for debugging.
> This is way better than letting printk depend on !RT which would provide
> no insight at all. The RT option is still hidden behind EXPERT.

Makes sense.

> > Also the consoles were never tested with interrupts enabled. I am
> > pretty sure that interrupts has to be disabled in some locations,
> > for example, when sending some data sequences on the ports. Are we
> > sure that they are always explicitly disabled inside the drivers?
> 
> The 8250 did disable interrupts via local_irq_disable() and this is not
> desired on RT since it *really* disables interrupts. This was long ago
> addressed in commit
>    ebade5e833eda ("serial: 8250: Clean up the locking for -rt")

Good to know.

> The lack of the console probably makes RT not production ready as of
> v5.20 but I was debugging systems without a UART so.
> It might not be production ready without a console. Also ARM and PowerPC
> can not be enabled so there will be a queue for a while unless people
> device that they don't care about what is left. However it will
> hopefully will attract x86 people to give it a spin. Then we may receive
> bug reports and or patches which we did not before and so get better.

OK, it all makes sense now. Could you please send v3 with an updated
commit message?

It should explain why it is not acceptable to call console drivers
with disabled interrupts in RT (spinlocks are sleeping locks,
pooling takes too long).

It should make clear the effect of the patch. printk() messages
will never reach console. They will be accessible only from userspace
(dmesg).

Finally, it should mention that this is only a temporary solution.
It allows to boot PREEMPT_RT mainline kernel without extra patches.
The consoles will later get enabled again by calling console
drivers with interrupts enabled from dedicated kthreads.
Also it will be possible to call consoles directly by
atomic consoles drivers.

If it gets acked by John then I could queue it for 5.20.

Thanks for explaining all the details.

Best Regards,
Petr

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v3] printk: Skip console drivers on PREEMPT_RT.
  2022-07-25 14:24                 ` Petr Mladek
@ 2022-07-25 15:16                   ` Sebastian Andrzej Siewior
  2022-07-26  7:39                     ` Petr Mladek
  2022-07-26  7:57                     ` John Ogness
  0 siblings, 2 replies; 15+ messages in thread
From: Sebastian Andrzej Siewior @ 2022-07-25 15:16 UTC (permalink / raw)
  To: Petr Mladek
  Cc: John Ogness, linux-kernel, Sergey Senozhatsky, Steven Rostedt,
	Thomas Gleixner

printk might be invoked in a context with disabled interrupts and or
preemption and additionally disables interrupts before it invokes the
console drivers. This behaviour is not desired on PREEMPT_RT:
- The console driver are using spinlock_t based locking which become sleeping
  locks on PREEMPT_RT and must not be acquired with disabled interrupts (or
  preemption).

- The locks within the console drivers must remain sleeping locks and they must
  not disable interrupts. Printing (and polling for its completion) at 115200
  baud on an UART takes too long for PREEMPT_RT in general and so raises the
  latency of the IRQ-off time of the system beyond acceptable levels.

Skip printing to the console as temporary workaround until the printing threads
and atomic consoles have been introduced or another solution which is
compatible with the PREEMPT_RT approach.
With this change, the user will not see any kernel message printed to the
console but can retrieve the printk buffer from userland (via the dmesg
command). This allows enable PREEMPT_RT as a whole without disabling printk and
loosing all kernel output.

Disable console printing on PREEMPT_RT.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
v2…v3:
  - Reword commit message by adding a few details/ explanations.

v1…v2:
   - Use __console_unlock() as suggested by John.

 kernel/printk/printk.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2843,6 +2843,16 @@ void console_unlock(void)
 	}
 
 	/*
+	 * On PREEMPT_RT it is not possible to invoke console drivers with
+	 * disabled interrupts and or preemption. Therefore all drivers are
+	 * skipped and the output can be retrieved from the buffer.
+	 */
+	if (IS_ENABLED(CONFIG_PREEMPT_RT)) {
+		__console_unlock();
+		return;
+	}
+
+	/*
 	 * Console drivers are called with interrupts disabled, so
 	 * @console_may_schedule should be cleared before; however, we may
 	 * end up dumping a lot of lines, for example, if called from

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3] printk: Skip console drivers on PREEMPT_RT.
  2022-07-25 15:16                   ` [PATCH v3] " Sebastian Andrzej Siewior
@ 2022-07-26  7:39                     ` Petr Mladek
  2022-07-26  7:57                     ` John Ogness
  1 sibling, 0 replies; 15+ messages in thread
From: Petr Mladek @ 2022-07-26  7:39 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior
  Cc: John Ogness, linux-kernel, Sergey Senozhatsky, Steven Rostedt,
	Thomas Gleixner

On Mon 2022-07-25 17:16:16, Sebastian Andrzej Siewior wrote:
> printk might be invoked in a context with disabled interrupts and or
> preemption and additionally disables interrupts before it invokes the
> console drivers. This behaviour is not desired on PREEMPT_RT:
> - The console driver are using spinlock_t based locking which become sleeping
>   locks on PREEMPT_RT and must not be acquired with disabled interrupts (or
>   preemption).
> 
> - The locks within the console drivers must remain sleeping locks and they must
>   not disable interrupts. Printing (and polling for its completion) at 115200
>   baud on an UART takes too long for PREEMPT_RT in general and so raises the
>   latency of the IRQ-off time of the system beyond acceptable levels.
> 
> Skip printing to the console as temporary workaround until the printing threads
> and atomic consoles have been introduced or another solution which is
> compatible with the PREEMPT_RT approach.
> With this change, the user will not see any kernel message printed to the
> console but can retrieve the printk buffer from userland (via the dmesg
> command). This allows enable PREEMPT_RT as a whole without disabling printk and
> loosing all kernel output.
> 
> Disable console printing on PREEMPT_RT.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Thanks a lot for updating the commit message. It looks good to me now.

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

Best Regards,
Petr

PS: John, if you ack it then I'll queue it for 5.20.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3] printk: Skip console drivers on PREEMPT_RT.
  2022-07-25 15:16                   ` [PATCH v3] " Sebastian Andrzej Siewior
  2022-07-26  7:39                     ` Petr Mladek
@ 2022-07-26  7:57                     ` John Ogness
  2022-07-26 13:11                       ` Petr Mladek
  1 sibling, 1 reply; 15+ messages in thread
From: John Ogness @ 2022-07-26  7:57 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior, Petr Mladek
  Cc: linux-kernel, Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner

On 2022-07-25, Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
> printk might be invoked in a context with disabled interrupts and or
> preemption and additionally disables interrupts before it invokes the
> console drivers. This behaviour is not desired on PREEMPT_RT:
> - The console driver are using spinlock_t based locking which become sleeping
>   locks on PREEMPT_RT and must not be acquired with disabled interrupts (or
>   preemption).
>
> - The locks within the console drivers must remain sleeping locks and they must
>   not disable interrupts. Printing (and polling for its completion) at 115200
>   baud on an UART takes too long for PREEMPT_RT in general and so raises the
>   latency of the IRQ-off time of the system beyond acceptable levels.
>
> Skip printing to the console as temporary workaround until the printing threads
> and atomic consoles have been introduced or another solution which is
> compatible with the PREEMPT_RT approach.
> With this change, the user will not see any kernel message printed to the
> console but can retrieve the printk buffer from userland (via the dmesg
> command). This allows enable PREEMPT_RT as a whole without disabling printk and
> loosing all kernel output.

Note that "the dmesg command" is not the only userspace tool to access
the kernel logs. Logging daemons (using /proc/kmsg or /dev/kmsg) also
have full access.

> Disable console printing on PREEMPT_RT.
>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Reviewed-by: John Ogness <john.ogness@linutronix.de>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3] printk: Skip console drivers on PREEMPT_RT.
  2022-07-26  7:57                     ` John Ogness
@ 2022-07-26 13:11                       ` Petr Mladek
  0 siblings, 0 replies; 15+ messages in thread
From: Petr Mladek @ 2022-07-26 13:11 UTC (permalink / raw)
  To: John Ogness
  Cc: Sebastian Andrzej Siewior, linux-kernel, Sergey Senozhatsky,
	Steven Rostedt, Thomas Gleixner

On Tue 2022-07-26 10:03:39, John Ogness wrote:
> On 2022-07-25, Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
> > printk might be invoked in a context with disabled interrupts and or
> > preemption and additionally disables interrupts before it invokes the
> > console drivers. This behaviour is not desired on PREEMPT_RT:
> > - The console driver are using spinlock_t based locking which become sleeping
> >   locks on PREEMPT_RT and must not be acquired with disabled interrupts (or
> >   preemption).
> >
> > - The locks within the console drivers must remain sleeping locks and they must
> >   not disable interrupts. Printing (and polling for its completion) at 115200
> >   baud on an UART takes too long for PREEMPT_RT in general and so raises the
> >   latency of the IRQ-off time of the system beyond acceptable levels.
> >
> > Skip printing to the console as temporary workaround until the printing threads
> > and atomic consoles have been introduced or another solution which is
> > compatible with the PREEMPT_RT approach.
> > With this change, the user will not see any kernel message printed to the
> > console but can retrieve the printk buffer from userland (via the dmesg
> > command). This allows enable PREEMPT_RT as a whole without disabling printk and
> > loosing all kernel output.
> 
> Note that "the dmesg command" is not the only userspace tool to access
> the kernel logs. Logging daemons (using /proc/kmsg or /dev/kmsg) also
> have full access.

I have updated the message when comitting, see
https://git.kernel.org/pub/scm/linux/kernel/git/printk/linux.git/commit/?h=rework/kthreads&id=c01c1c784a02aaa216524977b294b8834d0ee907


> > Disable console printing on PREEMPT_RT.
> >
> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> 
> Reviewed-by: John Ogness <john.ogness@linutronix.de>

JFYI, the patch has been committed into printk/linux.git, branch
rework/kthreads. I am going to add it into the pull request for 5.20.

Best Regards,
Petr

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2022-07-26 13:11 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-20 15:48 [PATCH] printk: Skip console drivers on PREEMPT_RT Sebastian Andrzej Siewior
2022-07-20 16:26 ` John Ogness
2022-07-20 16:35   ` Sebastian Andrzej Siewior
2022-07-20 17:50     ` John Ogness
2022-07-21  6:50       ` [PATCH v2] " Sebastian Andrzej Siewior
2022-07-22 12:39         ` Petr Mladek
2022-07-22 17:03           ` Sebastian Andrzej Siewior
2022-07-25 12:30             ` Petr Mladek
2022-07-25 12:51               ` John Ogness
2022-07-25 13:55               ` Sebastian Andrzej Siewior
2022-07-25 14:24                 ` Petr Mladek
2022-07-25 15:16                   ` [PATCH v3] " Sebastian Andrzej Siewior
2022-07-26  7:39                     ` Petr Mladek
2022-07-26  7:57                     ` John Ogness
2022-07-26 13:11                       ` Petr Mladek

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).