From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752683AbcCIGIp (ORCPT ); Wed, 9 Mar 2016 01:08:45 -0500 Received: from mail-pa0-f46.google.com ([209.85.220.46]:36296 "EHLO mail-pa0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752641AbcCIGIb (ORCPT ); Wed, 9 Mar 2016 01:08:31 -0500 Date: Wed, 9 Mar 2016 15:09:50 +0900 From: Sergey Senozhatsky To: Jan Kara Cc: Sergey Senozhatsky , Tetsuo Handa , akpm@linux-foundation.org, jack@suse.com, pmladek@suse.com, tj@kernel.org, linux-kernel@vger.kernel.org, sergey.senozhatsky@gmail.com Subject: Re: [RFC][PATCH v2 1/2] printk: Make printk() completely async Message-ID: <20160309060950.GA666@swordfish> References: <1457175338-1665-2-git-send-email-sergey.senozhatsky@gmail.com> <20160306063251.GA493@swordfish> <201603061618.GED43232.MtOQOFSLOFHJFV@I-love.SAKURA.ne.jp> <20160306093530.GA26055@swordfish> <201603062006.IJD17667.OOQFLtMVHOFSJF@I-love.SAKURA.ne.jp> <20160306132703.GA927@swordfish> <20160307082230.GB5201@quack.suse.cz> <20160307101233.GA10690@swordfish> <20160307105248.GF5201@quack.suse.cz> <20160307121625.GG5201@quack.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160307121625.GG5201@quack.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 Jan, On (03/07/16 13:16), Jan Kara wrote: [..] > > So if this will be a problem in practice, using a kthread will probably be > > the easiest solution. > > Hum, and thinking more about it: Considering that WQ_MEM_RECLAIM workqueues > create kthread anyway as a rescuer thread, it may be the simplest to just > go back to using a single kthread for printing. What do you think? I have this patch on top of the series now. In short, it closes one more possibility of lockups -- console_lock()/console_unlock() calls. the patch splits console_unlock() in two parts: -- the fast one just wake up printing kthread -- the slow one does call_console_drivers() loop I think it sort of makes sense to tweak the patch below a bit and fold it into 0001, and move _some_ of the vprintk_emit() checks to console_unlock(). very schematically, after folding, vprintk_emit() will be if (in_sched) { if (!printk_sync && printk_thread) wake_up() else irq_work_queue() } if (!in_sched) if (console_trylock()) console_unlock() and console_unlock() will be if (!in_panic && !printk_sync && printk_thread) { up_console_sem() wake_up() } else { console_unlock_for_printk() } console_unlock_for_printk() does the call_console_drivers() loop. console_flush_on_panic() and printing_func() call console_unlock_for_printk() directly. What do you think? Or would you prefer to first introduce async printk() rework, and move to console_unlock() in vprintk_emit() one release cycle later? IOW, in 3 steps: -- first make printk() async -- then console_unlock() async, and use console_unlock_for_printk() in vprintk_emit() -- then switch to console_unlock() in vprintk_emit(). below is the patch which introduces console_unlock_for_printk(). not the squashed console_unlock_for_printk() and 0001. -ss ====== >>From bc3932c68c5afb9bf43af98335c705c75067a93a Mon Sep 17 00:00:00 2001 From: Sergey Senozhatsky Subject: [PATCH 3/4] printk: introduce console_unlock_for_printk() Even though we already have asynchronous printk()->vprintk_emit(), there are still good chances to get lockups, because we don't have asynchronous console_unlock(). So any process doing console_lock() and console_unlock() will end up looping in console_unlock(), pushing the messages to console drivers (possibly with IRQs or preemption disabled), regardless the fact that we have a dedicated kthread for that particular job. Apart from that, console_lock()/console_unlock() can be executed by user processes as a part of system calls: a) SyS_open() ... chrdev_open() tty_open() console_device() console_lock() console_unlock() for (;;) { call_console_drivers() } b) SyS_read() ... sysfs_read_file() dev_attr_show() show_cons_active() console_lock() console_unlock() for (;;) { call_console_drivers() } c) doing `cat /proc/consoles` SyS_read() vfs_read() proc_reg_read() seq_read() c_stop() console_unlock() for (;;) { call_console_drivers() } etc. This can add unnecessary latencies to the user space processes. This patch splits console_unlock() in two parts: -- the fast path up() console semaphore and wake up printing kthread (if there is one, of course), otherwise -- the slow path: does what console_unlock() did previously, emit the messages and then up() console semaphore The actual printing loop is, thus, moved to a new function, console_unlock_for_printk(). There are 3 places that unconditionally call it: a) direct printing from vprintk_emit() b) console_flush_on_panic() c) printing kthread callback Signed-off-by: Sergey Senozhatsky --- kernel/printk/printk.c | 51 +++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 44 insertions(+), 7 deletions(-) diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c index de45d86..ddaf62e 100644 --- a/kernel/printk/printk.c +++ b/kernel/printk/printk.c @@ -303,6 +303,8 @@ static struct task_struct *printk_thread; /* Wait for printing wakeups from async vprintk_emit() */ static DECLARE_WAIT_QUEUE_HEAD(printing_wait); +static void console_unlock_for_printk(void); + static int printing_func(void *data) { while (1) { @@ -314,7 +316,7 @@ static int printing_func(void *data) remove_wait_queue(&printing_wait, &wait); console_lock(); - console_unlock(); + console_unlock_for_printk(); } return 0; @@ -1900,7 +1902,7 @@ asmlinkage int vprintk_emit(int facility, int level, * /dev/kmsg and syslog() users. */ if (console_trylock()) - console_unlock(); + console_unlock_for_printk(); lockdep_on(); } @@ -2339,20 +2341,20 @@ out: #define PRINT_MSGS_BEFORE_OOPS 100 /** - * console_unlock - unlock the console system + * console_unlock_for_printk - unlock the console system * * Releases the console_lock which the caller holds on the console system * and the console driver list. * * While the console_lock was held, console output may have been buffered - * by printk(). If this is the case, console_unlock(); emits + * by printk(). If this is the case, console_unlock_for_printk() emits * the output prior to releasing the lock. * * If there is output waiting, we wake /dev/kmsg and syslog() users. * - * console_unlock(); may be called from any context. + * console_unlock_for_printk() may be called from any context. */ -void console_unlock(void) +static void console_unlock_for_printk(void) { static char ext_text[CONSOLE_EXT_LOG_MAX]; static char text[LOG_LINE_MAX + PREFIX_MAX]; @@ -2511,6 +2513,41 @@ skip: if (wake_klogd) wake_up_klogd(); } + + +/** + * console_unlock - unlock the console system + * + * Releases the console_lock which the caller holds on the console system. + * + * The fast path is to wake up the printing kthread (if the system can + * perform asynchronous printing) and return; the slow path is to emit + * the messages directly invoking console_unlock_for_printk(). + * + * console_unlock() may be called from any context. + */ +void console_unlock(void) +{ + bool in_panic = console_loglevel == CONSOLE_LOGLEVEL_MOTORMOUTH; + + if (in_panic) { + /* + * If the system is in panic console_flush_on_panic() issued + * from panic_cpu will flush the messages. + */ + console_locked = 0; + up_console_sem(); + return; + } + + if (!printk_sync && printk_thread) { + console_locked = 0; + up_console_sem(); + wake_up(&printing_wait); + } else { + console_unlock_for_printk(); + } +} EXPORT_SYMBOL(console_unlock); /** @@ -2567,7 +2604,7 @@ void console_flush_on_panic(void) */ console_trylock(); console_may_schedule = 0; - console_unlock(); + console_unlock_for_printk(); } /* -- 2.8.0.rc0.1.gd285ab0