Re: [Xen-devel] [PATCH 3/3] xen: arm: retry trylock if strex fails on free lock.

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2013-07-29 at 16:52 +0100, Tim Deegan wrote:
> At 16:20 +0100 on 19 Jul (1374250810), Ian Campbell wrote:
> > This comes from the Linux patches 15e7e5c1ebf5 for arm32 and 4ecf7ccb1973 
> > for
> > arm64 by Will Deacon and Catalin Marinas respectively. The Linux commit 
> > message
> > says:
> > 
> >     An exclusive store instruction may fail for reasons other than lock
> >     contention (e.g. a cache eviction during the critical section) so, in
> >     line with other architectures using similar exclusive instructions
> >     (alpha, mips, powerpc), retry the trylock operation if the lock appears
> >     to be free but the strex reported failure.
> > 
> > I have observed this due to register_cpu_notifier containing:
> >     if ( !spin_trylock(&cpu_add_remove_lock) )
> >         BUG(); /* Should never fail as we are called only during boot. */
> > which was spuriously failing.
> > 
> > The ARMv8 variant is taken directly from the Linux patch. For v7 I had to
> > reimplement since we don't currently use ticket locks.
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> 
> Looks good, but:
> 
> >  static always_inline int _raw_spin_trylock(raw_spinlock_t *lock)
> >  {
> > -    unsigned long tmp;
> > -
> > -    __asm__ __volatile__(
> > -"   ldrex   %0, [%1]\n"
> > -"   teq     %0, #0\n"
> > -"   strexeq %0, %2, [%1]"
> > -    : "=&r" (tmp)
> > -    : "r" (&lock->lock), "r" (1)
> > -    : "cc");
> > -
> > -    if (tmp == 0) {
> > +    unsigned long contended, res;
> > +
> > +    do {
> > +        __asm__ __volatile__(
> > +    "   ldrex   %0, [%2]\n"
> > +    "   teq     %0, #0\n"
> > +    "   strexeq %1, %3, [%2]\n"
> > +    "   movne   %1, #0\n"
> > +        : "=&r" (contended), "=r" (res)
> > +        : "r" (&lock->lock), "r" (1)
> 
> Shouldn't this be using a 'Q' constraint for the lock, following the
> pattern set in patch 2/3?

That patch was arm64 specific while this is arm32 code. 'Q' is a machine
specific constraint which is at least worded differently for 32-vs-64 in
http://gcc.gnu.org/onlinedocs/gcc/Machine-Constraints.html#Machine-Constraints 
although I suppose they both read as the same thing.

I suppose the answer is that ldrex etc can take [rN, #imm] arguments,
which is what "Q" rather than "r" is trying to avoid, where as the newer
armv8 atomic instructions do not take a #imm (it is documented as "[rN,
#0]"), so you have to use Q there.

Ian.

> 
> > +        : "cc");
> > +    } while (res);
> > +
> > +    if (!contended) {
> >          smp_mb();
> >          return 1;
> >      } else {



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel