From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751696AbdCQMUz (ORCPT ); Fri, 17 Mar 2017 08:20:55 -0400 Received: from mx2.suse.de ([195.135.220.15]:47724 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751629AbdCQMUu (ORCPT ); Fri, 17 Mar 2017 08:20:50 -0400 Date: Fri, 17 Mar 2017 13:19:44 +0100 From: Petr Mladek To: Sergey Senozhatsky Cc: Steven Rostedt , Andrew Morton , Linus Torvalds , Peter Zijlstra , "Rafael J . Wysocki" , linux-kernel@vger.kernel.org, Sergey Senozhatsky Subject: Re: [RFC][PATCH 2/4] printk: offload printing from wake_up_klogd_work_func() Message-ID: <20170317121944.GO3977@pathway.suse.cz> References: <20170306124554.828-1-sergey.senozhatsky@gmail.com> <20170306124554.828-3-sergey.senozhatsky@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170306124554.828-3-sergey.senozhatsky@gmail.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon 2017-03-06 21:45:52, Sergey Senozhatsky wrote: > Offload printing of printk_deferred() messages from IRQ context > to a schedulable printing kthread, when possible (the same way > we do it in vprintk_emit()). Otherwise, console_unlock() can > force the printing CPU to spend unbound amount of time flushing > kernel messages from IRQ context. > > Signed-off-by: Sergey Senozhatsky > --- > kernel/printk/printk.c | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c > index 1c4232ca2e6a..6e00073a7331 100644 > --- a/kernel/printk/printk.c > +++ b/kernel/printk/printk.c > @@ -2735,9 +2735,16 @@ static void wake_up_klogd_work_func(struct irq_work *irq_work) > int pending = __this_cpu_xchg(printk_pending, 0); > > if (pending & PRINTK_PENDING_OUTPUT) { > - /* If trylock fails, someone else is doing the printing */ > - if (console_trylock()) > - console_unlock(); > + if (printk_kthread_enabled()) { > + wake_up_process(printk_kthread); I have just noticed a possible race. printk_deferred() does not set printk_kthread_need_flush_console and there might stay a pending job: CPU0 CPU1 printk_kthread_func() printk_kthread_need_flush_console = false; console_lock() console_unlock() printk_deferred() vprintk_emit() irq_work_queue() wake_up_klogd_work_func() if (printk_kthread_enabled()) wake_up_process(printk_kthread); set_current_state(TASK_INTERRUPTIBLE); if (!printk_kthread_need_flush_console) schedule(); Result: printk_kthread goes to sleep even though there is a pending job. A solution might be to rename the variable to something like printk_pending_output, always set it in vprintk_emit() and clear it in console_unlock() when there are no pending messages. I think that we have already discussed this in the past. This solution would also remove one extra cycle if more messages are handled by one console_unlock() call: CPU0 CPU1 printk() vprintk_emit() printk_kthread_need_flush_console = true; wake_up_process(printk_kthread) printk_kthread_need_flush_console = false; console_lock() printk() vprintk_emit() printk_kthread_need_flush_console = true; wake_up_process(printk_kthread) console_unlock() set_current_state(TASK_INTERRUPTIBLE); if (!printk_kthread_need_flush_console) _set_current_state(TASK_RUNNING); console_lock() console_unlock() Result: The second console_unlock() has nothing to do. If I remember correctly, you were not much happy with this solution because it did spread the logic. I think that you did not believe that it was worth fixing the second problem. But fixing the race might need to spread the logic as well. I see it the following way. vprintk_emit() is a producer, console_unlock() is a consumer, and printk_thread is a room that allows consumer to do its job. The consumer has more rooms available. The state variable is a flag showing that there is a pending job, consumer is looking for a room, and printk_kthread should offer it. Of course, it is possible that you will find a better solution. Best Regards, Petr