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

John Ogness
2022-06-19T23:17:12Z

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