Received-SPF: pass (google.com: domain of linux-kernel+bounces-86914-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Thu, 29 Feb 2024 16:19:09 +0100
From: Petr Mladek <pmladek@suse.com>
To: John Ogness <john.ogness@linutronix.de>
Cc: Sergey Senozhatsky <senozhatsky@chromium.org>,
	Steven Rostedt <rostedt@goodmis.org>,
	Thomas Gleixner <tglx@linutronix.de>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH printk v2 19/26] printk: Avoid console_lock dance if no
 legacy or boot consoles
Message-ID: <ZeCgbRkIl2q3_9uh@alley>
References: <20240218185726.1994771-1-john.ogness@linutronix.de>
 <20240218185726.1994771-20-john.ogness@linutronix.de>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240218185726.1994771-20-john.ogness@linutronix.de>

On Sun 2024-02-18 20:03:19, John Ogness wrote:
> Currently the console lock is used to attempt legacy-type
> printing even if there are no legacy or boot consoles registered.
> If no such consoles are registered, the console lock does not
> need to be taken.
> 
> Add tracking of legacy console registration and use it with
> boot console tracking to avoid unnecessary code paths, i.e.
> do not use the console lock if there are no boot consoles
> and no legacy consoles.
> 
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -3958,7 +3983,11 @@ void defer_console_output(void)
>  	 * New messages may have been added directly to the ringbuffer
>  	 * using vprintk_store(), so wake any waiters as well.
>  	 */
> -	__wake_up_klogd(PRINTK_PENDING_WAKEUP | PRINTK_PENDING_OUTPUT);
> +	int val = PRINTK_PENDING_WAKEUP;
> +
> +	if (printing_via_unlock)
> +		val |= PRINTK_PENDING_OUTPUT;
> +	__wake_up_klogd(val);

I was thinking about handling this in wake_up_klogd_work_func().
But then I saw that __wake_up_klogd() already handled
PRINTK_PENDING_WAKEUP a similar way. And it even did not
queue the work when there was nothing to do.

It would be nice to handle both on the same place.
It would even untangle the condition in __wake_up_klogd().
Something like:

static void __wake_up_klogd(int val)
{
	if (!printk_percpu_data_ready())
		return;

	preempt_disable();

	/*
	 * Guarantee any new records can be seen by tasks preparing to wait
	 * before this context checks if the wait queue is empty.
	 *
	 * The full memory barrier within wq_has_sleeper() pairs with the full
	 * memory barrier within set_current_state() of
	 * prepare_to_wait_event(), which is called after ___wait_event() adds
	 * the waiter but before it has checked the wait condition.
	 *
	 * This pairs with devkmsg_read:A and syslog_print:A.
	 */
	if (!wq_has_sleeper(&log_wait))		/* LMM(__wake_up_klogd:A) */
		val &= ~PRINTK_PENDING_WAKEUP;

	/*
	 * Simple read is safe. register_console() would flush a newly
	 * registered legacy console when writing the message about
	 * that it has been enabled.
	 */
	if (!printing_via_unlock)
		val &= ~PRINTK_PENDING_OUTPUT;


	if (val) {
		this_cpu_or(printk_pending, val);
		irq_work_queue(this_cpu_ptr(&wake_up_klogd_work));
	}

	preempt_enable();
}

Otherwise, it looks good.

Best Regards,
Petr