[PATCH 5.10-rt] printk: ignore that console preempted by irq/softirq

[PATCH 5.10-rt] printk: ignore that console preempted by irq/softirq

[xu.xin16]

From: xu xin <xu.xin16@zte.com.cn>

When we're in the unpreemptible context on the same cpu with which the
thread of console locates on, we should ignore this console for
pr_flush, because it's a vain and always lead to timeout until the console
thread get cpu resource.

Fixes: e65be5f4dc3ed("printk: Update John Ogness' printk series")
Signed-off-by: xu xin <xu.xin16@zte.com.cn>
Cc: Zhang Yunkai <zhang.yunkai@zte.com.cn>
---
kernel/printk/printk.c | 8 ++++++++
1 file changed, 8 insertions(+)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 7f27cfee283e..faab85dd4439 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3735,6 +3735,14 @@ bool pr_flush(int timeout_ms, bool reset_on_progress)
diff = 0;

for_each_console(con) {
+ /*
+ * When we're in the unpreemptible context on the same cpu
+ * with which the thread of console locates on, we should
+ * ignore this console, because it's a vain.
+ */
+ if (!preemptible() && con->thread &&
+ task_cpu(con->thread) == smp_processor_id())
+ continue;
if (!(con->flags & CON_ENABLED))
continue;
printk_seq = read_console_seq(con);
--
2.15.2

Re: [PATCH 5.10-rt] printk: ignore that console preempted by irq/softirq

[John Ogness]

On 2024-05-23, <xu.xin16@zte.com.cn> wrote:
> From: xu xin <xu.xin16@zte.com.cn>
>
> When we're in the unpreemptible context on the same cpu with which the
> thread of console locates on, we should ignore this console for
> pr_flush, because it's a vain and always lead to timeout until the console
> thread get cpu resource.

Newer RT kernels do things quite differently. But for the 5.10-rt
implementation, I can see how the 1 second timeout could be annoying.

> Fixes: e65be5f4dc3ed("printk: Update John Ogness' printk series")
> Signed-off-by: xu xin <xu.xin16@zte.com.cn>
> Cc: Zhang Yunkai <zhang.yunkai@zte.com.cn>
> ---
> kernel/printk/printk.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 7f27cfee283e..faab85dd4439 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -3735,6 +3735,14 @@ bool pr_flush(int timeout_ms, bool reset_on_progress)
> diff = 0;
>
> for_each_console(con) {
> + /*
> + * When we're in the unpreemptible context on the same cpu
> + * with which the thread of console locates on, we should
> + * ignore this console, because it's a vain.
> + */
> + if (!preemptible() && con->thread &&
> + task_cpu(con->thread) == smp_processor_id())
> + continue;
> if (!(con->flags & CON_ENABLED))
> continue;
> printk_seq = read_console_seq(con);

The code is OK, but you have lost the tab indenting. Perhaps these can
be added by the RT maintainer.

With the correct whitespace:

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

Re: [PATCH 5.10-rt] printk: ignore that console preempted by irq/softirq

[Sebastian Andrzej Siewior]

On 2024-05-23 23:55:37 [+0800], xu.xin16@zte.com.cn wrote:
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 7f27cfee283e..faab85dd4439 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -3735,6 +3735,14 @@ bool pr_flush(int timeout_ms, bool reset_on_progress)
> diff = 0;
> 
> for_each_console(con) {
> + /*
> + * When we're in the unpreemptible context on the same cpu
> + * with which the thread of console locates on, we should
> + * ignore this console, because it's a vain.
> + */
> + if (!preemptible() && con->thread &&
> + task_cpu(con->thread) == smp_processor_id())
> + continue;
> if (!(con->flags & CON_ENABLED))
> continue;
> printk_seq = read_console_seq(con);

This does not apply.
There is `may_sleep' set earlier.

There is no console_lock() around for each…

The other question is which kernel started enforcing might_sleep() for
pr_flush(). This should be applied to all kernel or none so we don't
have random behaviour across kernels (5.4 yes, 5.10 no, 5.15 yes).

This is a delay of max 1 sec during bug() and panic(). Not sure how
"critical" this is…

Sebastian

Re: [PATCH 5.10-rt] printk: ignore that console preempted by irq/softirq

[xu xin]

> > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> > index 7f27cfee283e..faab85dd4439 100644
> > --- a/kernel/printk/printk.c
> > +++ b/kernel/printk/printk.c
> > @@ -3735,6 +3735,14 @@ bool pr_flush(int timeout_ms, bool reset_on_progress)
> > diff = 0;
> > 
> > for_each_console(con) {
> > + /*
> > + * When we're in the unpreemptible context on the same cpu
> > + * with which the thread of console locates on, we should
> > + * ignore this console, because it's a vain.
> > + */
> > + if (!preemptible() && con->thread &&
> > + task_cpu(con->thread) == smp_processor_id())
> > + continue;
> > if (!(con->flags & CON_ENABLED))
> > continue;
> > printk_seq = read_console_seq(con);
> 
> This does not apply.
> There is `may_sleep' set earlier.
> 
> There is no console_lock() around for each…
> 

Sorry, I don't get it.

To clarify it again, this patch aims to solve the useless waiting of pr_flush
when the console is preempted by the current irq/softirq. This has nothing to
do with might_sleep().

> The other question is which kernel started enforcing might_sleep() for
> pr_flush(). This should be applied to all kernel or none so we don't
> have random behaviour across kernels (5.4 yes, 5.10 no, 5.15 yes).
>

Sorry, my understanding is that pr_flush didn't start enforcing might_sleep().
This patch can apply to 5.10 and 5.15 where the problem exist.

> This is a delay of max 1 sec during bug() and panic(). Not sure how
> "critical" this is…

In some industrial control scenarios, bugs and warnings containning a
pr_flush delay of 1 sec is very critical to the upper services.

Especiall for watchdog timeout(< 2s), just WARN can easily lead to system reset,
which is unacceptible.

Re: [PATCH 5.10-rt] printk: ignore that console preempted by irq/softirq

[Sebastian Andrzej Siewior]

On 2024-05-28 06:40:03 [+0000], xu xin wrote:
> > This does not apply.
> > There is `may_sleep' set earlier.
> > 
> > There is no console_lock() around for each…
> > 
> 
> Sorry, I don't get it.
> 
> To clarify it again, this patch aims to solve the useless waiting of pr_flush
> when the console is preempted by the current irq/softirq. This has nothing to
> do with might_sleep().

There is a `may_sleep` variable set earlier. Couldn't that be re-used?

> > The other question is which kernel started enforcing might_sleep() for
> > pr_flush(). This should be applied to all kernel or none so we don't
> > have random behaviour across kernels (5.4 yes, 5.10 no, 5.15 yes).
> >
> 
> Sorry, my understanding is that pr_flush didn't start enforcing might_sleep().
> This patch can apply to 5.10 and 5.15 where the problem exist.

Starting with v6.1-RT there is a might_sleep() at the beginning of
pr_flush(). This means that atomic context can not be used anymore.
Therefore is patch needs only to be applied to 5.10 and 5.15 as you
said. I just didn't see the information the following kernels (>=6.1)
already had that might_sleep() check and the previous (<5.4) lack
pr_flush().

> > This is a delay of max 1 sec during bug() and panic(). Not sure how
> > "critical" this is…
> 
> In some industrial control scenarios, bugs and warnings containning a
> pr_flush delay of 1 sec is very critical to the upper services.
> 
> Especiall for watchdog timeout(< 2s), just WARN can easily lead to system reset,
> which is unacceptible.

Now this would be important piece of information for the changelog in
terms of _why_ we do this.
You don't have an atomic console, do you? Because if this BUG/ WARNING is
printed via the atomic console then it could also raise your 1sec limit.

Sebastian

Re: [PATCH 5.10-rt] printk: ignore that console preempted by irq/softirq

[John Ogness]

On 2024-05-28, Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
> There is a `may_sleep` variable set earlier. Couldn't that be re-used?

Since the printing thread cannot preempt the softirq context and since
the printing threads are not started before @system_state is
SYSTEM_RUNNING, using @may_sleep is OK.

if (!may_sleep &&
    con->thread &&
    task_cpu(con->thread) == smp_processor_id()) {
	continue;
}

John Ogness

Re: [PATCH 5.10-rt] printk: ignore that console preempted by irq/softirq

[Sebastian Andrzej Siewior]

On 2024-06-03 22:17:35 [+0206], John Ogness wrote:
> On 2024-05-28, Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
> > There is a `may_sleep` variable set earlier. Couldn't that be re-used?
> 
> Since the printing thread cannot preempt the softirq context and since
> the printing threads are not started before @system_state is
> SYSTEM_RUNNING, using @may_sleep is OK.

will there be a repost?

> if (!may_sleep &&
>     con->thread &&
>     task_cpu(con->thread) == smp_processor_id()) {
> 	continue;
> }
> 
> John Ogness

Sebastian