From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754945AbcCPB74 (ORCPT ); Tue, 15 Mar 2016 21:59:56 -0400 Received: from mail-pf0-f182.google.com ([209.85.192.182]:35580 "EHLO mail-pf0-f182.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751033AbcCPB7x (ORCPT ); Tue, 15 Mar 2016 21:59:53 -0400 Date: Wed, 16 Mar 2016 11:01:15 +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 v4 1/2] printk: Make printk() completely async Message-ID: <20160316020115.GA3217@swordfish> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160315155849.GW10940@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/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(); > __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