Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sun, 19 Jun 2022 22:39:42 +0200
From:   "Jason A. Donenfeld" <Jason@zx2c4.com>
To:     John Ogness <john.ogness@linutronix.de>
Cc:     Petr Mladek <pmladek@suse.com>, Marco Elver <elver@google.com>,
        linux-kernel@vger.kernel.org,
        Sergey Senozhatsky <senozhatsky@chromium.org>,
        Steven Rostedt <rostedt@goodmis.org>
Subject: Re: [PATCH] printk: allow direct console printing to be enabled
 always
Message-ID: <Yq+Jjoyn/wj7yzeQ@zx2c4.com>
References: <YqyDqMAPHIxjA/xZ@zx2c4.com>
 <20220617133847.27517-1-Jason@zx2c4.com>
 <87a6a8syk8.fsf@jogness.linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <87a6a8syk8.fsf@jogness.linutronix.de>
Precedence: bulk

Hi John,

Thanks for your review. I'll have a v2 for you shortly.

On Sun, Jun 19, 2022 at 01:11:43PM +0206, John Ogness wrote:
> > +	printk.always_direct=
> 
> Do we need the word "always" in there? I would prefer the simplied:
> printk.direct=

Sure, no problem. I'll do that for v2.

> 
> > +			Rather than using kthreads for printk output, always
> > +			write to the console immediately. This has performance
> 
> The "best effort" part needs to be in there. Perhaps:
> 
> Rather than using kthreads for printk output, always attempt to write to
> the console immediately.

Ack.

> 
> > +			implications, but will result in a more faithful
> > +			ordering and interleaving with other processes writing
> > +			to the console.
> 
> My main concern is that "direct printing" is not synchronous. Since
> 2.4.10 it has not been printk's goal to be synchronous. Your patch is
> making the issue less likely again, but not solving it. And since this
> is the first time we are documenting the printk printing behavior, we
> should be careful to not mislead users to think it is truly direct.

Sure, that makes sense. The "attempt to" language clarifies that well.

> > +config PRINTK_ALWAYS_DIRECT
> 
> (perhaps)
> config PRINTK_DIRECT

Ack.

> 
> > +	bool "Flush printk output immediately"
> 
> Attempt to flush printk output immediately

Ack.

> 
> > +	depends on PRINTK
> > +	help
> > +	  Rather than using kthreads for printk output, always write to the
> 
> always attempt to write

Ack.

> 
> > +	  console immediately. This has performance implications, but will
> > +	  result in a more faithful ordering and interleaving with other
> > +	  processes writing to the console.
> > +
> > +	  Say N here unless you really need this. This may also be controlled
> > +	  at boot time with printk.always_direct=0/1.
> 
> (perhaps)
> printk.direct=0/1

Ack.

> 
> > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> > index ea3dd55709e7..d9f419a88429 100644
> > --- a/kernel/printk/printk.c
> > +++ b/kernel/printk/printk.c
> > @@ -471,6 +479,10 @@ void printk_prefer_direct_exit(void)
> >   */
> >  static inline bool allow_direct_printing(void)
> >  {
> > +	/* If the user has explicitly enabled this to be on always. */
> > +	if (always_direct_printk)
> > +		return true;
> 
> There is no reason to start the threads if they won't be used. Perhaps
> something like this instead:
> 
> -------- BEGIN HUNK -------
> @@ -3602,11 +3610,13 @@ static int __init printk_activate_kthreads(void)
>  {
>  	struct console *con;
>  
> -	console_lock();
> -	printk_kthreads_available = true;
> -	for_each_console(con)
> -		printk_start_kthread(con);
> -	console_unlock();
> +	if (!always_direct_printk) {
> +		console_lock();
> +		printk_kthreads_available = true;
> +		for_each_console(con)
> +			printk_start_kthread(con);
> +		console_unlock();
> +	}
>  
>  	return 0;
>  }
> -------- END HUNK -------

Good thinking. I'll do it like this.

> Direct printing has a lot of technical issues. It is sometimes directly
> responsible for kernels dying. It is sometimes directly responsible for
> preventing important information from being made available at crash
> time. For the fbcon, direct printing is a ticking timebomb. And in many
> cases it isn't even truly doing direct printing anyway.
> 
> Direct printing (in its current form) must be phased out at some
> point. We are working to bring true synchronous console printing
> mainline.
> 
> Aside from my above comments, I have no problems with this patch.

That's a fair point, though it remains a valuable resource for debugging
and CI. The way this patch is written, it defaults to off, and the help
text is kind of discouraging of its use, which is what we want I think.

v2 on its way.

Jason