Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
From:   John Ogness <john.ogness@linutronix.de>
To:     Petr Mladek <pmladek@suse.com>
Cc:     Sergey Senozhatsky <senozhatsky@chromium.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        linux-kernel@vger.kernel.org,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Helge Deller <deller@gmx.de>
Subject: Re: [PATCH printk v3 13/15] printk: add kthread console printers
In-Reply-To: <YmBIr1mkmIN1Zkb+@alley>
References: <20220419234637.357112-1-john.ogness@linutronix.de>
 <20220419234637.357112-14-john.ogness@linutronix.de>
 <YmBIr1mkmIN1Zkb+@alley>
Date:   Wed, 20 Apr 2022 22:08:39 +0206
Message-ID: <87h76nwmjk.fsf@jogness.linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

On 2022-04-20, Petr Mladek <pmladek@suse.com> wrote:
> On Wed 2022-04-20 01:52:35, John Ogness wrote:
>> @@ -2280,10 +2295,10 @@ asmlinkage int vprintk_emit(int facility, int level,
>>  	printed_len = vprintk_store(facility, level, dev_info, fmt, args);
>>  
>>  	/* If called from the scheduler, we can not call up(). */
>> -	if (!in_sched) {
>> +	if (!in_sched && allow_direct_printing()) {
>
> allow_direct_printing() is racy here. But I think that we could live
> with it, see below.

Well, it is not racy for its intended purpose, which is a context that
does:

printk_prefer_direct_enter();
printk();
printk_prefer_direct_exit();

It is only racy for _other_ contexts that might end up direct
printing. But since those other contexts don't have a preference, I see
no problem with it.

>> @@ -3524,7 +3774,16 @@ void defer_console_output(void)
>>  	 * New messages may have been added directly to the ringbuffer
>>  	 * using vprintk_store(), so wake any waiters as well.
>>  	 */
>> -	__wake_up_klogd(PRINTK_PENDING_WAKEUP | PRINTK_PENDING_OUTPUT);
>> +	int val = PRINTK_PENDING_WAKEUP;
>> +
>> +	/*
>> +	 * If console deferring was called with preferred direct printing,
>> +	 * make the irqwork perform the direct printing.
>> +	 */
>> +	if (atomic_read(&printk_prefer_direct))
>> +		val |= PRINTK_PENDING_DIRECT_OUTPUT;
>
> We actually need:
>
> 	/*
> 	 * Make sure that someone will handle the messages when direct
> 	 * printing is allowed. It happens when the kthreads are less
> 	 * reliable or unusable at all.
> 	 */
> 	if (allow_direct_printing())
> 		val |= PRINTK_PENDING_DIRECT_OUTPUT;

Agreed. I will update the comments appropriately as well.

> It is racy. But the same race is also in vprintk_emit().

It is not racy for the intended purpose, so I think it is fine.

> False positive is fine. console_flush_all() will bail out when
> the direct printing gets disabled in the meantime.
>
> False negative is worse. But we will still queue PRINTK_PENDING_WAKEUP
> that will try to wake up the kthreads that should still be around.
>
> And it was always problem even with console_trylock() approach.
> Failure means an expectation that someone else is doing the printing.
> It might be either a kthread or the current console_lock owner.
> But it is never guaranteed because both might be sleeping.

By "sleeping" I guess you mean "scheduled out". The console_lock owner
or mutex/atomic_t holder will be within printing code. And if a kthread
sees new records available, it will continue rather than wait.

> We do our best by calling pr_flush() or console_flush_on_panic()
> on various places. Also PRINTK_PENDING_WAKEUP will always try to wake
> up the kthreads.

Yes.

> Anyway, we should document this somewhere. At least in the commit
> message.
>
> My dream is Documentation/core-api/printk-design.rst but I do not
> want to force you to do it ;-)

I would be happy to contribute to such a document.

John