From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753494AbdIYE4k (ORCPT ); Mon, 25 Sep 2017 00:56:40 -0400 Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:36296 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752998AbdIYE4j (ORCPT ); Mon, 25 Sep 2017 00:56:39 -0400 Date: Sun, 24 Sep 2017 21:56:32 -0700 From: "Paul E. McKenney" To: Steven Rostedt Cc: Linus Torvalds , Linux Kernel Mailing List , Ingo Molnar , Andrew Morton , stable Subject: Re: [PATCH 1/4] rcu: Allow for page faults in NMI handlers Reply-To: paulmck@linux.vnet.ibm.com References: <20170923205621.811805556@goodmis.org> <20170923205804.098759069@goodmis.org> <20170925000303.GJ3521@linux.vnet.ibm.com> <20170925002653.GL3521@linux.vnet.ibm.com> <20170925003456.GA13412@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) X-TM-AS-GCONF: 00 x-cbid: 17092504-0044-0000-0000-00000394E85C X-IBM-SpamModules-Scores: X-IBM-SpamModules-Versions: BY=3.00007788; HX=3.00000241; KW=3.00000007; PH=3.00000004; SC=3.00000231; SDB=6.00921985; UDB=6.00463366; IPR=6.00702127; BA=6.00005602; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009; ZB=6.00000000; ZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00017261; XFM=3.00000015; UTC=2017-09-25 04:56:35 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17092504-0045-0000-0000-000007C3EF02 Message-Id: <20170925045632.GM3521@linux.vnet.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-09-24_07:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=0 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1707230000 definitions=main-1709250077 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Sep 25, 2017 at 06:41:30AM +0200, Steven Rostedt wrote: > Sorry for the top post, currently on a train to Paris. > > This series already went through all my testing, and I would hate to rebase it for this reason. Can you just add a patch to remove the READ_ONCE()s? If Linus accepts the original series, easy enough. Thanx, Paul > Thanks, > > -- Steve > > > On September 25, 2017 2:34:56 AM GMT+02:00, "Paul E. McKenney" wrote: > >On Sun, Sep 24, 2017 at 05:26:53PM -0700, Paul E. McKenney wrote: > >> On Sun, Sep 24, 2017 at 05:12:13PM -0700, Linus Torvalds wrote: > >> > On Sun, Sep 24, 2017 at 5:03 PM, Paul E. McKenney > >> > wrote: > >> > > > >> > > Mostly just paranoia on my part. I would be happy to remove it > >if > >> > > you prefer. Or you or Steve can do so if that is more > >convenient. > >> > > >> > I really don't think it's warranted. The values are *stable*. > >There's > >> > no subtle lack of locking, or some optimistic access to a value > >that > >> > can change. > >> > > >> > The compiler can generate code to read the value fifteen billion > >> > times, and it will always get the same value. > >> > > >> > Yes, maybe in between the different accesses, an NMI will happen, > >and > >> > the value will be incremented, but then as the NMI exits, it will > >> > decrement again, so the code that got interrupted will not actually > >> > see the change. > >> > > >> > So the READ_ONCE() isn't "paranoia". It's just confusing. > >> > > >> > > And yes, consistency would dictate that the uses in > >rcu_nmi_enter() > >> > > and rcu_nmi_exit() should be _ONCE(), particularly the stores to > >> > > ->dynticks_nmi_nesting. > >> > > >> > NO. > >> > > >> > That would be just more of that confusion. > >> > > >> > That value is STABLE. It's stable even within an NMI handler. The > >NMI > >> > code can read it, modify it, write it back, do a little dance, all > >> > without having to care. There's no "_ONCE()" about it - not for the > >> > readers, not for the writers, not for _anybody_. > >> > > >> > So adding even more READ/WRITE_ONCE() accesses wouldn't be > >> > "consistent", it would just be insanity. > >> > > >> > Now, if an NMI happens and the value would be different on entry > >than > >> > it is on exit, that would be something else. Then it really > >wouldn't > >> > be stable wrt random users. But that would also be a major bug in > >the > >> > NMI handler, as far as I can tell. > >> > > >> > So the reason I'm objecting to that READ_ONCE() is that it isn't > >> > "paranoia", it's "voodoo programming". And we don't do voodoo > >> > programming. > >> > >> I already agreed that the READ_ONCE() can be removed. > > > >And for whatever it is worth, here is the updated patch. > > > > Thanx, Paul > > > >------------------------------------------------------------------------ > > > >commit 3e2baa988b9c13095995c46c51e0e32c0b6a7d43 > >Author: Paul E. McKenney > >Date: Fri Sep 22 13:14:42 2017 -0700 > > > > rcu: Allow for page faults in NMI handlers > > > > A number of architecture invoke rcu_irq_enter() on exception entry in > >order to allow RCU read-side critical sections in the exception handler > > when the exception is from an idle or nohz_full CPU. This works, at > > least unless the exception happens in an NMI handler. In that case, > >rcu_nmi_enter() would already have exited the extended quiescent state, > > which would mean that rcu_irq_enter() would (incorrectly) cause RCU > > to think that it is again in an extended quiescent state. This will > > in turn result in lockdep splats in response to later RCU read-side > > critical sections. > > > > This commit therefore causes rcu_irq_enter() and rcu_irq_exit() to > > take no action if there is an rcu_nmi_enter() in effect, thus avoiding > > the unscheduled return to RCU quiescent state. This in turn should > > make the kernel safe for on-demand RCU voyeurism. > > > > Reported-by: Steven Rostedt > > Signed-off-by: Paul E. McKenney > > [ paulmck: Remove READ_ONCE() per Linux Torvalds feedback. ] > > > >diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > >index db5eb8c3f7af..e4fe06d42385 100644 > >--- a/kernel/rcu/tree.c > >+++ b/kernel/rcu/tree.c > >@@ -891,6 +891,11 @@ void rcu_irq_exit(void) > > > > RCU_LOCKDEP_WARN(!irqs_disabled(), "rcu_irq_exit() invoked with irqs > >enabled!!!"); > > rdtp = this_cpu_ptr(&rcu_dynticks); > >+ > >+ /* Page faults can happen in NMI handlers, so check... */ > >+ if (rdtp->dynticks_nmi_nesting) > >+ return; > >+ > > WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) && > > rdtp->dynticks_nesting < 1); > > if (rdtp->dynticks_nesting <= 1) { > >@@ -1036,6 +1041,11 @@ void rcu_irq_enter(void) > > > > RCU_LOCKDEP_WARN(!irqs_disabled(), "rcu_irq_enter() invoked with irqs > >enabled!!!"); > > rdtp = this_cpu_ptr(&rcu_dynticks); > >+ > >+ /* Page faults can happen in NMI handlers, so check... */ > >+ if (rdtp->dynticks_nmi_nesting) > >+ return; > >+ > > oldval = rdtp->dynticks_nesting; > > rdtp->dynticks_nesting++; > > WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) && > > -- > Sent from my Android device with K-9 Mail. Please excuse my brevity. >