From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755026AbcCPCLC (ORCPT ); Tue, 15 Mar 2016 22:11:02 -0400 Received: from LGEAMRELO12.lge.com ([156.147.23.52]:52207 "EHLO lgeamrelo12.lge.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753268AbcCPCLA (ORCPT ); Tue, 15 Mar 2016 22:11:00 -0400 X-Original-SENDERIP: 156.147.1.151 X-Original-MAILFROM: byungchul.park@lge.com X-Original-SENDERIP: 10.177.222.33 X-Original-MAILFROM: byungchul.park@lge.com Date: Wed, 16 Mar 2016 11:10:49 +0900 From: Byungchul Park To: Sergey Senozhatsky Cc: Petr Mladek , Sergey Senozhatsky , Andrew Morton , Jan Kara , Tejun Heo , Tetsuo Handa , linux-kernel@vger.kernel.org, Jan Kara Subject: Re: [RFC][PATCH v4 1/2] printk: Make printk() completely async Message-ID: <20160316021049.GI5220@X58A-UD3R> References: <1457964820-4642-1-git-send-email-sergey.senozhatsky@gmail.com> <1457964820-4642-2-git-send-email-sergey.senozhatsky@gmail.com> <20160315155849.GW10940@pathway.suse.cz> <20160316020115.GA3217@swordfish> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160316020115.GA3217@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 Wed, Mar 16, 2016 at 11:01:15AM +0900, Sergey Senozhatsky wrote: > Hello Petr, > > On (03/15/16 16:58), Petr Mladek wrote: > [..] > > > +static bool __read_mostly printk_sync = !IS_ENABLED(CONFIG_SMP); > > > +module_param_named(synchronous, printk_sync, bool, S_IRUGO | S_IWUSR); > > > > If we make it writtable, we also need to handle the situation that > > it gets disabled at runtime. It means to make sure that the kthread > > will be running event printk_sync was set during the boot. > > yes, I just thought this morning that may be disabling 'write' here would > be ok. > > > What about this? > > > > int need_flush_console; > > > > while(1) { > > set_current_state(TASK_INTERRUPTIBLE); > > if (!need_flush_console) > > schedule(); else // This shoule be here, IMHO. Hm? > > __set_current_state(TASK_RUNNING); > > > > need_flush_console = false; > > > > > + console_lock(); > > > + console_unlock(); > > > + } > > much better, indeed. > I assume `need_flush_console' is primarily for avoiding schedule() cost? > not that it closes the race window 100%, it can be false at the time we > check it, and become true by the time we schedule(). TASK_INTERRUPTIBLE > should prevent lost wake_up() case, AFAIK. > > > Also I wonder if we need some special handling of the system freezing > > but I do not thing so. > > hm, I don't think so either. > > > > + printk_thread = kthread_run(printing_func, NULL, "printk"); > > > + BUG_ON(IS_ERR(printk_thread)); > > > > I would prefer to force the synchronous mode instead. > > ok, no strong opinion here, I thought that if the system can't create > a kthread in late_initcall(), then it probably doesn't have many chances > to survive anyway. > > > > + * Delayed printk version, for scheduler-internal messages: > > > > This is not longer related to sheduler only. > > this has changed. KTHREAD/IRQ split is not needed anymore, please > see below. > > > BTW: I suggest to move this whole section in a separate patch. > > It will be more clear what has changed for the async printk > > and what stays for the deferred printk. > > hm, sounds good. > > > > if (pending & PRINTK_PENDING_CONSOLE_OUTPUT) { > > if (printk_sync || !printk_kthread) { > > /* If trylock fails, someone else is doing the printing */ > > if (console_trylock()) > > console_unlock(); > > } else { > > wake_up_process(printk_kthread); > > } > > > > if (pending & PRINTK_PENDING_KLOGD_WAKEUP) > > wake_up_interruptible(&log_wait); > > yes, agree. this is what I have here: > http://marc.info/?l=linux-kernel&m=145805101825604 > > > > + bool in_panic = console_loglevel == CONSOLE_LOGLEVEL_MOTORMOUTH; > > > + bool sync_print = printk_sync; > > > > I would force the global printk_sync if we are in_panic > > > > if (in_panic) > > printk_sync = true; > > can add, yes. > > > > - /* If called from the scheduler, we can not call up(). */ > > > - if (!in_sched) { > > > + if (sync_print) { > > > lockdep_off(); > > > > I wonder if it might be much easier with If we used only the two > > PRINTK_PENDING flags and force global printk_sync when in panic. > > two PENDING flags stuff was my bad. (I replied here > http://marc.info/?l=linux-kernel&m=145805101825604) > > in short, my intention was to move it out of that part of vprintk_emit() that > can recurse, but cannot detect the recursion. wake_up()/wake_up_process() > add spin_locks/etc., which add possibilities of > vprint_emit()->spin_lock()->spin_dump()->vprintk_emit()->... > that will not be handled by vprintk_emit() recursion detection code. but > I guess I simply want to move this under the logbuf lock section after all, > so printk recursion detection will have better chances to help us out. > > > Sigh, it would be great to rename also wake_up_klogd_work and > > wake_up_klogd_work_func(). They are not only about klogd. > > Well, this should be separate patch as well because it > > was even before. > > hm, yes, as a separate patch later I think. > > > I still to thing about possible races. Especially, when checking > > printk_kthread and printk_sync. > > hm, I don't think we risk anything here. if CPU saw an 'old' (NULL) @printk_kthread > then it just would do direct printk. once it's !NULL, we can wake it up. > is your concern here that `pointer = VALUE' can be !atomic? > > > I hope that some of the above suggestions makes sense. vprintk_emit() > > is crazy already now. I feel motivated to do not make it worse ;-) > > thanks for review. > > -ss