On Mon, 2013-11-11 at 18:10 +0000, Will Deacon wrote:
> Hello,
>
> On Fri, Nov 08, 2013 at 07:52:38PM +0000, Tim Chen wrote:
> > From: Waiman Long <Waiman.Long@xxxxxx>
> >
> > This patch corrects the way memory barriers are used in the MCS lock
> > with smp_load_acquire and smp_store_release fucnction.
> > It removes ones that are not needed.
> >
> > It uses architecture specific load-acquire and store-release
> > primitives for synchronization, if available. Generic implementations
> > are provided in case they are not defined even though they may not
> > be optimal. These generic implementation could be removed later on
> > once changes are made in all the relevant header files.
> >
> > Suggested-by: Michel Lespinasse <walken@xxxxxxxxxx>
> > Signed-off-by: Waiman Long <Waiman.Long@xxxxxx>
> > Signed-off-by: Jason Low <jason.low2@xxxxxx>
> > Signed-off-by: Tim Chen <tim.c.chen@xxxxxxxxxxxxxxx>
> > ---
> > kernel/locking/mcs_spinlock.c | 48 +++++++++++++++++++++++++++++++++++------
> > 1 files changed, 41 insertions(+), 7 deletions(-)
> >
> > diff --git a/kernel/locking/mcs_spinlock.c b/kernel/locking/mcs_spinlock.c
> > index b6f27f8..df5c167 100644
> > --- a/kernel/locking/mcs_spinlock.c
> > +++ b/kernel/locking/mcs_spinlock.c
> > @@ -23,6 +23,31 @@
> > #endif
> >
> > /*
> > + * Fall back to use the regular atomic operations and memory barrier if
> > + * the acquire/release versions are not defined.
> > + */
> > +#ifndef xchg_acquire
> > +# define xchg_acquire(p, v) xchg(p, v)
> > +#endif
> > +
> > +#ifndef smp_load_acquire
> > +# define smp_load_acquire(p) \
> > + ({ \
> > + typeof(*p) __v = ACCESS_ONCE(*(p)); \
> > + smp_mb(); \
> > + __v; \
> > + })
> > +#endif
> > +
> > +#ifndef smp_store_release
> > +# define smp_store_release(p, v) \
> > + do { \
> > + smp_mb(); \
> > + ACCESS_ONCE(*(p)) = v; \
> > + } while (0)
> > +#endif
>
> PeterZ already has a series implementing acquire/release accessors, so you
> should probably take a look at that rather than rolling your own here.
Yes, we are using Peter Z's implementation here. The above is for anything
where smp_load_acquire and smp_store_release are *not* defined. We can
remove this once all architectures implement the acquire and release
functions as mentioned in the comments of the patch.
>
> You could then augment that with [cmp]xchg_{acquire,release} as
> appropriate.
>
> > +/*
> > * In order to acquire the lock, the caller should declare a local node and
> > * pass a reference of the node to this function in addition to the lock.
> > * If the lock has already been acquired, then this will proceed to spin
> > @@ -37,15 +62,19 @@ void mcs_spin_lock(struct mcs_spinlock **lock, struct mcs_spinlock *node)
> > node->locked = 0;
> > node->next = NULL;
> >
> > - prev = xchg(lock, node);
> > + /* xchg() provides a memory barrier */
> > + prev = xchg_acquire(lock, node);
> > if (likely(prev == NULL)) {
> > /* Lock acquired */
> > return;
> > }
> > ACCESS_ONCE(prev->next) = node;
> > - smp_wmb();
> > - /* Wait until the lock holder passes the lock down */
> > - while (!ACCESS_ONCE(node->locked))
> > + /*
> > + * Wait until the lock holder passes the lock down.
> > + * Using smp_load_acquire() provides a memory barrier that
> > + * ensures subsequent operations happen after the lock is acquired.
> > + */
> > + while (!(smp_load_acquire(&node->locked)))
> > arch_mutex_cpu_relax();
>
An alternate implementation is
while (!ACCESS_ONCE(node->locked))
arch_mutex_cpu_relax();
smp_load_acquire(&node->locked);
Leaving the smp_load_acquire at the end to provide appropriate barrier.
Will that be acceptable?
Tim
> After a chat with some micro-architects, I'm going to have to disagree with
> Paul here. For architectures where acquire/release are implemented with
> explicit barriers (similarly for simple microarchitectures), emitting
> barriers in a loop *is* going to have an affect on overall performance,
> since those barriers may well result in traffic outside of the core (at
> least, on ARM).
>
> Thinking more about that, the real issue here is that arch_mutex_cpu_relax()
> doesn't have a corresponding hook on the unlock side. On ARM, for example,
> we can enter a low-power state using the wfe instruction, but that requires
> the unlocker to wake up the core when the lock is released.
>
> So, although I'm completely in favour of introducing acquire/release
> accessors, I really think the mcs locking routines would benefit from
> some arch-specific backend code, even if it's optional (although I would
> imagine most architectures implementing something to improve power and/or
> performance).
>
> Will
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>