From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753176AbcDAI7T (ORCPT ); Fri, 1 Apr 2016 04:59:19 -0400 Received: from mx2.suse.de ([195.135.220.15]:40260 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750701AbcDAI7P (ORCPT ); Fri, 1 Apr 2016 04:59:15 -0400 Date: Fri, 1 Apr 2016 10:59:12 +0200 From: Petr Mladek To: Sergey Senozhatsky Cc: Sergey Senozhatsky , Andrew Morton , Jan Kara , Tejun Heo , Tetsuo Handa , linux-kernel@vger.kernel.org, Byungchul Park , Jan Kara Subject: Re: [RFC][PATCH v8 1/2] printk: Make printk() completely async Message-ID: <20160401085912.GO5522@pathway.suse.cz> 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> <20160401010803.GA501@swordfish> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160401010803.GA501@swordfish> 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 Fri 2016-04-01 10:08:03, Sergey Senozhatsky wrote: > 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. You almost persuaded me :-) But what about this? CPU0 CPU1 printk() if (printk_kthread) # fails and need_flush_console # stays false init_printk_kthread() # put printk_thread into # run queue printk_kthread = ...; if (!in_panic && printk_kthread) wake_up_process(printk_kthread); # printk kthread finally gets # scheduled printk_kthread_func() set_current_state(TASK_INTERRUPTIBLE); if (!need_flush_console) schedule(); => printk_kthread is happily sleeping without calling console. We would rearrange printk_kthread_func() to call console first before going to sleep. But I feel that it still might be racy some other way because the operations are not synchronized. For example, printk_kthread_func() might get scheduled on yet another CPU and might go call consoles before printk_kthread gets assigned but it might get rescheduled before it goes into a sleep on preemptive kernel. I think that it does not matter how need_flush_console if the kthread is not running. The only drawback is that it will most likely call consoles once without any real work but it is only once on start up and we might need it anyway to handle the above race. > > + 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. I do not see any code that will modify need_flush_console when printk.synchronous is modified at runtime. If it is set during boot the kthread will never run and it does not matter how need_console_flush is set. I know that all this is rather theoretical. My main point is to remove unnecessary checks that make the code harder to read and does not bring any big advantage. > > > 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. If the kthread does not run, it does not matter how need_flush_console is set. It is similar to the above check. It adds extra logic to the code that does not bring any big win. Sigh, I never know when it is the right time to stop wrangling about details. Well, the easier code helps me to see real problems. Best Regards, Petr