From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757397AbcHZB5D (ORCPT ); Thu, 25 Aug 2016 21:57:03 -0400 Received: from mail-pf0-f196.google.com ([209.85.192.196]:32871 "EHLO mail-pf0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752354AbcHZB5B (ORCPT ); Thu, 25 Aug 2016 21:57:01 -0400 Date: Fri, 26 Aug 2016 10:56:41 +0900 From: Sergey Senozhatsky To: Petr Mladek Cc: Sergey Senozhatsky , 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: <20160826015641.GA520@swordfish> References: <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> <20160825210959.GA2273@dhcp128.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160825210959.GA2273@dhcp128.suse.cz> User-Agent: Mutt/1.7.0 (2016-08-17) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On (08/25/16 23:10), Petr Mladek wrote: [..] > 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. you didn't miss anything, I think I wasn't too descriptive and that caused some confusion. this patch is not a replacement of wake_up_process() patch posted earlier in the loop, but an addition to it. not only every WARN/BUG issued from wake_up_process() will do no good, but every lock we take is potentially dangerous as well. In the simplest case because of $LOCK-debug.c files in kernel/locking (spin_lock in our case); in the worst case -- because of WARNs issued by log_store() and friends (there may be custom modifications) or by violations of spinlock atomicity requirements. For example, vprintk_emit() local_irq_save() raw_spin_lock() text_len = vscnprintf(text, sizeof(textbuf), fmt, args) { vsnprintf() { if (WARN_ON_ONCE(size > INT_MAX)) return 0; } } ... this is a rather unlikely event, sure, there must be some sort of memory corruption or something else, but the thing is -- if it will happen, printk() will not be willing to help. wake_up_process() change, posted earlier, is using a deferred version of WARN macro, but we definitely can (and we better do) switch to lockless alternative printk() in both cases and don't bother with new macros. replacing all of the existing ones with 'safe' deferred versions is a difficult task, but keeping track of a newly introduced ones is even harder (if possible at all). > +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 { this can do, thanks. I would probably prefer, for the time being, to have a single mechanism that we will use in both cases. something like this: vprintk_emit() { alt_printk_enter(); ... log_store(); ... alt_printk_exit(); wakep_up_process() /* direct from async printk, or indirect from console_unlock()->up() */ alt_printk_enter(); ... enqueue task alt_printk_exit(); } and we need to have some sort of rollback to default printk() if BUG() goes to panic() (both on HAVE_ARCH_BUG and !HAVE_ARCH_BUG platforms): static void oops_end(...) { ... if (in_interrupt()) panic("Fatal exception in interrupt"); if (panic_on_oops) panic("Fatal exception"); if (signr) do_exit(signr); } not so sure about do_exit(). we are specifically talking here about wake_up_process()->try_to_wake_up(), which does all of its job under raw_spin_lock_irqsave(&p->pi_lock), so IF there is a BUG() that does do_exit() /* hard to imagine that */, then nothing will help us out, I think. -ss