Messages in this thread Patch in this message | | | From | John Ogness <> | Subject | Re: [PATCH printk v4 20/27] printk: Avoid console_lock dance if no legacy or boot consoles | Date | Wed, 03 Apr 2024 12:07:32 +0206 |
| |
On 2024-04-03, John Ogness <john.ogness@linutronix.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)
| |