From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965324AbcJYB4C (ORCPT ); Mon, 24 Oct 2016 21:56:02 -0400 Received: from mail-pf0-f196.google.com ([209.85.192.196]:34183 "EHLO mail-pf0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754688AbcJYBz6 (ORCPT ); Mon, 24 Oct 2016 21:55:58 -0400 Date: Tue, 25 Oct 2016 10:55:54 +0900 From: Sergey Senozhatsky To: Linus Torvalds Cc: Sergey Senozhatsky , Joe Perches , Geert Uytterhoeven , Tetsuo Handa , Linux Kernel Mailing List , Petr Mladek , Sergey Senozhatsky , Tejun Heo , Calvin Owens , Steven Rostedt , Andrew Morton Subject: Re: linux.git: printk() problem Message-ID: <20161025015554.GA495@swordfish> References: <201610122230.DID43237.FSOHFFQOJOtVML@I-love.SAKURA.ne.jp> <1477249607.3561.2.camel@perches.com> <20161024140845.GA626@swordfish> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.7.1 (2016-10-04) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hello, Cc more people report: https://marc.info/?l=linux-kernel&m=147721454506634&w=2 patch: https://marc.info/?l=linux-kernel&m=147733173800859 FB is using ext header a lot (afaik), so may be Tejun or Calvin will also be interested. On (10/24/16 10:55), Linus Torvalds wrote: > > Note the "totally untested" part. It compiles for me. But it may do > > unspeakably stupid things. Caveat applior. > > Well, it is hard to apply a patch that I didn't even attach. Blush. [..] > -static void cont_flush(void) > +static bool cont_flush(void) > { > - if (cont.flushed) > - return; > - if (cont.len == 0) > + if (!cont.len) > + return false; > + > + log_store(cont.facility, cont.level, cont.flags, cont.ts_nsec, > + NULL, 0, cont.buf, cont.len); > + cont.len = 0; > + return true; > +} I think cont_flush() should grab the logbuf_lock lock, because it does log_store() and touches the cont.len. so something like this perhaps diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c index c7f490f..47f887c 100644 --- a/kernel/printk/printk.c +++ b/kernel/printk/printk.c @@ -1608,13 +1608,20 @@ static struct cont { static bool cont_flush(void) { + unsigned long flags; + bool flushed = false; + + raw_spin_lock_irqsave(&logbuf_lock, flags); if (!cont.len) - return false; + goto out; log_store(cont.facility, cont.level, cont.flags, cont.ts_nsec, NULL, 0, cont.buf, cont.len); cont.len = 0; - return true; + flushed = true; +out: + raw_spin_unlock_irqrestore(&logbuf_lock, flags); + return flushed; } [..] > @@ -2449,7 +2311,6 @@ void console_unlock(void) > } > console_idx = log_next(console_idx); > console_seq++; > - console_prev = msg->flags; > raw_spin_unlock(&logbuf_lock); > > stop_critical_timings(); /* don't trace print latency */ > @@ -2483,7 +2344,7 @@ void console_unlock(void) > if (retry && console_trylock()) > goto again; > > - if (wake_klogd) > + if (wake_klogd || cont.len) ^^^^^^^^^^ this _technically_ can result in additional spurious wakeups - cont.len check is done outside of console_sem && logbuf_lock - but I don't think this is a huge problem. -ss