From: xu xin <[email protected]>
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 <[email protected]>
Cc: Zhang Yunkai <[email protected]>
---
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
On 2024-05-23, <[email protected]> wrote:
> From: xu xin <[email protected]>
>
> 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 <[email protected]>
> Cc: Zhang Yunkai <[email protected]>
> ---
> 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 <[email protected]>
On 2024-05-23 23:55:37 [+0800], [email protected] 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
> > 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.
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
On 2024-05-28, Sebastian Andrzej Siewior <[email protected]> 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
On 2024-06-03 22:17:35 [+0206], John Ogness wrote:
> On 2024-05-28, Sebastian Andrzej Siewior <[email protected]> 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