Messages in this thread | | | From | John Ogness <> | Subject | Re: [PATCH printk v2 08/12] printk: add pr_flush() | Date | Fri, 08 Apr 2022 21:03:03 +0206 |
| |
On 2022-04-06, Petr Mladek <pmladek@suse.com> wrote: >> @@ -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 suggest 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()
At this point in the series, we do not have kthreads yet. ;-)
And besides, you are implying that there are messages in the ringbuffer, but _nobody_ is printing them. That would be a bug in our implementation.
>> + >> + 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.
The messages must be flushed by another context. It is wrong to rely on this context to do the flushing. In direct mode, those messages should have been flushed before the above console_lock() succeeded. Otherwise it is a bug. (For threaded mode, console_unlock() does not flush anyway.)
> Well, it is ugly to keep it this way.
If you don't mind, I will not change this function for v3. I do not want to start sprinkling console_trylock()/console_unlock() calls all over the code to cover up possible bugs and races in the implementation.
John
| |