Messages in this thread | | | Subject | Re: [PATCH 4.19 regression fix] printk: For early boot messages check loglevel when flushing the buffer | From | Hans de Goede <> | Date | Mon, 10 Sep 2018 17:02:14 +0200 |
| |
Hi,
On 10-09-18 16:57, Petr Mladek wrote: > On Fri 2018-09-07 13:21:32, Sergey Senozhatsky wrote: >> On (09/06/18 16:28), Petr Mladek wrote: >>> On Thu 2018-09-06 16:29:40, Sergey Senozhatsky wrote: >>>> On (09/05/18 13:02), Petr Mladek wrote: >>>>> Note that the first registered console prints all messages >>>>> even without this flag. >>>> >>>> Hmm, OK, interesting point. >>>> >>>> I assumed that the first console usually has CON_PRINTBUFFER bit set. >>>> Or even a CON_PRINTBUFFER | CON_ANYTIME combo. E.g. 8250. It sort of >>>> makes sense to have CON_PRINTBUFFER for the first console. Any later >>>> consoles [e.g. fbcon, netcon] don't necessarily have CON_PRINTBUFFER. >>>> >>>> And the first console has CON_PRINTBUFFER bit set. Well, just because >>>> it sounds reasonable. Those were the main assumptions behind my code >>>> snippet. Was any of those assumptions wrong? >>> >>> This assumption makes sense. In fact, I was wrong. I thought that >>> console_seq/console_idx were not updated until the first console >>> was registered. But it is not the case. >>> >>> It means that the hack with exclusive_console might be usable. >> >> Yeah, it is a hack. But not as dirty as it might appear, I think. In some >> sense it's aligned with what we do for exlusive_consoles - we treat exclusive >> consoles specially. So specially that even if the system panics while we >> re-flush logbuf messages to a new exclusive console, we flush_on_panic() only >> to that exclusive console, ignoring the rest of them. > > I consider this a bug. Another bug is that other consoles might miss > messages that appear while the older messages are being > replayed on exclusive_console. > > AFAIK, nobody complains about it. But I not comfortable with reusing > this racy pattern even more ;-) > > >> Not sure if it's totally right. There can be a netcon, for instance, >> available, which will not see panic flush() because of a exclusive >> console: > > Good catch. > >> --- >> >> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c >> index c036f128cdc3..ede29a7ba6db 100644 >> --- a/kernel/printk/printk.c >> +++ b/kernel/printk/printk.c >> @@ -2545,6 +2545,7 @@ void console_flush_on_panic(void) >> * ensure may_schedule is cleared. >> */ >> console_trylock(); >> + exclusive_console = NULL; > > This is not be enough. It would cause replying old messages > on all consoles. > > Most problems should probably be solved when we store console_seq > before setting exclusive_console. Then we could clear > exclusive_console when reaching the stored sequence number. > > Can this be that simple? ;-) > > >>> I prefer the revert for now. >> >> OK, agreed. >> IIRC I didn't see any upstream code which would have been fixed >> by the commit in question. > > My proposal for 4.19: > > > From 46ceb577021c8899edd1977a52929b309a5ebf85 Mon Sep 17 00:00:00 2001 > From: Petr Mladek <pmladek@suse.com> > Date: Mon, 10 Sep 2018 15:52:06 +0200 > Subject: [PATCH] Revert "printk: make sure to print log on console." > > This reverts commit 375899cddcbb26881b03cb3fbdcfd600e4e67f4a. > > The visibility of early messages did not longer take into account > "quiet", "debug", and "loglevel" early parameters. > > It would be possible to invalidate and recompute LOG_NOCONS flag > for the affected messages. But it would be hairy. > > Instead this patch just reverts the problematic commit. We could > come up with a better solution for the original problem. For example, > we could simplify the logic and just mark messages that should always > be visible or always invisible on the console. > > Also this patch does not put back the unused LOG_NOCONS flag. > > Reported-by: Hans de Goede <hdegoede@redhat.com> > Signed-off-by: Petr Mladek <pmladek@suse.com>
Sounds like a good solution to me:
Acked-by: Hans de Goede <hdegoede@redhat.com>
Regards,
Hans
> --- > kernel/printk/printk.c | 11 ++++------- > 1 file changed, 4 insertions(+), 7 deletions(-) > > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c > index fd6f8ed28e01..5fae295fcb75 100644 > --- a/kernel/printk/printk.c > +++ b/kernel/printk/printk.c > @@ -351,7 +351,6 @@ static int console_msg_format = MSG_FORMAT_DEFAULT; > */ > > enum log_flags { > - LOG_NOCONS = 1, /* suppress print, do not print to console */ > LOG_NEWLINE = 2, /* text ended with a newline */ > LOG_PREFIX = 4, /* text started with a prefix */ > LOG_CONT = 8, /* text is a fragment of a continuation line */ > @@ -1881,9 +1880,6 @@ int vprintk_store(int facility, int level, > if (dict) > lflags |= LOG_PREFIX|LOG_NEWLINE; > > - if (suppress_message_printing(level)) > - lflags |= LOG_NOCONS; > - > return log_output(facility, level, lflags, > dict, dictlen, text, text_len); > } > @@ -2368,10 +2364,11 @@ void console_unlock(void) > break; > > msg = log_from_idx(console_idx); > - if (msg->flags & LOG_NOCONS) { > + if (suppress_message_printing(msg->level)) { > /* > - * Skip record if !ignore_loglevel, and > - * record has level above the console loglevel. > + * Skip record we have buffered and already printed > + * directly to the console when we received it, and > + * record that has level above the console loglevel. > */ > console_idx = log_next(console_idx); > console_seq++; >
| |