lkml.org 
[lkml]   [2022]   [Mar]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH printk v1 08/13] printk: add pr_flush()
    Date
    On 2022-02-17, Petr Mladek <pmladek@suse.com> wrote:
    >> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
    >> index 02bde45c1149..1e80fd052bd5 100644
    >> --- a/kernel/printk/printk.c
    >> +++ b/kernel/printk/printk.c
    >> @@ -2802,8 +2804,10 @@ void console_unblank(void)
    >> if (oops_in_progress) {
    >> if (down_trylock_console_sem() != 0)
    >> return;
    >> - } else
    >> + } else {
    >> + pr_flush(1000, true);
    >
    > It would make more sense to flush the consoles after they are
    > unblanked. I mean to move this to the end of the function.

    Agreed.

    > Also it is not obvious why this is not called when oops_in_progress
    > is set. I guess that it is because trylock is needed in this case.
    > It should be handled inside pr_flush().
    >
    > I mean that pr_flush() should internally use trylock when
    > @oops_in_progress is set. It will make it safe even in this
    > mode.

    pr_flush() is a might_sleep() function. We agreed on this at
    LPC2019.

    Creating a pr_flush() that will directly push out the messages (or
    busy-wait) in non-preemptible contexts is complicated. It might be
    something to attempt for the future, but I would prefer to avoid it at
    this stage.

    >> console_lock();
    >> + }
    >>
    >> console_locked = 1;
    >> console_may_schedule = 0;
    >> @@ -2869,6 +2873,7 @@ struct tty_driver *console_device(int *index)
    >> */
    >> void console_stop(struct console *console)
    >> {
    >> + pr_flush(1000, true);
    >
    > It would be enough to flush just the given @console.

    For v2 I will create an internal __pr_flush() to allow specifying that
    only a single console is flushed. The high level pr_flush() will then
    call __pr_flush() specifying all consoles.

    > It might be possible to take over the job from the related
    > kthread and flush it in this context. Well, I am not sure if
    > it is a good idea.

    I agree that it might not be a good idea. Let's keep things simple for
    now.

    >> console_lock();
    >> console->flags &= ~CON_ENABLED;
    >> console_unlock();
    >> @@ -2880,6 +2885,7 @@ void console_start(struct console *console)
    >> console_lock();
    >> console->flags |= CON_ENABLED;
    >> console_unlock();
    >> + pr_flush(1000, true);
    >
    > Same here.

    OK.

    >> }
    >> EXPORT_SYMBOL(console_start);
    >>
    >> @@ -3249,6 +3255,71 @@ static int __init printk_late_init(void)
    >> late_initcall(printk_late_init);
    >>
    >> #if defined CONFIG_PRINTK
    >> +/**
    >> + * pr_flush() - Wait for printing threads to catch up.
    >> + *
    >
    > Alternative solution would be to take over the job from the kthreads
    > and flush the consoles in this context. Well, I am not sure
    > if it is a good idea or not.

    Since pr_flush() is might_sleep() this would be relatively simple. Just
    grab the console mutex and go. My concern is that this task may have
    different scheduling parameters that could negatively affect the
    system. For normal operation, I really would prefer that the designated
    kthreads do the work. If "waiting for the kthreads" turns out to be
    problematic, then maybe we could go down this path.

    >> + * @timeout_ms: The maximum time (in ms) to wait.
    >> + * @reset_on_progress: Reset the timeout if forward progress is seen.
    >> + *
    >> + * A value of 0 for @timeout_ms means no waiting will occur. A value of -1
    >> + * represents infinite waiting.
    >> + *
    >> + * If @reset_on_progress is true, the timeout will be reset whenever any
    >> + * printer has been seen to make some forward progress.
    >> + *
    >> + * Context: Process context. May sleep while acquiring console lock.
    >> + * Return: true if all enabled printers are caught up.
    >> + */
    >> +bool pr_flush(int timeout_ms, bool reset_on_progress)
    >> +{
    >> + int remaining = timeout_ms;
    >> + struct console *con;
    >> + u64 last_diff = 0;
    >> + u64 printk_seq;
    >> + u64 diff;
    >> + u64 seq;
    >> +
    >> + might_sleep();
    >> +
    >> + seq = prb_next_seq(prb);
    >> +
    >> + for (;;) {
    >> + diff = 0;
    >> +
    >> + console_lock();
    >> + for_each_console(con) {
    >> + if (!console_is_usable(con))
    >> + continue;
    >> + printk_seq = con->seq;
    >> + if (printk_seq < seq)
    >> + diff += seq - printk_seq;
    >> + }
    >> + console_unlock();
    >> +
    >> + 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);
    >> +}
    >> +EXPORT_SYMBOL(pr_flush);
    >
    > Summary:
    >
    > The pr_flush() API and the optional timeout look reasonable to me.
    >
    > Please, handle oops_in_progress in pr_flush() and make it safe in this
    > mode. It will allow to move it at the end of console_unblank() where
    > it makes more sense.

    I will add another oops_in_progress check at the end. pr_flush() will
    not be made safe for oops_in_progress. Keep in mind that when
    oops_in_progress is set, direct printing will be active, so there should
    be no need for pr_flush() anyway.

    > I do not resist on flushing only the given consoles in console_stop()
    > and console_start(). It is nice to have and can be done later.

    It is a simple change. I will do it for v2.

    > Also I do not resist on doing the flush in the context of the caller.
    > I am not even sure if it is a good idea. We could play with it
    > later when there are some problems with the current approach
    > in practice.

    For now, let's keep it as a waiting function.

    John

    \
     
     \ /
      Last update: 2022-03-02 18:24    [W:3.015 / U:1.188 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site