Re: [PATCH 1/2] printk: Introduce LOUD_CON flag

[John Ogness <john.ogness@linutronix.de>]

Hi Marcus,

On 2024-10-16, Marcos Paulo de Souza <mpdesouza@suse.com> wrote:
> Introduce LOUD_CON flag to printk.

Generally speaking, I do not like the name "LOUD_CON". The flag is
related to records, not consoles. Something like "NO_SUPPRESS" or
"FORCE_PRINT" might be more appropriate. Note that naming is not my
strength.

> The new flag will make it possible to
> create a context where printk messages will never be suppressed. This
> new context information will be stored in the already existing
> printk_context per-CPU variable. This variable was changed from 'int' to
> 'unsigned int' to avoid issues with automatic casting.
>
> This mechanism will be used in the next patch to create a loud_console
> context on sysrq handling, removing an existing workaround on the
> loglevel global variable. The workaround existed to make sure that sysrq
> header messages were sent to all consoles.

IMO the more interesting aspect is that the "loud" flag is stored in the
ringbuffer so that the message is not suppressed, even if printed later
(for example because it was deferred). This actually even fixes a bug
since the current workaround will not perform as expected if the sysrq
records are deferred (for example due to threaded printing or consoles
that are registered later).

> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index beb808f4c367..b893825fe21d 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -1321,6 +1321,7 @@ static void boot_delay_msec(int level)
>  	unsigned long timeout;
>  
>  	if ((boot_delay == 0 || system_state >= SYSTEM_RUNNING)
> +		|| is_printk_console_loud()
>  		|| suppress_message_printing(level)) {

I do not think "loud" should be a reason to skip the delays. The delays
are there to slow down printing. I would think that for "loud" messages,
this is even more important. I suppose this function (as well as
printk_delay()) would need a new boolean parameter whether it is a
"loud" message. Then:

	|| (!loud_con && suppress_message_printing(level))

> @@ -2273,6 +2274,9 @@ int vprintk_store(int facility, int level,
>  	if (dev_info)
>  		flags |= LOG_NEWLINE;
>  
> +	if (is_printk_console_loud())
> +		flags |= LOG_LOUD_CON;
> +
>  	if (flags & LOG_CONT) {
>  		prb_rec_init_wr(&r, reserve_size);
>  		if (prb_reserve_in_last(&e, prb, &r, caller_id, PRINTKRB_RECORD_MAX)) {

I guess LOG_LOUD_CON should also be set in the LOG_CONT case (like
LOG_NEWLINE does).

> diff --git a/kernel/printk/printk_safe.c b/kernel/printk/printk_safe.c
> index 2b35a9d3919d..4618988baeea 100644
> --- a/kernel/printk/printk_safe.c
> +++ b/kernel/printk/printk_safe.c
> @@ -12,7 +12,30 @@
>  
>  #include "internal.h"
>  
> -static DEFINE_PER_CPU(int, printk_context);
> +static DEFINE_PER_CPU(unsigned int, printk_context);
> +
> +#define PRINTK_SAFE_CONTEXT_MASK		0x0000ffffU
> +#define PRINTK_LOUD_CONSOLE_CONTEXT_MASK	0xffff0000U
> +#define PRINTK_LOUD_CONSOLE_CONTEXT_OFFSET	0x00010000U
> +
> +void noinstr printk_loud_console_enter(void)
> +{
> +	cant_migrate();
> +	this_cpu_add(printk_context, PRINTK_LOUD_CONSOLE_CONTEXT_OFFSET);
> +}

Have you tested this with lockdep? AFAICT, the write_sysrq_trigger()
path can migrate since it is only using rcu_read_lock() in
__handle_sysrq().

John Ogness