From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752200AbbJGImU (ORCPT ); Wed, 7 Oct 2015 04:42:20 -0400 Received: from mail.efficios.com ([78.47.125.74]:54960 "EHLO mail.efficios.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751684AbbJGImQ (ORCPT ); Wed, 7 Oct 2015 04:42:16 -0400 Date: Wed, 7 Oct 2015 08:42:05 +0000 (UTC) From: Mathieu Desnoyers To: Peter Zijlstra Cc: "Paul E. McKenney" , linux-kernel@vger.kernel.org, Ingo Molnar , Lai Jiangshan , dipankar@in.ibm.com, Andrew Morton , josh@joshtriplett.org, Thomas Gleixner , rostedt , dhowells@redhat.com, edumazet@google.com, dvhart@linux.intel.com, fweisbec@gmail.com, oleg@redhat.com, bobby prani Message-ID: <941567906.21756.1444207325021.JavaMail.zimbra@efficios.com> In-Reply-To: <20151007075114.GW2881@worktop.programming.kicks-ass.net> References: <20151006162907.GA12020@linux.vnet.ibm.com> <1444148977-14108-1-git-send-email-paulmck@linux.vnet.ibm.com> <1444148977-14108-2-git-send-email-paulmck@linux.vnet.ibm.com> <20151006202937.GX3604@twins.programming.kicks-ass.net> <20151006205850.GW3910@linux.vnet.ibm.com> <20151007075114.GW2881@worktop.programming.kicks-ass.net> Subject: Re: [PATCH tip/core/rcu 02/18] rcu: Move rcu_report_exp_rnp() to allow consolidation MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-Originating-IP: [78.47.125.74] X-Mailer: Zimbra 8.6.0_GA_1178 (ZimbraWebClient - FF41 (Linux)/8.6.0_GA_1178) Thread-Topic: Move rcu_report_exp_rnp() to allow consolidation Thread-Index: UNDx14uevyAfnqtaBbxpgzaeXaxhvg== Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org ----- On Oct 7, 2015, at 3:51 AM, Peter Zijlstra peterz@infradead.org wrote: > On Tue, Oct 06, 2015 at 01:58:50PM -0700, Paul E. McKenney wrote: >> On Tue, Oct 06, 2015 at 10:29:37PM +0200, Peter Zijlstra wrote: >> > On Tue, Oct 06, 2015 at 09:29:21AM -0700, Paul E. McKenney wrote: >> > > +static void __maybe_unused rcu_report_exp_rnp(struct rcu_state *rsp, >> > > + struct rcu_node *rnp, bool wake) >> > > +{ >> > > + unsigned long flags; >> > > + unsigned long mask; >> > > + >> > > + raw_spin_lock_irqsave(&rnp->lock, flags); >> > >> > Normally we require a comment with barriers, explaining the order and >> > the pairing etc.. :-) >> > >> > > + smp_mb__after_unlock_lock(); >> >> Hmmmm... That is not good. >> >> Worse yet, I am missing comments on most of the pre-existing barriers >> of this form. > > Yes I noticed.. :/ > >> The purpose is to enforce the heavy-weight grace-period memory-ordering >> guarantees documented in the synchronize_sched() header comment and >> elsewhere. > >> They pair with anything you might use to check for violation >> of these guarantees, or, simiarly, any ordering that you might use when >> relying on these guarantees. > > I'm sure you know what that means, but I've no clue ;-) That is, I > wouldn't know where to start looking in the RCU implementation to verify > the barrier is either needed or sufficient. Unless you mean _everywhere_ > :-) One example is the new membarrier system call. It relies on synchronize_sched() to enforce this: from kernel/membarrier.c: * All memory accesses performed in program order from each targeted thread * is guaranteed to be ordered with respect to sys_membarrier(). If we use * the semantic "barrier()" to represent a compiler barrier forcing memory * accesses to be performed in program order across the barrier, and * smp_mb() to represent explicit memory barriers forcing full memory * ordering across the barrier, we have the following ordering table for * each pair of barrier(), sys_membarrier() and smp_mb(): * * The pair ordering is detailed as (O: ordered, X: not ordered): * * barrier() smp_mb() sys_membarrier() * barrier() X X O * smp_mb() X O O * sys_membarrier() O O O And include/uapi/linux/membarrier.h: * @MEMBARRIER_CMD_SHARED: Execute a memory barrier on all running threads. * Upon return from system call, the caller thread * is ensured that all running threads have passed * through a state where all memory accesses to * user-space addresses match program order between * entry to and return from the system call * (non-running threads are de facto in such a * state). This covers threads from all processes * running on the system. This command returns 0. I hope this sheds light on a userspace-facing interface to synchronize_sched() and clarifies its expected semantic a bit. Thanks, Mathieu > >> I could add something like "/* Enforce GP memory ordering. */" >> >> Or perhaps "/* See synchronize_sched() header. */" >> >> I do not propose reproducing the synchronize_sched() header on each >> of these. That would be verbose, even for me! ;-) >> >> Other thoughts? > > Well, this is an UNLOCK+LOCK on non-matching lock variables upgrade to > full barrier thing, right? > > To me its not clear which UNLOCK we even match here. I've just read the > sync_sched() header, but that doesn't help me either, so referring to > that isn't really helpful either. > > In any case, I don't want to make too big a fuzz here, but I just > stumbled over a lot of unannotated barriers and figured I ought to say > something about it. -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com