On Wed, Feb 07, 2024 at 03:43:52PM +0100, Petr Mladek wrote:
> On Thu 2024-02-01 15:53:40, Sreenath Vijayan wrote:
> > It is useful to be able to dump the printk buffer directly to
> > consoles in some situations so as to not flood the buffer.
>
> This is not longer true. I think that it was valid for
> the previous versions which used separate buffers with
> the kmsg_dump API.
>
> > To do this, we reuse the CONSOLE_REPLAY_ALL mode code in
> > console_flush_on_panic() by moving the code to a helper function
> > console_rewind_all(). This is done because console_flush_on_panic()
> > sets console_may_schedule to 0 but this should not be done in our
> > case.
>
> Also the "c->seq = seq;" is not safe in the panic version.
> But it will be safe when called under the console_lock.
>
> > Then console_rewind_all() is called from the new function
> > dump_printk_buffer() with console lock held to set the console
> > sequence number to oldest record in the buffer for all consoles.
> > Releasing the console lock will flush the contents of printk buffer
> > to the consoles.
>
> My proposed commit message is:
>
> <proposal>
> Add a generic function for replaying the kernel log on consoles.
> It would allow seeing the the log on an unresponsive terminal
> via sysrq interface.
>
> Reuse the existing code from console_flush_on_panic() for
> reseting the sequence numbers. It will be safe when called
> under console_lock(). Also the console_unlock() will
> automatically flush the messages on the consoles.
>
> > --- a/kernel/printk/printk.c
> > +++ b/kernel/printk/printk.c
> > @@ -3134,6 +3134,32 @@ void console_unblank(void)
> > pr_flush(1000, true);
> > }
> >
>
> I would call this function __console_rewind_all(void)
> because it is not safe on its own. Also It would
> deserve a comment, something like:
>
> /*
> * Rewind all consoles to the oldest available record.
> *
> * IMPORTANT: The function is safe only when called under
> * console_lock(). It is not enforced because
> * it is used as a best effort in panic().
> */
> static void __console_rewind_all(void)
>
>
> This would deserve a comment because it is not safe by
> default.
>
> > +static void console_rewind_all(void)
> > +{
> > + struct console *c;
> > + short flags;
> > + int cookie;
> > + u64 seq;
> > +
> > + seq = prb_first_valid_seq(prb);
> > +
> > + cookie = console_srcu_read_lock();
> > + for_each_console_srcu(c) {
> > + flags = console_srcu_read_flags(c);
> > +
> > + if (flags & CON_NBCON) {
> > + nbcon_seq_force(c, seq);
> > + } else {
> > + /*
> > + * This is an unsynchronized assignment. On
> > + * panic legacy consoles are only best effort.
> > + */
>
> We should change this to something like:
>
> /*
> * This assigment is safe only when called under
> * console_lock(). */
> */
>
> > + c->seq = seq;
> > + }
> > + }
> > + console_srcu_read_unlock(cookie);
> > +}
> > +
> > /**
> > * console_flush_on_panic - flush console content on panic
> > * @mode: flush all messages in buffer or just the pending ones
> > @@ -3162,30 +3188,8 @@ void console_flush_on_panic(enum con_flush_mode mode)
> > */
> > console_may_schedule = 0;
> >
> > - if (mode == CONSOLE_REPLAY_ALL) {
> > - struct console *c;
> > - short flags;
> > - int cookie;
> > - u64 seq;
> > -
> > - seq = prb_first_valid_seq(prb);
> > -
> > - cookie = console_srcu_read_lock();
> > - for_each_console_srcu(c) {
> > - flags = console_srcu_read_flags(c);
> > -
> > - if (flags & CON_NBCON) {
> > - nbcon_seq_force(c, seq);
> > - } else {
> > - /*
> > - * This is an unsynchronized assignment. On
> > - * panic legacy consoles are only best effort.
> > - */
> > - c->seq = seq;
> > - }
> > - }
> > - console_srcu_read_unlock(cookie);
> > - }
> > + if (mode == CONSOLE_REPLAY_ALL)
> > + console_rewind_all();
> >
> > console_flush_all(false, &next_seq, &handover);
> > }
> > @@ -4259,6 +4263,15 @@ void kmsg_dump_rewind(struct kmsg_dump_iter *iter)
> > }
> > EXPORT_SYMBOL_GPL(kmsg_dump_rewind);
> >
> > +/**
> > + * Dump the printk ring buffer directly to consoles
> > + */
> > +void dump_printk_buffer(void)
>
> I would call this function console_replay_all(). IMHO, it better describes
> what it does.
>
> > +{
> > + console_lock();
> > + console_rewind_all();
>
> I would add a comment:
>
> /* Consoles are flushed as part of console_unlock(). */
>
> > + console_unlock();
> > +}
> > #endif
>
> Best Regards,
> Petr
Thank you for the review comments. Will fix all these points in the
next version.
Regards,
Sreenath
