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
Subject: Re: [PATCH printk v1 03/18] printk: Consolidate console deferred
 printing
In-Reply-To: <ZAiKhAA37/jehmD7@alley>
References: <20230302195618.156940-1-john.ogness@linutronix.de>
 <20230302195618.156940-4-john.ogness@linutronix.de>
 <ZAiKhAA37/jehmD7@alley>
Date:   Fri, 17 Mar 2023 14:11:01 +0106
Message-ID: <871qln7cle.fsf@jogness.linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

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."

> Anyway, I like this change.

Thanks.

John