RE: [PATCH v2] printk: allow direct console printing to be enabled always

[David Laight]

From: John Ogness
> Sent: 20 June 2022 00:17
> 
> On 2022-06-19, "Jason A. Donenfeld" <Jason@xxxxxxxxx> wrote:
> > diff --git a/init/Kconfig b/init/Kconfig
> > index c7900e8975f1..47466aa2b0e8 100644
> > --- a/init/Kconfig
> > +++ b/init/Kconfig
> 
> Sorry, I missed this in your v1. But I think this config belongs in
> lib/Kconfig.debug under the "printk and dmesg options" menu.
> 
> > @@ -798,6 +798,18 @@ config PRINTK_INDEX
> >
> >  	  There is no additional runtime cost to printk with this enabled.
> >
> > +config PRINTK_DIRECT
> > +	bool "Attempt to flush printk output immediately"
> > +	depends on PRINTK
> > +	help
> > +	  Rather than using kthreads for printk output, always attempt to write
> > +	  to the 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.direct=0/1.
> > +
> >  #
> >  # Architectures with an unreliable sched_clock() should select this:
> >  #
> > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> > index ea3dd55709e7..43f8a0074ed6 100644
> > --- a/kernel/printk/printk.c
> > +++ b/kernel/printk/printk.c
> > @@ -178,6 +178,14 @@ static int __init control_devkmsg(char *str)
> >  }
> >  __setup("printk.devkmsg=", control_devkmsg);
> >
> > +static bool printk_direct = IS_ENABLED(CONFIG_PRINTK_DIRECT);
> 
> I understand why you would name the variable to match the boot arg. But
> I would prefer the _internal_ variable had a name that makes it clear
> (to us developers) that it is a permanent setting. Perhaps
> printk_direct_only or printk_direct_always?
> 
> The reason is because when kthreads are active, direct printing is
> turned on and off dynamically (using @printk_prefer_direct). And if this
> new variable is named @printk_direct, one could easily mistake its
> meaning to be related to the dynamic turning on and off.

Do you need both variables?
I presume the whole lot can be compiled out (for small kernels).
Apart from that having a sysctl to control which message levels
get 'direct printing' (much like the one that controls whether
they get printed at all) would surely be enough.

That just leaves the question of the initial level at boot.

	David

> 
> > +
> > +static int __init control_printk_direct(char *str)
> > +{
> > +	return kstrtobool(str, &printk_direct);
> > +}
> > +__setup("printk.direct=", control_printk_direct);
> > +
> >  char devkmsg_log_str[DEVKMSG_STR_MAX_SIZE] = "ratelimit";
> >  #if defined(CONFIG_PRINTK) && defined(CONFIG_SYSCTL)
> >  int devkmsg_sysctl_set_loglvl(struct ctl_table *table, int write,
> > @@ -3602,6 +3610,9 @@ static int __init printk_activate_kthreads(void)
> >  {
> >  	struct console *con;
> >
> > +	if (printk_direct)
> 
> I'm wondering if we should output a message here. My suggestion is:
> 
> pr_info("printing threads disabled, using direct printing\n");
> 
> > +		return 0;
> > +
> >  	console_lock();
> >  	printk_kthreads_available = true;
> >  	for_each_console(con)
> 
> Otherwise it looks OK to me. But you may want to wait on a response from
> Petr, Sergey, or Steven before sending a v3. You are adding a kernel
> config and a boot argument. Both of these are sensitive topics that
> require more feedback from others.
> 
> John Ogness

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)