lkml.org 
[lkml]   [2018]   [Sep]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 4.19 regression fix] printk: For early boot messages check loglevel when flushing the buffer
From
Date
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++;
>

\
 
 \ /
  Last update: 2018-09-10 17:02    [W:0.080 / U:0.060 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site