On Wed, 2024-10-16 at 20:17 +0206, John Ogness wrote:
> Hi Marcus,
>
> On 2024-10-16, Marcos Paulo de Souza <mpdesouza@xxxxxxxx> 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.
Makes sense. I'll talk with Petr to check which better name we case,
but personally speaking I liked FORCE_CON that he suggested.
>
> > 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).
Indeed, I'll describe that this new behavior fixes a problem.
>
> > 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))
Done locally, thanks!
>
> > @@ -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().
Now I have and I found the error. Let me investigate how I can proceed
here.
Thanks a lot for the review!
>
> John Ogness
