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
