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

[Tim Deegan <tim@xxxxxxx>]

At 17:20 +0100 on 29 Jul (1375118421), Ian Campbell wrote:
> 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.

Righto, thanks.

Acked-by: Tim Deegan <tim@xxxxxxx>

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