Messages in this thread | | | From | John Ogness <> | Subject | Re: [PATCH v2] printk: allow direct console printing to be enabled always | Date | Mon, 20 Jun 2022 01:23:04 +0206 |
| |
On 2022-06-19, "Jason A. Donenfeld" <Jason@zx2c4.com> 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.
> + > +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
| |