diff mbox series

[PREEMPT_RT] printk: Enhance the condition check of msleep in pr_flush()

Message ID 20210719022649.3444072-1-chao.qin@intel.com
State New
Headers show
Series [PREEMPT_RT] printk: Enhance the condition check of msleep in pr_flush() | expand

Commit Message

Qin, Chao July 19, 2021, 2:26 a.m. UTC 
From: Chao Qin <chao.qin@intel.com>

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 <chao.qin@intel.com>
Signed-off-by: Lili Li <lili.li@intel.com>
---
 kernel/printk/printk.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)


base-commit: 7e175e6b59975c8901ad370f7818937f68de45c1

Comments

John Ogness July 19, 2021, 2:55 p.m. UTC | #1 
On 2021-07-19, chao.qin@intel.com wrote:
> From: Chao Qin <chao.qin@intel.com>
>
> 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 <chao.qin@intel.com>
> Signed-off-by: Lili Li <lili.li@intel.com>

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

> ---
>  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
Joe Perches July 20, 2021, 11:55 a.m. UTC | #2 
On Mon, 2021-07-19 at 10:26 +0800, chao.qin@intel.com wrote:
> From: Chao Qin <chao.qin@intel.com>

> 

> 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);
John Ogness July 20, 2021, 2:03 p.m. UTC | #3 
On 2021-07-20, Joe Perches <joe@perches.com> 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
Qin, Chao July 21, 2021, 1:42 a.m. UTC | #4 
Noted. Thanks for your review.

-Chao.

-----Original Message-----
From: John Ogness <john.ogness@linutronix.de> 

Sent: Tuesday, July 20, 2021 10:03 PM
To: Joe Perches <joe@perches.com>; Qin, Chao <chao.qin@intel.com>; linux-kernel@vger.kernel.org; linux-rt-users@vger.kernel.org; bigeasy@linutronix.de; tglx@linutronix.de; rostedt@goodmis.org
Cc: mgross@linux.intel.com; Mei, Paul <paul.mei@intel.com>; Li, Lili <lili.li@intel.com>
Subject: Re: [PREEMPT_RT][PATCH] printk: Enhance the condition check of msleep in pr_flush()

On 2021-07-20, Joe Perches <joe@perches.com> 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
Sebastian Andrzej Siewior July 30, 2021, 2:01 p.m. UTC | #5 
On 2021-07-19 17:01:51 [+0206], John Ogness wrote:
> On 2021-07-19, chao.qin@intel.com 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
John Ogness July 30, 2021, 2:46 p.m. UTC | #6 
On 2021-07-30, Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
> On 2021-07-19 17:01:51 [+0206], John Ogness wrote:

>> On 2021-07-19, chao.qin@intel.com 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
Qin, Chao Aug. 2, 2021, 6:08 a.m. UTC | #7 
As John Ogness commented, I also found it would hang if sleep when >= SYSTEM_SCHEDULING.

 And as in commit https://lore.kernel.org/lkml/20170516184736.272225698@linutronix.de/,  it should enable sleep right when the scheduler starts working(>= SYSTEM_RUNNING).

-Chao.

-----Original Message-----
From: John Ogness <john.ogness@linutronix.de> 

Sent: Friday, July 30, 2021 10:47 PM
To: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Qin, Chao <chao.qin@intel.com>; linux-kernel@vger.kernel.org; linux-rt-users@vger.kernel.org; tglx@linutronix.de; rostedt@goodmis.org; mgross@linux.intel.com; Mei, Paul <paul.mei@intel.com>; Li, Lili <lili.li@intel.com>
Subject: Re: [PREEMPT_RT][PATCH] printk: Enhance the condition check of msleep in pr_flush()

On 2021-07-30, Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
> On 2021-07-19 17:01:51 [+0206], John Ogness wrote:

>> On 2021-07-19, chao.qin@intel.com 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
Sebastian Andrzej Siewior Aug. 2, 2021, 6:48 a.m. UTC | #8 
On 2021-07-30 16:52:42 [+0206], John Ogness wrote:
> On 2021-07-30, Sebastian Andrzej Siewior <bigeasy@linutronix.de> 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
Qin, Chao Aug. 5, 2021, 3:01 a.m. UTC | #9 
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 <john.ogness@linutronix.de> 

Sent: Tuesday, July 20, 2021 10:03 PM
To: Joe Perches <joe@perches.com>; Qin, Chao <chao.qin@intel.com>; linux-kernel@vger.kernel.org; linux-rt-users@vger.kernel.org; bigeasy@linutronix.de; tglx@linutronix.de; rostedt@goodmis.org
Cc: mgross@linux.intel.com; Mei, Paul <paul.mei@intel.com>; Li, Lili <lili.li@intel.com>
Subject: Re: [PREEMPT_RT][PATCH] printk: Enhance the condition check of msleep in pr_flush()

On 2021-07-20, Joe Perches <joe@perches.com> 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
John Ogness Aug. 5, 2021, 12:21 p.m. UTC | #10 
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" <chao.qin@intel.com> 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 <chao.qin@intel.com>

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 <chao.qin@intel.com>

Signed-off-by: Lili Li <lili.li@intel.com>

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

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

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

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

Link: https://lore.kernel.org/lkml/20210719022649.3444072-1-chao.qin@intel.com
---
 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
Steven Rostedt Aug. 5, 2021, 4:12 p.m. UTC | #11 
On Thu, 05 Aug 2021 14:27:25 +0206
John Ogness <john.ogness@linutronix.de> 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
Steven Rostedt Aug. 6, 2021, 6:34 p.m. UTC | #12 
On Thu, 5 Aug 2021 12:12:00 -0400
Steven Rostedt <rostedt@goodmis.org> 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
diff mbox series

Patch

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