[1/2] printk: Block console kthreads when direct printing will be required

Commit Message

Petr Mladek June 15, 2022, 4:28 p.m. UTC

There are known situations when the console kthreads are not
reliable or does not work in principle, for example, early boot,
panic, shutdown.

For these situations there is the direct (legacy) mode when printk() tries
to get console_lock() and flush the messages directly. It works very well
during the early boot when the console kthreads are not available at all.
It gets more complicated in the other situations when console kthreads
might be actively printing and block console_trylock() in printk().

The same problem is in the legacy code as well. Any console_lock()
owner could block console_trylock() in printk(). It is solved by
a trick that the current console_lock() owner is responsible for
printing all pending messages. It is actually the reason why there
is the risk of softlockups and why the console kthreads were
introduced.

The console kthreads use the same approach. They are responsible
for printing the messages by definition. So that they handle
the messages anytime when they are awake and see new ones.
The global console_lock is available when there is nothing
to do.

It should work well when the problematic context is correctly
detected and printk() switches to the direct mode. But it seems
that it is not enough in practice. There are reports that
the messages are not printed during panic() or shutdown()
even though printk() tries to use the direct mode here.

The problem seems to be that console kthreads become active in these
situation as well. They steel the job before other CPUs are stopped.
Then they are stopped in the middle of the job and block the global
console_lock.

First part of the solution is to block console kthreads when
the system is in a problematic state and requires the direct
printk() mode.

BugLink: https://lore.kernel.org/r/20220610205038.GA3050413@paulmck-ThinkPad-P17-Gen-1
BugLink: https://lore.kernel.org/r/[e-mail]
Suggested-by: John Ogness <[e-mail]>
Signed-off-by: Petr Mladek <[e-mail]>
---
 kernel/printk/printk.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Linus Torvalds June 15, 2022, 5:47 p.m. UTC | #1

On Wed, Jun 15, 2022 at 9:28 AM Petr Mladek <[e-mail]> wrote:
>
> BugLink: https://lore.kernel.org/r/20220610205038.GA3050413@paulmck-ThinkPad-P17-Gen-1
> BugLink: https://lore.kernel.org/r/[e-mail]

Other thread discussion about this exact thing:

   https://lore.kernel.org/all/CAHk-=[e-mail]/

please stop making up random tags that make no sense.

Just use "Link:"

Look at that first one (I didn't even bother following the second
one). The "bug" part is not even the most important part.

The reason to follow that link is all the discussion, the test-patch,
and the confirmation from Paul that "yup, that patch solves the
problem for me".

It's extra context to the commit, in case somebody wants to know the
history. The "bug" part is (and always should be) already explained in
the commit message, there's absolutely no point in adding soem extra
noise to the "Link:" tag.

And if the only reason for "BugLink:" to exist is to show "look, this
tag actually contains relevant and interesting information", then the
solution to THAT problem is to not have the links that are useless and
pointless in the first place.

Put another way: if you want to distinguish useless links from useful
ones, just do it by not including the useless ones.

Ok?

                   Linus

Petr Mladek June 15, 2022, 7:20 p.m. UTC | #2

On Wed 2022-06-15 10:47:14, Linus Torvalds wrote:
> On Wed, Jun 15, 2022 at 9:28 AM Petr Mladek <[e-mail]> wrote:
> >
> > BugLink: https://lore.kernel.org/r/20220610205038.GA3050413@paulmck-ThinkPad-P17-Gen-1
> > BugLink: https://lore.kernel.org/r/[e-mail]
> 
> Other thread discussion about this exact thing:
> 
>    https://lore.kernel.org/all/CAHk-=[e-mail]/
> 
> please stop making up random tags that make no sense.
> 
> Just use "Link:"
> 
> Look at that first one (I didn't even bother following the second
> one). The "bug" part is not even the most important part.
> 
> The reason to follow that link is all the discussion, the test-patch,
> and the confirmation from Paul that "yup, that patch solves the
> problem for me".
> 
> It's extra context to the commit, in case somebody wants to know the
> history. The "bug" part is (and always should be) already explained in
> the commit message, there's absolutely no point in adding soem extra
> noise to the "Link:" tag.
> 
> And if the only reason for "BugLink:" to exist is to show "look, this
> tag actually contains relevant and interesting information", then the
> solution to THAT problem is to not have the links that are useless and
> pointless in the first place.
> 
> Put another way: if you want to distinguish useless links from useful
> ones, just do it by not including the useless ones.
> 
> Ok?

Got it! I am going to use "Link:" instead.

I just see how the discussion evolved at
https://lore.kernel.org/all/CAHk-=[e-mail]/

It is actually this exact discussion that confused me. I got the
impression that BugLink was a commonly used tag. I see that
I was too fast.

Thanks for stopping me.

Best Regards,
Petr

Patch

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index ea3dd55709e7..45c6c2b0b104 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3729,7 +3729,9 @@  static bool printer_should_wake(struct console *con, u64 seq)
 		return true;
 
 	if (con->blocked ||
-	    console_kthreads_atomically_blocked()) {
+	    console_kthreads_atomically_blocked() ||
+	    system_state > SYSTEM_RUNNING ||
+	    oops_in_progress) {
 		return false;
 	}