| Message ID | 1451572003-2440-21-git-send-email-mst@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Dec 31, 2015 at 09:08:22PM +0200, Michael S. Tsirkin wrote: > +#ifdef CONFIG_SMP > +#define fence() metag_fence() > +#else > +#define fence() do { } while (0) > #endif James, it strikes me as odd that fence() is a no-op instead of a barrier() for UP, can you verify/explain?
Hi Peter, On Mon, Jan 04, 2016 at 02:41:28PM +0100, Peter Zijlstra wrote: > On Thu, Dec 31, 2015 at 09:08:22PM +0200, Michael S. Tsirkin wrote: > > +#ifdef CONFIG_SMP > > +#define fence() metag_fence() > > +#else > > +#define fence() do { } while (0) > > #endif > > James, it strikes me as odd that fence() is a no-op instead of a > barrier() for UP, can you verify/explain? fence() is an unfortunate workaround for a specific issue on a certain SoC, where writes from different hw threads get reordered outside of the core, resulting in incoherency between RAM and cache. It has slightly different semantics to the normal SMP barriers, since I was assured it is required before a write rather than after it. Here's the comment: > This is needed before a write to shared memory in a critical section, > to prevent external reordering of writes before the fence on other > threads with writes after the fence on this thread (and to prevent the > ensuing cache-memory incoherence). It is therefore ineffective if used > after and on the same thread as a write. It is used along with the metag specific __global_lock1() (global voluntary lock between hw threads) whenever a write is performed, and by smp_mb/smp_rmb to try to catch other cases, but I've never been confident this fixes every single corner case, since there could be other places where multiple CPUs perform unsynchronised writes to the same memory location, and expect cache not to become incoherent at that location. It seemed to be sufficient to achieve stability however, and SMP on Meta Linux never made it into a product anyway, since the other hw thread tended to be used for RTOS stuff, so it didn't seem worth extending the generic barrier API for it. Cheers James
On Mon, Jan 04, 2016 at 03:25:58PM +0000, James Hogan wrote: > It is used along with the metag specific __global_lock1() (global > voluntary lock between hw threads) whenever a write is performed, and by > smp_mb/smp_rmb to try to catch other cases, but I've never been > confident this fixes every single corner case, since there could be > other places where multiple CPUs perform unsynchronised writes to the > same memory location, and expect cache not to become incoherent at that > location. Ah, yuck, I thought blackfin was the only one attempting !coherent SMP. And yes, this is bound to break in lots of places in subtle ways. We very much assume cache coherency for SMP in generic code. > It seemed to be sufficient to achieve stability however, and SMP on Meta > Linux never made it into a product anyway, since the other hw thread > tended to be used for RTOS stuff, so it didn't seem worth extending the > generic barrier API for it. *phew*, should we take it out then, just to be sure nobody accidentally tries to use it then?
On Mon, Jan 04, 2016 at 04:30:36PM +0100, Peter Zijlstra wrote: > On Mon, Jan 04, 2016 at 03:25:58PM +0000, James Hogan wrote: > > It is used along with the metag specific __global_lock1() (global > > voluntary lock between hw threads) whenever a write is performed, and by > > smp_mb/smp_rmb to try to catch other cases, but I've never been > > confident this fixes every single corner case, since there could be > > other places where multiple CPUs perform unsynchronised writes to the > > same memory location, and expect cache not to become incoherent at that > > location. > > Ah, yuck, I thought blackfin was the only one attempting !coherent SMP. > And yes, this is bound to break in lots of places in subtle ways. We > very much assume cache coherency for SMP in generic code. Well, its usually completely coherent, its just a bit dodgy in a particular hardware corner case, which was pretty hard to hit, even without these workarounds. > > > It seemed to be sufficient to achieve stability however, and SMP on Meta > > Linux never made it into a product anyway, since the other hw thread > > tended to be used for RTOS stuff, so it didn't seem worth extending the > > generic barrier API for it. > > *phew*, should we take it out then, just to be sure nobody accidentally > tries to use it then? SMP support on this SoC you mean? I doubt it'll be a problem tbh, and it'd work fine in QEMU when emulating this SoC, so I'd prefer to keep it in. Cheers James
Hi Michael, On Thu, Dec 31, 2015 at 09:08:22PM +0200, Michael S. Tsirkin wrote: > This defines __smp_xxx barriers for metag, > for use by virtualization. > > smp_xxx barriers are removed as they are > defined correctly by asm-generic/barriers.h > > Note: as __smp_XX macros should not depend on CONFIG_SMP, they can not > use the existing fence() macro since that is defined differently between > SMP and !SMP. For this reason, this patch introduces a wrapper > metag_fence() that doesn't depend on CONFIG_SMP. > fence() is then defined using that, depending on CONFIG_SMP. I'm not a fan of the inconsistent commit message wrapping. I wrap to 72 columns (although I now notice SubmittingPatches says to use 75...). > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > Acked-by: Arnd Bergmann <arnd@arndb.de> > --- > arch/metag/include/asm/barrier.h | 32 +++++++++++++++----------------- > 1 file changed, 15 insertions(+), 17 deletions(-) > > diff --git a/arch/metag/include/asm/barrier.h b/arch/metag/include/asm/barrier.h > index b5b778b..84880c9 100644 > --- a/arch/metag/include/asm/barrier.h > +++ b/arch/metag/include/asm/barrier.h > @@ -44,13 +44,6 @@ static inline void wr_fence(void) > #define rmb() barrier() > #define wmb() mb() > > -#ifndef CONFIG_SMP > -#define fence() do { } while (0) > -#define smp_mb() barrier() > -#define smp_rmb() barrier() > -#define smp_wmb() barrier() > -#else !SMP kernel text differs, but only because of new presence of unused metag_fence() inline function. If I #if 0 that out, then it matches, so thats fine. > - > #ifdef CONFIG_METAG_SMP_WRITE_REORDERING > /* > * Write to the atomic memory unlock system event register (command 0). This is > @@ -60,26 +53,31 @@ static inline void wr_fence(void) > * incoherence). It is therefore ineffective if used after and on the same > * thread as a write. > */ > -static inline void fence(void) > +static inline void metag_fence(void) > { > volatile int *flushptr = (volatile int *) LINSYSEVENT_WR_ATOMIC_UNLOCK; > barrier(); > *flushptr = 0; > barrier(); > } > -#define smp_mb() fence() > -#define smp_rmb() fence() > -#define smp_wmb() barrier() > +#define __smp_mb() metag_fence() > +#define __smp_rmb() metag_fence() > +#define __smp_wmb() barrier() > #else > -#define fence() do { } while (0) > -#define smp_mb() barrier() > -#define smp_rmb() barrier() > -#define smp_wmb() barrier() > +#define metag_fence() do { } while (0) > +#define __smp_mb() barrier() > +#define __smp_rmb() barrier() > +#define __smp_wmb() barrier() Whitespace is now messed up. Admitedly its already inconsistent tabs/spaces, but it'd be nice if the definitions at least still all lined up. You're touching all the definitions which use spaces anyway, so feel free to convert them to tabs while you're at it. Other than those niggles, it looks sensible to me: Acked-by: James Hogan <james.hogan@imgtec.com> Cheers James > #endif > + > +#ifdef CONFIG_SMP > +#define fence() metag_fence() > +#else > +#define fence() do { } while (0) > #endif > > -#define smp_mb__before_atomic() barrier() > -#define smp_mb__after_atomic() barrier() > +#define __smp_mb__before_atomic() barrier() > +#define __smp_mb__after_atomic() barrier() > > #include <asm-generic/barrier.h> > > -- > MST >
On Tue, Jan 05, 2016 at 12:09:30AM +0000, James Hogan wrote: > Hi Michael, > > On Thu, Dec 31, 2015 at 09:08:22PM +0200, Michael S. Tsirkin wrote: > > This defines __smp_xxx barriers for metag, > > for use by virtualization. > > > > smp_xxx barriers are removed as they are > > defined correctly by asm-generic/barriers.h > > > > Note: as __smp_XX macros should not depend on CONFIG_SMP, they can not > > use the existing fence() macro since that is defined differently between > > SMP and !SMP. For this reason, this patch introduces a wrapper > > metag_fence() that doesn't depend on CONFIG_SMP. > > fence() is then defined using that, depending on CONFIG_SMP. > > I'm not a fan of the inconsistent commit message wrapping. I wrap to 72 > columns (although I now notice SubmittingPatches says to use 75...). > > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > Acked-by: Arnd Bergmann <arnd@arndb.de> > > --- > > arch/metag/include/asm/barrier.h | 32 +++++++++++++++----------------- > > 1 file changed, 15 insertions(+), 17 deletions(-) > > > > diff --git a/arch/metag/include/asm/barrier.h b/arch/metag/include/asm/barrier.h > > index b5b778b..84880c9 100644 > > --- a/arch/metag/include/asm/barrier.h > > +++ b/arch/metag/include/asm/barrier.h > > @@ -44,13 +44,6 @@ static inline void wr_fence(void) > > #define rmb() barrier() > > #define wmb() mb() > > > > -#ifndef CONFIG_SMP > > -#define fence() do { } while (0) > > -#define smp_mb() barrier() > > -#define smp_rmb() barrier() > > -#define smp_wmb() barrier() > > -#else > > !SMP kernel text differs, but only because of new presence of unused > metag_fence() inline function. If I #if 0 that out, then it matches, so > thats fine. > > > - > > #ifdef CONFIG_METAG_SMP_WRITE_REORDERING > > /* > > * Write to the atomic memory unlock system event register (command 0). This is > > @@ -60,26 +53,31 @@ static inline void wr_fence(void) > > * incoherence). It is therefore ineffective if used after and on the same > > * thread as a write. > > */ > > -static inline void fence(void) > > +static inline void metag_fence(void) > > { > > volatile int *flushptr = (volatile int *) LINSYSEVENT_WR_ATOMIC_UNLOCK; > > barrier(); > > *flushptr = 0; > > barrier(); > > } > > -#define smp_mb() fence() > > -#define smp_rmb() fence() > > -#define smp_wmb() barrier() > > +#define __smp_mb() metag_fence() > > +#define __smp_rmb() metag_fence() > > +#define __smp_wmb() barrier() > > #else > > -#define fence() do { } while (0) > > -#define smp_mb() barrier() > > -#define smp_rmb() barrier() > > -#define smp_wmb() barrier() > > +#define metag_fence() do { } while (0) > > +#define __smp_mb() barrier() > > +#define __smp_rmb() barrier() > > +#define __smp_wmb() barrier() > > Whitespace is now messed up. Admitedly its already inconsistent > tabs/spaces, but it'd be nice if the definitions at least still all > lined up. You're touching all the definitions which use spaces anyway, > so feel free to convert them to tabs while you're at it. > > Other than those niggles, it looks sensible to me: > Acked-by: James Hogan <james.hogan@imgtec.com> > > Cheers > James Thanks! I did this in my tree (replaced spaces with tabs in the new definitions); not reposting just because of this change. > > #endif > > + > > +#ifdef CONFIG_SMP > > +#define fence() metag_fence() > > +#else > > +#define fence() do { } while (0) > > #endif > > > > -#define smp_mb__before_atomic() barrier() > > -#define smp_mb__after_atomic() barrier() > > +#define __smp_mb__before_atomic() barrier() > > +#define __smp_mb__after_atomic() barrier() > > > > #include <asm-generic/barrier.h> > > > > -- > > MST > >
diff --git a/arch/metag/include/asm/barrier.h b/arch/metag/include/asm/barrier.h index b5b778b..84880c9 100644 --- a/arch/metag/include/asm/barrier.h +++ b/arch/metag/include/asm/barrier.h @@ -44,13 +44,6 @@ static inline void wr_fence(void) #define rmb() barrier() #define wmb() mb() -#ifndef CONFIG_SMP -#define fence() do { } while (0) -#define smp_mb() barrier() -#define smp_rmb() barrier() -#define smp_wmb() barrier() -#else - #ifdef CONFIG_METAG_SMP_WRITE_REORDERING /* * Write to the atomic memory unlock system event register (command 0). This is @@ -60,26 +53,31 @@ static inline void wr_fence(void) * incoherence). It is therefore ineffective if used after and on the same * thread as a write. */ -static inline void fence(void) +static inline void metag_fence(void) { volatile int *flushptr = (volatile int *) LINSYSEVENT_WR_ATOMIC_UNLOCK; barrier(); *flushptr = 0; barrier(); } -#define smp_mb() fence() -#define smp_rmb() fence() -#define smp_wmb() barrier() +#define __smp_mb() metag_fence() +#define __smp_rmb() metag_fence() +#define __smp_wmb() barrier() #else -#define fence() do { } while (0) -#define smp_mb() barrier() -#define smp_rmb() barrier() -#define smp_wmb() barrier() +#define metag_fence() do { } while (0) +#define __smp_mb() barrier() +#define __smp_rmb() barrier() +#define __smp_wmb() barrier() #endif + +#ifdef CONFIG_SMP +#define fence() metag_fence() +#else +#define fence() do { } while (0) #endif -#define smp_mb__before_atomic() barrier() -#define smp_mb__after_atomic() barrier() +#define __smp_mb__before_atomic() barrier() +#define __smp_mb__after_atomic() barrier() #include <asm-generic/barrier.h>