[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <87r0fmydc3.fsf@jogness.linutronix.de>
Date: Wed, 03 Apr 2024 12:07:32 +0206
From: John Ogness <john.ogness@...utronix.de>
To: Petr Mladek <pmladek@...e.com>
Cc: Sergey Senozhatsky <senozhatsky@...omium.org>, Steven Rostedt
<rostedt@...dmis.org>, Thomas Gleixner <tglx@...utronix.de>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH printk v4 20/27] printk: Avoid console_lock dance if no
legacy or boot consoles
On 2024-04-03, John Ogness <john.ogness@...utronix.de> wrote:
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index df84c6bfbb2d..4ff3800e8e8e 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -3810,6 +3833,7 @@ static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progre
> u64 last_diff = 0;
> u64 printk_seq;
> short flags;
> + bool locked;
> int cookie;
> u64 diff;
> u64 seq;
> @@ -3819,22 +3843,35 @@ static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progre
> seq = prb_next_reserve_seq(prb);
>
> /* Flush the consoles so that records up to @seq are printed. */
> - console_lock();
> - console_unlock();
> + if (printing_via_unlock) {
> + console_lock();
> + console_unlock();
> + }
>
> for (;;) {
> unsigned long begin_jiffies;
> unsigned long slept_jiffies;
>
> + locked = false;
> diff = 0;
>
> + if (printing_via_unlock) {
> + /*
> + * Hold the console_lock to guarantee safe access to
> + * console->seq. Releasing console_lock flushes more
> + * records in case @seq is still not printed on all
> + * usable consoles.
> + */
> + console_lock();
> + locked = true;
> + }
> +
> /*
> - * Hold the console_lock to guarantee safe access to
> - * console->seq. Releasing console_lock flushes more
> - * records in case @seq is still not printed on all
> - * usable consoles.
> + * Ensure the compiler does not optimize @locked to be
> + * @printing_via_unlock since the latter can change at any
> + * time.
> */
> - console_lock();
> + barrier();
When I originally implemented this, my objective was to force the
compiler to use a local variable. But to be fully paranoid (which we
should) we must also forbid the compiler from being able to do this
nonsense:
if (printing_via_unlock) {
console_lock();
locked = printing_via_unlock;
}
I suggest replacing the __pr_flush() hunks to be as follows instead
(i.e. ensure all conditional console lock usage within the loop is using
the local variable):
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index df84c6bfbb2d..1dbd2a837b67 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3819,22 +3842,34 @@ static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progre
seq = prb_next_reserve_seq(prb);
/* Flush the consoles so that records up to @seq are printed. */
- console_lock();
- console_unlock();
+ if (printing_via_unlock) {
+ console_lock();
+ console_unlock();
+ }
for (;;) {
unsigned long begin_jiffies;
unsigned long slept_jiffies;
-
- diff = 0;
+ bool use_console_lock = printing_via_unlock;
/*
- * Hold the console_lock to guarantee safe access to
- * console->seq. Releasing console_lock flushes more
- * records in case @seq is still not printed on all
- * usable consoles.
+ * Ensure the compiler does not optimize @use_console_lock to
+ * be @printing_via_unlock since the latter can change at any
+ * time.
*/
- console_lock();
+ barrier();
+
+ diff = 0;
+
+ if (use_console_lock) {
+ /*
+ * Hold the console_lock to guarantee safe access to
+ * console->seq. Releasing console_lock flushes more
+ * records in case @seq is still not printed on all
+ * usable consoles.
+ */
+ console_lock();
+ }
cookie = console_srcu_read_lock();
for_each_console_srcu(c) {
@@ -3854,6 +3889,7 @@ static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progre
if (flags & CON_NBCON) {
printk_seq = nbcon_seq_read(c);
} else {
+ WARN_ON_ONCE(!use_console_lock);
printk_seq = c->seq;
}
@@ -3865,7 +3901,8 @@ static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progre
if (diff != last_diff && reset_on_progress)
remaining_jiffies = timeout_jiffies;
- console_unlock();
+ if (use_console_lock)
+ console_unlock();
/* Note: @diff is 0 if there are no usable consoles. */
if (diff == 0 || remaining_jiffies == 0)
Powered by blists - more mailing lists