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

[John Ogness]

On 2024-10-21, Petr Mladek <pmladek@xxxxxxxx> wrote:
>> That will not work because migrate_enable() can only be called from
>> can_sleep context. Instead, the migrate_disable()/enable() should be at
>> the few (one?) call sites where printk_loud_console_enter()/exit() is
>> used from task context.
>
> Hmm, if I get it correctly, we could not use migrate_disable() in
> __handle_sysrq() because it can be called also in atomic context,

I am talking about callers of __handle_sysrq() and/or their callers.

For example write_sysrq_trigger() could do:

	migrate_disable();
	__handle_sysrq(c, false);
	migrate_enable();

Or a new wrapper could be introduced for this purpose:

static inline void wrapper handle_sysrq_task(u8 key, bool check_mask)
{
	migrate_disable();
	__handle_sysrq(key, check_mask);
	migrate_enable();
}

A quick grep shows about 25 call sites to check.

> I do not see any easy way how to distinguish whether it was called in
> an atomic context or not.

There is no clean way to do that. If this information is needed, it must
be tracked by the call chain.

> So, I see three possibilities:
>
>   1. Explicitly call preempt_disable() in __handle_sysrq().
>
>      It would be just around the the single line or the help. But still,
>      I do not like it much.

Not acceptable for PREEMPT_RT since sysrq is exposed to external inputs.

>   2. Avoid the per-CPU variable. Force adding the LOUD_CON/FORCE_CON
>      flag using a global variable, e.g. printk_force_console.
>
>      The problem is that it might affect also messages printed by
>      other CPUs. And there might be many.
>
>      Well, console_loglevel is a global variable. The original code
>      had a similar problem.

If disabling migration is not an option for you, this would be my second
choice. I assume tagging too many messages is better than not tagging
enough. And, as you say, this is effectively what the current code is
trying to do.

>   3. Add the LOUD_CON/FLUSH_CON flag via a parameter. For example,
>      by a special LOGLEVEL_FORCE_CON, similar to LOGLEVEL_SCHED.
>
>      I might work well for __handle_sysrq() which calls the affected
>      printk() directly.
>
>      But it won't work, for example, for kdb_show_stack(). It wants
>      to show messages printed by a nested functions.

Right, this has limited usefulness and might miss the important things,
which tend to be within helper functions.

John