Re: [PATCH printk v2 08/12] printk: add pr_flush()

Petr Mladek
2022-04-06T17:17:42Z

On Tue 2022-04-05 15:31:31, John Ogness wrote:
> Provide a might-sleep function to allow waiting for console printers
> to catch up to the latest logged message.
>
> Use pr_flush() whenever it is desirable to get buffered messages
> printed before continuing: suspend_console(), resume_console(),
> console_stop(), console_start(), console_unblank().
>
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 06f0b29d909d..d0369afaf365 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -3321,6 +3331,79 @@ static int __init printk_late_init(void)
> late_initcall(printk_late_init);
>
> #if defined CONFIG_PRINTK
> +/* If @con is specified, only wait for that console. Otherwise wait for all. */
> +static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progress)
> +{
> + int remaining = timeout_ms;
> + struct console *c;
> + u64 last_diff = 0;
> + u64 printk_seq;
> + u64 diff;
> + u64 seq;
> +
> + might_sleep();
> +
> + seq = prb_next_seq(prb);

I auggest to add here:

/*
* Try to flush the messages when kthreads are not available
* and there is not other console lock owner.
*/
if (console_trylock())
console_unlock()

> +
> + for (;;) {
> + diff = 0;
> +
> + console_lock();
> + for_each_console(c) {
> + if (con && con != c)
> + continue;
> + if (!console_is_usable(c))
> + continue;
> + printk_seq = c->seq;
> + if (printk_seq < seq)
> + diff += seq - printk_seq;
> + }
> + console_unlock();

This is a bit sub-optimal when the kthreads are not available or
are disabled. In this case, the messages are flushed [*] by
the console_unlock() and the diff is outdated.

As a result, the code would non-necessarily sleep 100ms before
it realizes that the job is done.

One might argue that the problem will be only when there are
non-handled messages and in situations when the delay probably
is not critical.

Well, it is ugly to keep it this way. I suggest to add the extra
console_trylock()/unlock() at the beginning.

> +
> + if (diff != last_diff && reset_on_progress)
> + remaining = timeout_ms;
> +
> + if (diff == 0 || remaining == 0)
> + break;
> +
> + if (remaining < 0) {
> + /* no timeout limit */
> + msleep(100);
> + } else if (remaining < 100) {
> + msleep(remaining);
> + remaining = 0;
> + } else {
> + msleep(100);
> + remaining -= 100;
> + }
> +
> + last_diff = diff;
> + }
> +
> + return (diff == 0);
> +}

Otherwise, it looks good to me. It is an interesting function.
I wonder if it gets popular also outside printk code. ;-)

Best Regards,
Petr