[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZAieQtcs7YEuCCDa@alley>
Date: Wed, 8 Mar 2023 15:40:02 +0100
From: Petr Mladek <pmladek@...e.com>
To: John Ogness <john.ogness@...utronix.de>
Cc: Sergey Senozhatsky <senozhatsky@...omium.org>,
Steven Rostedt <rostedt@...dmis.org>,
Thomas Gleixner <tglx@...utronix.de>,
linux-kernel@...r.kernel.org,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [PATCH printk v1 04/18] printk: Add per-console suspended state
On Thu 2023-03-02 21:02:04, John Ogness wrote:
> Currently the global @console_suspended is used to determine if
> consoles are in a suspended state. Its primary purpose is to allow
> usage of the console_lock when suspended without causing console
> printing. It is synchronized by the console_lock.
>
> Rather than relying on the console_lock to determine suspended
> state, make it an official per-console state that is set within
> console->flags. This allows the state to be queried via SRCU.
>
> @console_suspended will continue to exist, but now only to implement
> the console_lock/console_unlock trickery and _not_ to represent
> the suspend state of a particular console.
>
> --- a/include/linux/console.h
> +++ b/include/linux/console.h
> @@ -153,6 +153,8 @@ static inline int con_debug_leave(void)
> * receiving the printk spam for obvious reasons.
> * @CON_EXTENDED: The console supports the extended output format of
> * /dev/kmesg which requires a larger output buffer.
> + * @CON_SUSPENDED: Indicates if a console is suspended. If true, the
> + * printing callbacks must not be called.
> */
> enum cons_flags {
> CON_PRINTBUFFER = BIT(0),
> @@ -162,6 +164,7 @@ enum cons_flags {
> CON_ANYTIME = BIT(4),
> CON_BRL = BIT(5),
> CON_EXTENDED = BIT(6),
> + CON_SUSPENDED = BIT(7),
We have to show it in /proc/consoles, see fs/proc/consoles.c.
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2563,10 +2563,26 @@ MODULE_PARM_DESC(console_no_auto_verbose, "Disable console loglevel raise to hig
> */
> void suspend_console(void)
> {
> + struct console *con;
> +
> if (!console_suspend_enabled)
> return;
> pr_info("Suspending console(s) (use no_console_suspend to debug)\n");
> pr_flush(1000, true);
> +
> + console_list_lock();
> + for_each_console(con)
> + console_srcu_write_flags(con, con->flags | CON_SUSPENDED);
> + console_list_unlock();
> +
> + /*
> + * Ensure that all SRCU list walks have completed. All printing
> + * contexts must be able to see that they are suspended so that it
> + * is guaranteed that all printing has stopped when this function
> + * completes.
> + */
> + synchronize_srcu(&console_srcu);
> +
> console_lock();
> console_suspended = 1;
> up_console_sem();
> @@ -2574,11 +2590,26 @@ void suspend_console(void)
>
> void resume_console(void)
> {
> + struct console *con;
> +
> if (!console_suspend_enabled)
> return;
> down_console_sem();
> console_suspended = 0;
> console_unlock();
> +
> + console_list_lock();
> + for_each_console(con)
> + console_srcu_write_flags(con, con->flags & ~CON_SUSPENDED);
> + console_list_unlock();
> +
> + /*
> + * Ensure that all SRCU list walks have completed. All printing
> + * contexts must be able to see they are no longer suspended so
> + * that they are guaranteed to wake up and resume printing.
> + */
> + synchronize_srcu(&console_srcu);
> +
The setting of the global "console_suspended" and per-console
CON_SUSPENDED flag is not synchronized. As a result, they might
become inconsistent:
CPU0 CPU1
suspend_console()
console_list_lock();
# set CON_SUSPENDED
console_list_unlock();
console_resume()
down_console_sem();
console_suspended = 0;
console_unlock();
console_list_lock()
# clear CON_SUSPENDED
console_list_unlock();
console_lock();
console_suspended = 1;
up_console_sem();
I think that we could just remove the global "console_suspended" flag.
IMHO, it used to be needed to avoid moving the global "console_seq" forward
when the consoles were suspended. But it is not needed now with the
per-console con->seq. console_flush_all() skips consoles when
console_is_usable() fails and it bails out when there is no progress.
It seems that both console_flush_all() and console_unlock() would
handle this correctly.
Hmm, it would change the behavior of console_lock() and console_trylock().
They would set "console_locked" and "console_may_schedule" even when
the consoles are suspended. But it should be OK:
+ "console_may_schedule" actually should be set according
to the context where console_unlock() will be called.
+ "console_locked" seems to be used only in WARN_CONSOLE_UNLOCKED().
I could imagine a corner case where, for example, "vt" code does
not print the warning because it works as it works. But it does
not make much sense. IMHO, such a code should get fixed. And it
is just a warning after all.
> pr_flush(1000, true);
> }
>
> @@ -3712,14 +3745,7 @@ static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progre
> }
> console_srcu_read_unlock(cookie);
>
> - /*
> - * If consoles are suspended, it cannot be expected that they
> - * make forward progress, so timeout immediately. @diff is
> - * still used to return a valid flush status.
> - */
> - if (console_suspended)
> - remaining = 0;
Heh, I though that this might cause regression, e.g. non-necessary
delays during suspend.
But it actually works because "diff" is counted only for usable
consoles. It will stay "0" if there is no usable console.
I wonder if it would make sense to add a comment somewhere,
e.g. above the later check:
+ /* diff is zero also when there is no usable console. */
if (diff == 0 || remaining == 0)
break;
Anyway, we should update the comment above pr_flush():
- * Return: true if all enabled printers are caught up.
+ * Return: true if all usable printers are caught up.
> - else if (diff != last_diff && reset_on_progress)
> + if (diff != last_diff && reset_on_progress)
> remaining = timeout_ms;
>
> console_unlock();
Best Regards,
Petr
Powered by blists - more mailing lists