Messages in this thread | | | From | John Ogness <> | Subject | Re: [PATCH printk v2 5/8] printk: nbcon: Add sequence handling | Date | Tue, 29 Aug 2023 13:57:11 +0206 |
| |
On 2023-08-10, Petr Mladek <pmladek@suse.com> wrote: >> void console_flush_on_panic(enum con_flush_mode mode) >> { >> + struct console *c; >> bool handover; >> - u64 next_seq; >> + short flags; >> + int cookie; >> + u64 seq; >> + >> + seq = prb_first_valid_seq(prb); >> + >> + /* >> + * Safely handle the atomic consoles before trying to flush any >> + * legacy consoles. >> + */ > > This is a bit weird because the loop below just sets sequence > number for NBCON consoles. But they are not really flushed. > > I think that you already agreed with it for v3. But let me mention > it here for completeness. > > I would prefer to just add the API and use it later when some > particular action get supported. And the flush could not do > anything until nbcon_write() callback is added.
Actually I agreed with this for v2, but didn't take it far enough. I now also agree that console_flush_on_panic() should not be modified at all until the actual flushing functions are available.
>> @@ -3829,7 +3846,9 @@ static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progre >> if (!console_is_usable(c)) >> continue; >> >> - if (locked) >> + if (flags & CON_NBCON) >> + printk_seq = nbcon_seq_read(c); >> + else if (locked) >> printk_seq = c->seq; >> else >> continue; > > I think that I mentioned this already in a previous patch. The "else > continue" path is bad. It allows quietly skip messages for classic > consoles when "locked" is false. I know that it should not happen > but... > > A solution would be to add WARN_ON_ONCE() before the continue.
I like the WARN_ON_ONCE() suggestion. For v3 I will do:
if (flags & CON_NBCON) { printk_seq = nbcon_seq_read(c); } else { WARN_ON_ONCE(!locked); printk_seq = c->seq; }
>> +static bool nbcon_seq_try_update(struct nbcon_context *ctxt) >> +{ >> + struct console *con = ctxt->console; >> + u64 con_seq = nbcon_seq_read(con); >> + >> + while (con_seq < ctxt->seq) { > > What if anyone called nbcon_seq_force() to reply the entire log > in the meantime? > > IMHO, we should remember the original nbcon_seq before > the context handle a line. And this function should update > nbcon_seq only when it has not been changed by other context > in the meantime.
Makes sense. I will do that for v3.
John
| |