On Thu, Feb 06, 2025 at 12:06:08PM +0100, Petr Mladek wrote:

On Wed 2025-02-05 15:50:14, Paul E. McKenney wrote:

quoted

On Wed, Feb 05, 2025 at 11:37:25PM +0106, John Ogness wrote:

quoted

On 2025-02-05, John Ogness [off-list ref] wrote:

quoted

quoted

OK, so I don't need to add "if (IS_ENABLED(CONFIG_PREEMPT_RT))" to guard
it, then?

For !CONFIG_PREEMPT_RT, if there are legacy consoles registered,
pr_flush() will additionally perform a blocking lock on the
console_lock.

Sorry, I was just thinking about the flushing component of
pr_flush(). Later in the function it takes the console_lock even for
CONFIG_PREEMPT_RT. So please do _not_ have a guard.

Thank you all for looking this over!

It should be possible to avoid this console_lock() when only NBCON
consoles are registered. We take it in pr_flush() for reading con->seq
and potential race with register_console(). But it can be handled
another way, for example, by restarting the cycle when non-NBCON
console suddenly appears.

But it might be done later. We do not have NBCON consoles at the
moment so the console_lock() is needed anyway.

Fair enough.

quoted

quoted

pr_flush() should never hang on the console_lock during shutdown, but if
does, that is something that will need to be debugged and fixed.

@pmladek: Looking forward to reading your input on this.

My only concern is that kernel_power_off() is called in many
code paths so it is not easy to review all scenarios.

But I haven't found any code path where it would be an obvious problem.
Especially, it seems that it is _not_ called in panic situations.

Also kernel_power_off() calls syscore_shutdown() which takes
syscore_ops_lock mutex. So that is must be called only in
a schedulable context even before this patch.

Summary: I do not see any obvious problem in the end.
	 Except, for some small details, see below.

The presence of that syscore_shutdown() call greatly reduced my level
of anxiety as well.  ;-)

quoted

Here is an updated commit, hopefully applying your feedback correctly.

							Thanx, Paul

------------------------------------------------------------------------

commit 35679c18b062368855e183ee6712ca5c16145d8c
Author: Paul E. McKenney [off-list ref]
Date:   Wed Feb 5 12:27:23 2025 -0800

    printk: Flush console log from kernel_power_off()
    
    Kernels built with CONFIG_PREEMPT_RT=y can lose significant console output
    and shutdown time, which hides shutdown-time RCU issues from rcutorture.
    Therefore, make pr_flush() public and invoke it after then last print
    in kernel_power_off().
    

--- a/include/linux/printk.h
+++ b/include/linux/printk.h

@@ -207,6 +207,7 @@ void printk_legacy_allow_panic_sync(void);
 extern bool nbcon_device_try_acquire(struct console *con);
 extern void nbcon_device_release(struct console *con);
 void nbcon_atomic_flush_unsafe(void);
+bool pr_flush(int timeout_ms, bool reset_on_progress);
 #else
 static inline __printf(1, 0)
 int vprintk(const char *s, va_list args)

@@ -315,6 +316,11 @@ static inline void nbcon_atomic_flush_unsafe(void)
 {
 }
 
+static bool pr_flush(int timeout_ms, bool reset_on_progress)

There is missing "inline":

static inline bool pr_flush(int timeout_ms, bool reset_on_progress)


Otherwise, I get

In file included from ./include/asm-generic/bug.h:22,
                 from ./arch/x86/include/asm/bug.h:99,
                 from ./include/linux/bug.h:5,
                 from ./include/linux/page-flags.h:10,
                 from kernel/bounds.c:10:
./include/linux/printk.h:319:13: warning: ‘pr_flush’ defined but not used [-Wunused-function]

Fixed, thank you both!

quoted

+{
+	return true;
+}
+
 #endif
 
 bool this_cpu_in_panic(void);

--- a/kernel/reboot.c
+++ b/kernel/reboot.c

@@ -704,6 +704,7 @@ void kernel_power_off(void)
 	migrate_to_reboot_cpu();
 	syscore_shutdown();
 	pr_emerg("Power down\n");
+	pr_flush(1000, 1);

This should be pr_flush(1000, true) as suggested by Sebastian.

Also fixed, also thank you both!

quoted

 	kmsg_dump(KMSG_DUMP_SHUTDOWN);
 	machine_power_off();
 }

With the two changes:

Reviewed-by: Petr Mladek <pmladek@suse.com>

Paul, I assume that you will pass this via the RCU tree together with
the other changes. Or would you prefer to route this particular
change via printk tree? Both ways are fine to me.

One advantage of carrying it via RCU is that I can arrange things so that
rcutorture users don't see this problem.  So unless a problem arises,
thank you, and I do plan to send this up via RCU.

							Thanx, Paul