Re: printk: selective deactivation of feature ignoring non panic cpu's messages

[Petr Mladek <pmladek@suse.com>]

On Fri 2025-02-28 15:26:34, John Ogness wrote:
> On 2025-02-26, Petr Mladek <pmladek@suse.com> wrote:
> > I wonder if this is actually safe. I recall that we simplified the
> > design somewhere because we expected that non-panic CPUs will not
> > add messages.
> 
> Perhaps you are recalling commit 7412dc6d55ee ("dump_stack: Do not get
> cpu_sync for panic CPU").

Yeah.

> > I am not sure that I found all locations. But
> > we might want to revise:
> >
> >
> > 1st problem: _prb_read_valid() skips non-finalized records on non-panic CPUs.
> >
> >    opinion: We should not do it in this case.
> 
> I don't know. This could result in seeing even less output if some CPU
> didn't finalize a record.

But it might also drop messages which are just being added.
And the person enabling the new option is explicitly interested
into the messages from non-panic CPUs.

To make it clear. I do not want to revert the change. But I would
avoid the skip when the new option is used.

The messages might be skipped later when CPUs are stopped.
The information is already available via the @legacy_allow_panic_sync
variable.

I mean something like:

--- a/kernel/printk/printk_ringbuffer.c
+++ b/kernel/printk/printk_ringbuffer.c
@@ -2143,7 +2143,9 @@ static bool _prb_read_valid(struct printk_ringbuffer *rb, u64 *seq,
 			 * But it would have the sequence number returned
 			 * by "prb_next_reserve_seq() - 1".
 			 */
-			if (this_cpu_in_panic() && ((*seq + 1) < prb_next_reserve_seq(rb)))
+			if (this_cpu_in_panic() &&
+			    (!printk_debug_non_panic_cpus || legacy_allow_panic_sync)
+			    ((*seq + 1) < prb_next_reserve_seq(rb)))
 				(*seq)++;
 			else
 				return false;

Sigh, I am not much happy with the complexity. But if we did not
do it then people might have hard times to realize why the messages
from non-panic CPUs are skipped. Especially because this behavior is
pretty hidden in the log buffer code...

Another motivation is the thread about processing panic notifiers
before stopping CPU. It is still not clear whether it is really
needed. But it is another case where panic() did not work as
expected because a non-panic CPU did not finish a work,
see https://lore.kernel.org/all/20250221022328.47078-1-ryotkkr98@gmail.com/

> > 2nd problem: Is _prb_read_valid() actually safe when
> > 	panic_triggering_all_cpu_backtrace is true?
> >
> >    opinion: It should be safe because the backtraces from different CPUs
> > 	are serialized via printk_cpu_sync_get_irqsave().
> 
> To clarify, by "safe" you mean it does not skip backtrace records from
> other CPUs.
>
> It does not skip their records because trigger_all_cpu_backtrace() waits
> (up to 10 seconds) for the other CPUs to finish storing their backtraces
> before continuing.

OK, I propose the following changes:

  + rename the option to "printk_debug_non_panic_cpus"

  + do not skip the messages in _prb_read_valid() when this option
    is used before the non-panic CPUs are stopped.

Best Regards,
Petr