Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 13 Apr 2023 17:15:54 +0200
From:   Petr Mladek <pmladek@suse.com>
To:     John Ogness <john.ogness@linutronix.de>
Cc:     Sergey Senozhatsky <senozhatsky@chromium.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH printk v1 03/18] printk: Consolidate console deferred
 printing
Message-ID: <ZDgcqqLNZuvsbgET@alley>
References: <20230302195618.156940-1-john.ogness@linutronix.de>
 <20230302195618.156940-4-john.ogness@linutronix.de>
 <ZAiKhAA37/jehmD7@alley>
 <871qln7cle.fsf@jogness.linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <871qln7cle.fsf@jogness.linutronix.de>
Precedence: bulk

On Fri 2023-03-17 14:11:01, John Ogness wrote:
> On 2023-03-08, Petr Mladek <pmladek@suse.com> wrote:
> >> --- a/kernel/printk/printk.c
> >> +++ b/kernel/printk/printk.c
> >> @@ -2321,7 +2321,10 @@ asmlinkage int vprintk_emit(int facility, int level,
> >>  		preempt_enable();
> >>  	}
> >>  
> >> -	wake_up_klogd();
> >> +	if (in_sched)
> >> +		defer_console_output();
> >> +	else
> >> +		wake_up_klogd();
> >
> > Nit: I would add an empty line here. Or I would move this up into the
> >      previous if (in_sched()) condition.
> 
> Empty line is ok. I do not want to move it up because the above
> condition gets more complicated later. IMHO a simple if/else for
> specifying what the irq_work should do is the most straight forward
> here.
> 
> >> @@ -3811,11 +3814,30 @@ static void __wake_up_klogd(int val)
> >>  	preempt_enable();
> >>  }
> >>  
> >> +/**
> >> + * wake_up_klogd - Wake kernel logging daemon
> >> + *
> >> + * Use this function when new records have been added to the ringbuffer
> >> + * and the console printing for those records is handled elsewhere. In
> >
> > "elsewhere" is ambiguous. I would write:
> >
> > "and the console printing for those records maybe handled in this context".
> 
> The reason for using the word "elsewhere" is because in later patches it
> is also the printing threads that can handle it. I can change it to
> "this context" for this patch, but then after adding threads I will need
> to adjust the comment again. How about:
> 
> "and the console printing for those records should not be handled by the
> irq_work context because another context will handle it."

It is better but still a bit hard to follow. As a reader, I see three
contexts:

   + context that calls wake_up_klogd()
   + irq_work context
   + another context that would handle the console printing

The confusing part is that the "another context". It might be the
context calling wake_up_klogd(). If feels like scratching
right ear by left hand ;-)

In fact, also the next sentence "In this case only the logging
daemon needs to be woken." is misleading. Also the printk
kthreads need to be woken but it is done by another function.

OK, what about?

 * wake_up_klogd - Wake kernel logging daemon
 *
 * Use this function when new records have been added to the ringbuffer
 * and the console printing does not have to be deferred to irq_work
 * context. This function will only wake the logging daemons.


Heh, the "wake_up_klogd_work" has became confusing since it started
handling deferred console output. And it is even more confusing now
when it does not handle the kthreads which are yet another deferred
output. But I can't think of any reasonable solution at the moment.

Maybe, we should somehow distinguish the API that will handle only
the legacy consoles. For example, suspend_console() handles both
but console_flush_all() will handle only the legacy ones.

I think that we are going to use nbcon_ prefix for the API
handling the new consoles. Maybe we could use another prefix
for the legacy-consoles-specific API.

Hmm, what about?

    + "bcon_" like the opposite of "nbcon_" but it might be
      confused with boot console

    + "lcon_" like "legacy" or "locked" consoles

    + "scon" like synchronized or serialized consoles.


Honestly, I am not sure how much important this is. But it might
be pretty helpful for anyone who would try to understand the code
in the future. And this rework might be really challenging for
future archaeologists. Not to say, that legacy consoles will
likely stay with us many years, maybe decades.

Best Regards,
Petr