From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756599AbcHYVXr (ORCPT ); Thu, 25 Aug 2016 17:23:47 -0400 Received: from mx2.suse.de ([195.135.220.15]:50290 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753962AbcHYVXo (ORCPT ); Thu, 25 Aug 2016 17:23:44 -0400 Date: Thu, 25 Aug 2016 23:10:00 +0200 From: Petr Mladek To: Sergey Senozhatsky Cc: Jan Kara , Sergey Senozhatsky , Viresh Kumar , Andrew Morton , Jan Kara , Tejun Heo , Tetsuo Handa , "linux-kernel@vger.kernel.org" , Byungchul Park , vlevenetz@mm-sol.com, Greg Kroah-Hartman Subject: Re: [PATCH v10 1/2] printk: Make printk() completely async Message-ID: <20160825210959.GA2273@dhcp128.suse.cz> References: <20160812094447.GD7339@pathway.suse.cz> <20160818022712.GB500@swordfish> <20160818093329.GL13300@pathway.suse.cz> <20160818095144.GA425@swordfish> <20160818105629.GE26194@pathway.suse.cz> <20160819063236.GA584@swordfish> <20160819095455.GR13300@pathway.suse.cz> <20160819190007.GA8275@quack2.suse.cz> <20160820052430.GA695@swordfish> <20160822041520.GA511@swordfish> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160822041520.GA511@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 Mon 2016-08-22 13:15:20, Sergey Senozhatsky wrote: > Hello, > > On (08/20/16 14:24), Sergey Senozhatsky wrote: > > On (08/19/16 21:00), Jan Kara wrote: > > > > > depending on .config BUG() may never return back -- passing control > > > > > to do_exit(), so printk_deferred_exit() won't be executed. thus we > > > > > probably need to have a per-cpu variable that would indicate that > > > > > we are in deferred_bug. hm... but do we really need deferred BUG() > > > > > in the first place? > > > > > since we are basically interested in wake_up_process() only from > printk() POV. not sure how acceptable 2 * preempt_count and 2 * per-CPU > writes for every try_to_wake_up(). > > > the other thing I just thought of is doing something as follows > !!!not tested, will not compile, just an idea!!! > > --- > > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c > index 6e260a0..bb8d719 100644 > --- a/kernel/printk/printk.c > +++ b/kernel/printk/printk.c > @@ -1789,6 +1789,7 @@ asmlinkage int vprintk_emit(int facility, int level, > printk_delay(); > > local_irq_save(flags); > + printk_nmi_enter(); > this_cpu = smp_processor_id(); > > /* > @@ -1804,6 +1805,7 @@ asmlinkage int vprintk_emit(int facility, int level, > */ > if (!oops_in_progress && !lockdep_recursing(current)) { > recursion_bug = true; > + printk_nmi_exit(); > local_irq_restore(flags); > return 0; > } > @@ -1920,6 +1922,7 @@ asmlinkage int vprintk_emit(int facility, int level, > logbuf_cpu = UINT_MAX; > raw_spin_unlock(&logbuf_lock); > lockdep_on(); > + printk_nmi_exit(); > local_irq_restore(flags); > > /* If called from the scheduler, we can not call up(). */ I was so taken by the idea of temporary forcing a lockless and "trivial" printk implementation that I missed one thing. Your patch use the alternative printk() variant around logbuf_lock. But this is not the problem with wake_up_process(). printk_deferred() takes logbuf_lock without problems. Our problem is with calling wake_up_process() recursively. The deadlock is in the scheduler locks. But the patch still inspired me. What about blocking the problematic wake_up_process() call by a per-cpu variable. I mean something like this completely untested code: diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c index ca9733b802ce..93915eb1fd0d 100644 --- a/kernel/printk/printk.c +++ b/kernel/printk/printk.c @@ -1708,6 +1708,8 @@ static size_t cont_print_text(char *text, size_t size) return textlen; } +DEFINE_PER_CPU(bool, printk_wakeup); + asmlinkage int vprintk_emit(int facility, int level, const char *dict, size_t dictlen, const char *fmt, va_list args) @@ -1902,8 +1904,17 @@ asmlinkage int vprintk_emit(int facility, int level, lockdep_off(); if (printk_kthread && !in_panic) { + bool __percpu *printk_wakeup_ptr; + /* Offload printing to a schedulable context. */ - wake_up_process(printk_kthread); + local_irq_save(flags); + printk_wake_up_ptr = this_cpu_ptr(&printk_wake_up); + if (!*printk_wakeup_ptr) { + *printk_wake_up_ptr = true; + wake_up_process(printk_kthread); + *printk_wake_up_ptr = false; + } + local_irq_restore(flags); goto out_lockdep; } else { /* We might eventually hide this into a wake_up_process_safe() or so. Also we might need to use it also in console_unlock() to avoid similar recursion there as well. Best Regards, Petr