From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757704AbcIGNYL (ORCPT ); Wed, 7 Sep 2016 09:24:11 -0400 Received: from merlin.infradead.org ([205.233.59.134]:47054 "EHLO merlin.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757088AbcIGNYI (ORCPT ); Wed, 7 Sep 2016 09:24:08 -0400 Date: Wed, 7 Sep 2016 15:23:54 +0200 From: Peter Zijlstra To: Nicholas Piggin Cc: Linus Torvalds , Will Deacon , Oleg Nesterov , Paul McKenney , Benjamin Herrenschmidt , Michael Ellerman , linux-kernel@vger.kernel.org, Ingo Molnar , Alan Stern Subject: Re: Question on smp_mb__before_spinlock Message-ID: <20160907132354.GR10138@twins.programming.kicks-ass.net> References: <20160905093753.GN10138@twins.programming.kicks-ass.net> <20160907221726.37981b30@roar.ozlabs.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160907221726.37981b30@roar.ozlabs.ibm.com> User-Agent: Mutt/1.5.23.1 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Sep 07, 2016 at 10:17:26PM +1000, Nicholas Piggin wrote: > > /* > > + * This barrier must provide two things: > > + * > > + * - it must guarantee a STORE before the spin_lock() is ordered against a > > + * LOAD after it, see the comments at its two usage sites. > > + * > > + * - it must ensure the critical section is RCsc. > > + * > > + * The latter is important for cases where we observe values written by other > > + * CPUs in spin-loops, without barriers, while being subject to scheduling. > > + * > > + * CPU0 CPU1 CPU2 > > + * > > + * for (;;) { > > + * if (READ_ONCE(X)) > > + * break; > > + * } > > + * X=1 > > + * > > + * > > + * r = X; > > + * > > + * without transitivity it could be that CPU1 observes X!=0 breaks the loop, > > + * we get migrated and CPU2 sees X==0. > > + * > > + * Since most load-store architectures implement ACQUIRE with an smp_mb() after > > + * the LL/SC loop, they need no further barriers. Similarly all our TSO > > + * architectures imlpy an smp_mb() for each atomic instruction and equally don't > > + * need more. > > + * > > + * Architectures that can implement ACQUIRE better need to take care. > > */ > > +#ifndef smp_mb__after_spinlock > > +#define smp_mb__after_spinlock() do { } while (0) > > #endif > > It seems okay, but why not make it a special sched-only function name > to prevent it being used in generic code? > > I would not mind seeing responsibility for the switch barrier moved to > generic context switch code like this (alternative for powerpc reducing > number of hwsync instructions was to add documentation and warnings about > the barriers in arch dependent and independent code). And pairing it with > a spinlock is reasonable. > > It may not strictly be an "smp_" style of barrier if MMIO accesses are to > be ordered here too, despite critical section may only be providing > acquire/release for cacheable memory, so maybe it's slightly more > complicated than just cacheable RCsc? Interesting idea.. So I'm not a fan of that raw_spin_lock wrapper, since that would end up with a lot more boiler-plate code than just the one extra barrier. But moving MMIO/DMA/TLB etc.. barriers into this spinlock might not be a good idea, since those are typically fairly heavy barriers, and its quite common to call schedule() without ending up in switch_to(). For PowerPC it works out, since there's only SYNC, no other option afaik. But ARM/ARM64 will have to do DSB(ISH) instead of DMB(ISH). IA64 would need to issue "sync.i" and mips-octeon "synciobdma". Will, any idea of the extra cost involved in DSB vs DMB?