From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757596AbcDABGj (ORCPT ); Thu, 31 Mar 2016 21:06:39 -0400 Received: from mail-pa0-f42.google.com ([209.85.220.42]:35535 "EHLO mail-pa0-f42.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753263AbcDABGh (ORCPT ); Thu, 31 Mar 2016 21:06:37 -0400 Date: Fri, 1 Apr 2016 10:08:03 +0900 From: Sergey Senozhatsky To: Petr Mladek Cc: Sergey Senozhatsky , Andrew Morton , Jan Kara , Tejun Heo , Tetsuo Handa , linux-kernel@vger.kernel.org, Byungchul Park , Sergey Senozhatsky , Jan Kara Subject: Re: [RFC][PATCH v8 1/2] printk: Make printk() completely async Message-ID: <20160401010803.GA501@swordfish> References: <1458834203-3392-1-git-send-email-sergey.senozhatsky@gmail.com> <1458834203-3392-2-git-send-email-sergey.senozhatsky@gmail.com> <20160331111229.GB1023@pathway.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160331111229.GB1023@pathway.suse.cz> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hello Petr, On (03/31/16 13:12), Petr Mladek wrote: > > + * Set printing kthread sleep condition early, under the > > + * logbuf_lock, so it (if RUNNING) will go to console_lock() > > + * and spin on logbuf_lock. > > + */ > > + if (!in_panic && printk_kthread && !need_flush_console) > > + need_flush_console = true; > > I would remove the if-statement and always set it: > > + It does not matter if we set it in panic. It will not affect > anything. hm... yes, you're right. > + The check for printk_kthread is racy. It might be false here > and it might be true later when check whether to wakeup > the kthread or try to get console directly. which is fine, isn't it? we will wakeup the printing kthread, it will console_lock()/console_unlock() (regardless the state of need_flush_console). printing thread checks need_flush_console only when it decides whether it shall schedule. > + We might set it true even when it was true before. > > I think that this was an attempt to avoid a spurious wake up. > But we solve it better way now. we also may have 'printk.synchronous = 1', which will purposelessly dirty need_flush_console from various CPUs from every printk /* and upon every return from console_unlock() */; that's why I added both printk_kthread and !need_flush_console (re-dirty already dirtied) checks. > > raw_spin_lock(&logbuf_lock); > > retry = console_seq != log_next_seq; > > + if (!retry && printk_kthread) > > + need_flush_console = false; > > Similar here. I remove the if-statement and always set it. We will > either retry or it should be false anyway. well, 'printk.synchronous = 1'. seems that `!retry' check can be dropped, I agree. a side nano-note, apart from 'printk.synchronous = 1', we also can have !printk_kthread because kthread_run(printk_kthread_func) failed. it's quite unlikely, but still. [..] > > + while (1) { > > + set_current_state(TASK_INTERRUPTIBLE); > > + if (!need_flush_console) > > + schedule(); > > + > > + __set_current_state(TASK_RUNNING); > > > We still must do here: > > need_flush_console = false; oh, wow! that's a major mistake. thanks a lot for catching this! shame on me. > /* > * Avoid an infinite loop when console_unlock() cannot > * access consoles, e.g. because of a suspend. We > * could get asleep here. Someone else will call > * consoles if conditions change. > */ looks ok. > Also another name might help. If we set it false here, the value > does describe a global state. The variable describes if this > kthread needs to flush the console. So, more precise name would be > > printk_kthread_need_flush_console yes, makes sense. > I think that we are close. I really like the current state of > the patch and how minimalistic it is. thanks for your review. I'll re-spin. -ss