From:   John Ogness <john.ogness@linutronix.de>
To:     Petr Mladek <pmladek@suse.com>
Cc:     Sergey Senozhatsky <senozhatsky@chromium.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        linux-kernel@vger.kernel.org,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Subject: Re: [PATCH printk v1 04/18] printk: Add per-console suspended state
In-Reply-To: <ZAieQtcs7YEuCCDa@alley>
References: <20230302195618.156940-1-john.ogness@linutronix.de>
 <20230302195618.156940-5-john.ogness@linutronix.de>
 <ZAieQtcs7YEuCCDa@alley>
Date:   Fri, 17 Mar 2023 14:28:04 +0106
Message-ID: <87y1nv5x8j.fsf@jogness.linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

On 2023-03-08, Petr Mladek <pmladek@suse.com> wrote:
>> --- a/include/linux/console.h
>> +++ b/include/linux/console.h
>> @@ -153,6 +153,8 @@ static inline int con_debug_leave(void)
>>   *			receiving the printk spam for obvious reasons.
>>   * @CON_EXTENDED:	The console supports the extended output format of
>>   *			/dev/kmesg which requires a larger output buffer.
>> + * @CON_SUSPENDED:	Indicates if a console is suspended. If true, the
>> + *			printing callbacks must not be called.
>>   */
>>  enum cons_flags {
>>  	CON_PRINTBUFFER		= BIT(0),
>> @@ -162,6 +164,7 @@ enum cons_flags {
>>  	CON_ANYTIME		= BIT(4),
>>  	CON_BRL			= BIT(5),
>>  	CON_EXTENDED		= BIT(6),
>> +	CON_SUSPENDED		= BIT(7),
>
> We have to show it in /proc/consoles, see fs/proc/consoles.c.

Are we supposed to show all flags in /proc/consoles? Currently
CON_EXTENDED is not shown either.

>> --- a/kernel/printk/printk.c
>> +++ b/kernel/printk/printk.c
>> @@ -2574,11 +2590,26 @@ void suspend_console(void)
>>  
>>  void resume_console(void)
>>  {
>> +	struct console *con;
>> +
>>  	if (!console_suspend_enabled)
>>  		return;
>>  	down_console_sem();
>>  	console_suspended = 0;
>>  	console_unlock();
>> +
>> +	console_list_lock();
>> +	for_each_console(con)
>> +		console_srcu_write_flags(con, con->flags & ~CON_SUSPENDED);
>> +	console_list_unlock();
>> +
>> +	/*
>> +	 * Ensure that all SRCU list walks have completed. All printing
>> +	 * contexts must be able to see they are no longer suspended so
>> +	 * that they are guaranteed to wake up and resume printing.
>> +	 */
>> +	synchronize_srcu(&console_srcu);
>> +
>
> The setting of the global "console_suspended" and per-console
> CON_SUSPENDED flag is not synchronized. As a result, they might
> become inconsistent:

They do not need to be synchronized and it doesn't matter if they become
inconsistent. With this patch they are no longer related. One is for
tracking the state of the console (CON_SUSPENDED), the other is for
tracking the suspend trick for the console_lock.

> I think that we could just remove the global "console_suspended" flag.
>
> IMHO, it used to be needed to avoid moving the global "console_seq" forward
> when the consoles were suspended. But it is not needed now with the
> per-console con->seq. console_flush_all() skips consoles when
> console_is_usable() fails and it bails out when there is no progress.

The @console_suspended flag is used to allow console_lock/console_unlock
to be called without triggering printing. This is probably so that vt
code can make use of the console_lock for its own internal locking, even
when in a state where ->write() should not be called. I would expect we
still need it, even if the consoles do not.

The only valid criteria for allowing to call ->write() is CON_SUSPENDED.

>> @@ -3712,14 +3745,7 @@ static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progre
>>  		}
>>  		console_srcu_read_unlock(cookie);
>>  
>> -		/*
>> -		 * If consoles are suspended, it cannot be expected that they
>> -		 * make forward progress, so timeout immediately. @diff is
>> -		 * still used to return a valid flush status.
>> -		 */
>> -		if (console_suspended)
>> -			remaining = 0;
>
> I wonder if it would make sense to add a comment somewhere,
> e.g. above the later check:
>
> +		/* diff is zero also when there is no usable console. */
> 		if (diff == 0 || remaining == 0)
> 			break;

I think that is obvious, but I can add a similar comment to remind the
reader that only usable consoles are counted.

> Anyway, we should update the comment above pr_flush():
>
> -  * Return: true if all enabled printers are caught up.
> +  * Return: true if all usable printers are caught up.

Nice catch.

John