From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751310AbcCFJe1 (ORCPT ); Sun, 6 Mar 2016 04:34:27 -0500 Received: from mail-pf0-f170.google.com ([209.85.192.170]:34200 "EHLO mail-pf0-f170.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751147AbcCFJeK (ORCPT ); Sun, 6 Mar 2016 04:34:10 -0500 Date: Sun, 6 Mar 2016 18:35:30 +0900 From: Sergey Senozhatsky To: Tetsuo Handa Cc: sergey.senozhatsky.work@gmail.com, akpm@linux-foundation.org, jack@suse.com, pmladek@suse.com, tj@kernel.org, linux-kernel@vger.kernel.org, sergey.senozhatsky@gmail.com, jack@suse.cz Subject: Re: [RFC][PATCH v2 1/2] printk: Make printk() completely async Message-ID: <20160306093530.GA26055@swordfish> References: <1457175338-1665-1-git-send-email-sergey.senozhatsky@gmail.com> <1457175338-1665-2-git-send-email-sergey.senozhatsky@gmail.com> <20160306063251.GA493@swordfish> <201603061618.GED43232.MtOQOFSLOFHJFV@I-love.SAKURA.ne.jp> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <201603061618.GED43232.MtOQOFSLOFHJFV@I-love.SAKURA.ne.jp> 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 On (03/06/16 16:18), Tetsuo Handa wrote: > Sergey Senozhatsky wrote: > > printk() is expected to work under different conditions and in different > > scenarios, including corner cases of OOM when all of the workers are busy > > (e.g. allocating memory). Thus by default printk() uses its own dedicated > > workqueue with WQ_MEM_RECLAIM bit set. It falls back to system_long_wq > > (console_unlock() is time unbound) only if it has failed to allocate > > printk_wq. Another thing to mention, is that deferred printk() messages > > may appear before printk_wq is allocated, so in the very beginning we > > have to printk deferred messages the old way -- in IRQ context. > > I think we should not depend on system_long_wq which does not have > WQ_MEM_RECLAIM bit. If workqueue allocation upon boot fails (due to ENOMEM), > such systems won't be able to start userspace programs. well, yes. system_long_wq is a fallback, but probably just BUG_ON(!printk_wq) would be a better thing to do. > Moreover, I don't like use of a workqueue even if printk_wq was allocated > with WQ_MEM_RECLAIM bit. As you can see in the discussion of the OOM reaper, > the OOM reaper chose a dedicated kernel thread rather than a workqueue > ( http://lkml.kernel.org/r/1454505240-23446-2-git-send-email-mhocko@kernel.org ). > > Blocking actual printing until ongoing workqueue item calls schedule_timeout_*() > is not nice (see commit 373ccbe59270 and 564e81a57f97). printk and console_drivers are expected to be 'callable' from any context, including IRQs, or under spin_locks, etc. so neither part of printk->console_unlock()->console_driver->write() can sleep. console_driver->write() is quite often something like this foo_write() { spin_lock_irqsave(&port->lock); uart_console_write( ... foo_put_char); spin_unlock_iqrestore(&port->lock); } and foo_put_char(...), perhaps, waits for device to become ready transaction while (!(UART_GET_STATUS(port) & TXEMPTY)) cpu_relax(); and then sends out the character to the device UART_SET_DATA(port, (unsigned char)ch) so an already executing printk_wq item shouldn't block because of OOM happening on another CPU. do you mean a new worker allocation delay and a MAYDAY timer delay? > Use of WQ_MEM_RECLAIM means we add a task_struct for that workqueue. Thus, using > a kernel thread does not change total number of task_struct compared to WQ_MEM_RECLAIM > approach. I think actual printing should occur as soon as possible rather than randomly > deferred until workqueue item sleeps. hm, need to take a look. > > +static void printing_work_func(struct work_struct *work) > > +{ > > + console_lock(); > > + console_unlock(); > > +} > > Is this safe? If somebody invokes the OOM killer between console_lock() > and console_unlock(), won't this cause OOM killer messages not printed? between console_lock() and console_unlock() nothing can steal that lock from worker; so once a worker grabbed the console_lock() it will print (and console_lock() does not mean "lock logbuf", printk() from other CPUs can add messages to the logbuf while worker loops in console_unlock()). if you mean something like console_lock(); for (...) { do_foo() { ... pr_err(" ... foo message ...\n"); ... } } console_unlock(); then yes, nothing will be printed until that process executes console_unlock(), because it's console_unlock() that pushes the messages to console drivers. (console_lock()/console_unlock() have other problems and issues, which are not addressed in this patch set). there is only one function that ignores the state of console semaphore: panic()->console_flush_on_panic() but that's a separate thing, not related to this patch set. if you mean something else here, then please more details. -ss