lkml.org 
[lkml]   [2022]   [Apr]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH printk v2 08/12] printk: add pr_flush()
    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

    \
     
     \ /
      Last update: 2022-04-06 19:22    [W:2.177 / U:0.348 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site