From: Chao Qin <[email protected]>
There is msleep in pr_flush(). If call WARN() in the early boot
stage such as in early_initcall, pr_flush() will run into msleep
when process scheduler is not ready yet. And then the system will
sleep forever.
Before the system_state is SYSTEM_RUNNING, make sure DO NOT sleep
in pr_flush().
Fixes: 63cf1e4b564a ("printk: add pr_flush()")
Signed-off-by: Chao Qin <[email protected]>
Signed-off-by: Lili Li <[email protected]>
---
kernel/printk/printk.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 209d2392f0d8..a9b3afbdac39 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3620,7 +3620,8 @@ bool pr_flush(int timeout_ms, bool reset_on_progress)
u64 diff;
u64 seq;
- may_sleep = (preemptible() && !in_softirq());
+ may_sleep = (preemptible() && !in_softirq()
+ && (system_state >= SYSTEM_RUNNING));
seq = prb_next_seq(prb);
base-commit: 7e175e6b59975c8901ad370f7818937f68de45c1
--
2.25.1
On 2021-07-19, [email protected] wrote:
> From: Chao Qin <[email protected]>
>
> There is msleep in pr_flush(). If call WARN() in the early boot
> stage such as in early_initcall, pr_flush() will run into msleep
> when process scheduler is not ready yet. And then the system will
> sleep forever.
>
> Before the system_state is SYSTEM_RUNNING, make sure DO NOT sleep
> in pr_flush().
>
> Fixes: 63cf1e4b564a ("printk: add pr_flush()")
>
> Signed-off-by: Chao Qin <[email protected]>
> Signed-off-by: Lili Li <[email protected]>
Reviewed-by: John Ogness <[email protected]>
> ---
> kernel/printk/printk.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 209d2392f0d8..a9b3afbdac39 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -3620,7 +3620,8 @@ bool pr_flush(int timeout_ms, bool reset_on_progress)
> u64 diff;
> u64 seq;
>
> - may_sleep = (preemptible() && !in_softirq());
> + may_sleep = (preemptible() && !in_softirq()
> + && (system_state >= SYSTEM_RUNNING));
>
> seq = prb_next_seq(prb);
>
>
> base-commit: 7e175e6b59975c8901ad370f7818937f68de45c1
> --
> 2.25.1
On Mon, 2021-07-19 at 10:26 +0800, [email protected] wrote:
> From: Chao Qin <[email protected]>
>
> There is msleep in pr_flush(). If call WARN() in the early boot
> stage such as in early_initcall, pr_flush() will run into msleep
> when process scheduler is not ready yet. And then the system will
> sleep forever.
>
> Before the system_state is SYSTEM_RUNNING, make sure DO NOT sleep
> in pr_flush().
Makes sense, thanks.
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
[]
> @@ -3620,7 +3620,8 @@ bool pr_flush(int timeout_ms, bool reset_on_progress)
> ? u64 diff;
> ? u64 seq;
>
> - may_sleep = (preemptible() && !in_softirq());
> + may_sleep = (preemptible() && !in_softirq()
> + && (system_state >= SYSTEM_RUNNING));
trivial style note:
Logic continuations are typically at the end of the previous line.
And there are few too many parentheses for my taste.
Maybe exceed 80 columns in a single line
may_sleep = preemptible() && !in_softirq() && system_state >= SYSTEM_RUNNING;
or align the continuation
may_sleep = (preemptible() && !in_softirq() &&
system_state >= SYSTEM_RUNNING);
or use individual lines
may_sleep = (preemptible() &&
!in_softirq() &&
system_state >= SYSTEM_RUNNING);
On 2021-07-20, Joe Perches <[email protected]> wrote:
> Logic continuations are typically at the end of the previous line.
> And there are few too many parentheses for my taste.
>
> Maybe exceed 80 columns in a single line
>
> may_sleep = preemptible() && !in_softirq() && system_state >= SYSTEM_RUNNING;
>
> or align the continuation
>
> may_sleep = (preemptible() && !in_softirq() &&
> system_state >= SYSTEM_RUNNING);
>
> or use individual lines
>
> may_sleep = (preemptible() &&
> !in_softirq() &&
> system_state >= SYSTEM_RUNNING);
The kernel now has a 100-column policy, but I decided to go with this
third variant for easy reading.
Chao Qin, your patch will be part of the next PREEMPT_RT release. Thank
you.
John Ogness
Noted. Thanks for your review.
-Chao.
-----Original Message-----
From: John Ogness <[email protected]>
Sent: Tuesday, July 20, 2021 10:03 PM
To: Joe Perches <[email protected]>; Qin, Chao <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Cc: [email protected]; Mei, Paul <[email protected]>; Li, Lili <[email protected]>
Subject: Re: [PREEMPT_RT][PATCH] printk: Enhance the condition check of msleep in pr_flush()
On 2021-07-20, Joe Perches <[email protected]> wrote:
> Logic continuations are typically at the end of the previous line.
> And there are few too many parentheses for my taste.
>
> Maybe exceed 80 columns in a single line
>
> may_sleep = preemptible() && !in_softirq() && system_state >=
> SYSTEM_RUNNING;
>
> or align the continuation
>
> may_sleep = (preemptible() && !in_softirq() &&
> system_state >= SYSTEM_RUNNING);
>
> or use individual lines
>
> may_sleep = (preemptible() &&
> !in_softirq() &&
> system_state >= SYSTEM_RUNNING);
The kernel now has a 100-column policy, but I decided to go with this third variant for easy reading.
Chao Qin, your patch will be part of the next PREEMPT_RT release. Thank you.
John Ogness
On 2021-07-19 17:01:51 [+0206], John Ogness wrote:
> On 2021-07-19, [email protected] wrote:
> > --- a/kernel/printk/printk.c
> > +++ b/kernel/printk/printk.c
> > @@ -3620,7 +3620,8 @@ bool pr_flush(int timeout_ms, bool reset_on_progress)
> > u64 diff;
> > u64 seq;
> >
> > - may_sleep = (preemptible() && !in_softirq());
> > + may_sleep = (preemptible() && !in_softirq()
> > + && (system_state >= SYSTEM_RUNNING));
I don't have more context but scheduling should work starting with
SYSTEM_SCHEDULING.
> >
> > seq = prb_next_seq(prb);
> >
> >
Sebastian
On 2021-07-30, Sebastian Andrzej Siewior <[email protected]> wrote:
> On 2021-07-19 17:01:51 [+0206], John Ogness wrote:
>> On 2021-07-19, [email protected] wrote:
>> > --- a/kernel/printk/printk.c
>> > +++ b/kernel/printk/printk.c
>> > @@ -3620,7 +3620,8 @@ bool pr_flush(int timeout_ms, bool reset_on_progress)
>> > u64 diff;
>> > u64 seq;
>> >
>> > - may_sleep = (preemptible() && !in_softirq());
>> > + may_sleep = (preemptible() && !in_softirq()
>> > + && (system_state >= SYSTEM_RUNNING));
>
> I don't have more context but scheduling should work starting with
> SYSTEM_SCHEDULING.
I also thought this, but a quick test shows that is not the case. For
example, init/main.c:kernel_init() is called in preemptible context, but
msleep() will hang if called at the beginning of that function.
John Ogness
As John Ogness commented, I also found it would hang if sleep when >= SYSTEM_SCHEDULING.
And as in commit https://lore.kernel.org/lkml/[email protected]/, it should enable sleep right when the scheduler starts working(>= SYSTEM_RUNNING).
-Chao.
-----Original Message-----
From: John Ogness <[email protected]>
Sent: Friday, July 30, 2021 10:47 PM
To: Sebastian Andrzej Siewior <[email protected]>
Cc: Qin, Chao <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Mei, Paul <[email protected]>; Li, Lili <[email protected]>
Subject: Re: [PREEMPT_RT][PATCH] printk: Enhance the condition check of msleep in pr_flush()
On 2021-07-30, Sebastian Andrzej Siewior <[email protected]> wrote:
> On 2021-07-19 17:01:51 [+0206], John Ogness wrote:
>> On 2021-07-19, [email protected] wrote:
>> > --- a/kernel/printk/printk.c
>> > +++ b/kernel/printk/printk.c
>> > @@ -3620,7 +3620,8 @@ bool pr_flush(int timeout_ms, bool reset_on_progress)
>> > u64 diff;
>> > u64 seq;
>> >
>> > - may_sleep = (preemptible() && !in_softirq());
>> > + may_sleep = (preemptible() && !in_softirq()
>> > + && (system_state >= SYSTEM_RUNNING));
>
> I don't have more context but scheduling should work starting with
> SYSTEM_SCHEDULING.
I also thought this, but a quick test shows that is not the case. For example, init/main.c:kernel_init() is called in preemptible context, but
msleep() will hang if called at the beginning of that function.
John Ogness
On 2021-07-30 16:52:42 [+0206], John Ogness wrote:
> On 2021-07-30, Sebastian Andrzej Siewior <[email protected]> wrote:
> > I don't have more context but scheduling should work starting with
> > SYSTEM_SCHEDULING.
>
> I also thought this, but a quick test shows that is not the case. For
> example, init/main.c:kernel_init() is called in preemptible context, but
> msleep() will hang if called at the beginning of that function.
hmm. So the timer device is missing. Everything else is fine.
> John Ogness
Sebastian
Hi John Ogness,
Do you have plan to backport this fix into v5.10.y-rt kernel?
Thanks,
Qin Chao.
-----Original Message-----
From: John Ogness <[email protected]>
Sent: Tuesday, July 20, 2021 10:03 PM
To: Joe Perches <[email protected]>; Qin, Chao <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Cc: [email protected]; Mei, Paul <[email protected]>; Li, Lili <[email protected]>
Subject: Re: [PREEMPT_RT][PATCH] printk: Enhance the condition check of msleep in pr_flush()
On 2021-07-20, Joe Perches <[email protected]> wrote:
> Logic continuations are typically at the end of the previous line.
> And there are few too many parentheses for my taste.
>
> Maybe exceed 80 columns in a single line
>
> may_sleep = preemptible() && !in_softirq() && system_state >=
> SYSTEM_RUNNING;
>
> or align the continuation
>
> may_sleep = (preemptible() && !in_softirq() &&
> system_state >= SYSTEM_RUNNING);
>
> or use individual lines
>
> may_sleep = (preemptible() &&
> !in_softirq() &&
> system_state >= SYSTEM_RUNNING);
The kernel now has a 100-column policy, but I decided to go with this third variant for easy reading.
Chao Qin, your patch will be part of the next PREEMPT_RT release. Thank you.
John Ogness
Hello Steven,
Could you please cherry-pick 83e9288d9c42("printk: Enhance the condition
check of msleep in pr_flush()") from linux-rt-devel.git (branch
linux-5.14.y-rt-rebase) for the next stable v5.10-rt release?
The commit is provided below as well.
Thanks.
John Ogness
On 2021-08-05, "Qin, Chao" <[email protected]> wrote:
> Do you have plan to backport this fix into v5.10.y-rt kernel?
From 83e9288d9c4295d1195e9d780fcbc42c72ba4a83 Mon Sep 17 00:00:00 2001
From: Chao Qin <[email protected]>
Date: Mon, 19 Jul 2021 10:26:50 +0800
Subject: [PATCH] printk: Enhance the condition check of msleep in pr_flush()
There is msleep in pr_flush(). If call WARN() in the early boot
stage such as in early_initcall, pr_flush() will run into msleep
when process scheduler is not ready yet. And then the system will
sleep forever.
Before the system_state is SYSTEM_RUNNING, make sure DO NOT sleep
in pr_flush().
Fixes: c0b395bd0fe3("printk: add pr_flush()")
Signed-off-by: Chao Qin <[email protected]>
Signed-off-by: Lili Li <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Reviewed-by: John Ogness <[email protected]>
Signed-off-by: John Ogness <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Link: https://lore.kernel.org/lkml/[email protected]
---
kernel/printk/printk.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index e4085e2cafb5..500ae4b18864 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3661,7 +3661,9 @@ bool pr_flush(int timeout_ms, bool reset_on_progress)
u64 diff;
u64 seq;
- may_sleep = (preemptible() && !in_softirq());
+ may_sleep = (preemptible() &&
+ !in_softirq() &&
+ system_state >= SYSTEM_RUNNING);
seq = prb_next_seq(prb);
--
2.20.1
On Thu, 05 Aug 2021 14:27:25 +0206
John Ogness <[email protected]> wrote:
> Hello Steven,
>
> Could you please cherry-pick 83e9288d9c42("printk: Enhance the condition
> check of msleep in pr_flush()") from linux-rt-devel.git (branch
> linux-5.14.y-rt-rebase) for the next stable v5.10-rt release?
>
> The commit is provided below as well.
>
I'll add it to my queue for tomorrow.
Thanks!
-- Steve
On Thu, 5 Aug 2021 12:12:00 -0400
Steven Rostedt <[email protected]> wrote:
> I'll add it to my queue for tomorrow.
I'm currently testing 5.10.56-rt48 which only brings the 5.10 rt branch
up to latest stable. After that is done and pushed out, I'll backport
this patch and do a 5.10.56-rt49-rc release.
Letting you know in case you see the 5.10.56-rt48 and wonder where your
patch is.
-- Steve