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

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

[chao.qin]

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

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

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

[John Ogness]

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

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

[Joe Perches]

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

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

[John Ogness]

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

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

[Qin, Chao]

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

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

[Sebastian Andrzej Siewior]

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

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

[John Ogness]

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

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

[Qin, Chao]

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

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

[Sebastian Andrzej Siewior]

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

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

[Qin, Chao]

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

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

[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" <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

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

[Steven Rostedt]

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

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

[Steven Rostedt]

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