Re: [PATCHv2 3/6] xen: generic xadd() for ticket locks

[Ian Campbell <ian.campbell@citrix.com>]

On Fri, 2015-04-17 at 13:34 +0100, David Vrabel wrote:
> On 17/04/15 13:32, Ian Campbell wrote:
> > On Thu, 2015-04-16 at 16:28 +0100, Jan Beulich wrote:
> >>>>> On 10.04.15 at 16:19, <david.vrabel@citrix.com> wrote:
> >>> +#define xadd(ptr, v) generic_xaddl((ptr), (v))
> >>
> >> I think it is at least confusing to call the thing xadd (looking to be
> >> size generic) and then expand to generic_xaddl (only supporting
> >> 32-bit operations), yet subsequently implementing a size-generic
> >> xadd() for x86.
> > 
> > Indeed, and I went to build on arm32 prior to hacking up a proper xadd
> > and:
> > 
> > spinlock.c: In function ‘_spin_lock’:
> > spinlock.c:145:5: error: passing argument 1 of ‘generic_xaddl’ from incompatible pointer type [-Werror]
> >      tickets.head_tail = xadd(&lock->tickets, tickets.head_tail);
> >      ^
> > spinlock.c:15:12: note: expected ‘volatile u32 *’ but argument is of type ‘union spinlock_tickets_t *’
> >  static u32 generic_xaddl(volatile u32 *ptr, u32 v)
> >             ^
> > 
> > (I hope to knock up the arm asm version in the next hour or so, so you
> > may not care...)
> 
> Can you use
> 
>   git://xenbits.xen.org/people/dvrabel/xen.git ticketlocks-v3
> 
> as a base instead?

I tried that and it built and booted just fine on both arm32 and arm64.

I eyeballed the assembly produced via the use of __sync_fetch_and_add
(for _spin_lock only) and it is exactly what I would have written in my
own versions.

I was using gcc 4.8.3 in both cases. For arm64 I'm pretty sure we don't
want to consider anything earlier.

For arm32 I have also tried gcc 4.6.3 (Debian Wheezy's compiler) and it
built and booted, and eyeballing shows the same asm. I think that's the
earliest we really need to worry about.

IOW I'm not going to bother with custom versions of these functions on
ARM. If you wanted you could drop the #ifndef xadd from
asm-arm/system.h.

Perhaps it would be useful to add some of the info from my tests
reported above, or a reference to this mail, to the commit log?

in either case you can add:

        Acked-by: Ian Campbell <ian.campbell@citrix.com>

to the patch below. 

commit b08cf3fa4791d7ff0d01fb932192e02078ce670a
Author: David Vrabel <david.vrabel@citrix.com>
Date:   Thu Apr 16 15:31:18 2015 +0100

    arm: provide xadd()
    
    xadd() atomically adds a value and returns the previous value.  This
    is needed to implement ticket locks.
    
    This generic arm implementation uses the GCC __sync_fetch_and_add()
    builtin, but a arm32 or arm64 specific variant could be provided in the
    future (e.g., if required to support older versions of GCC).
    
    Signed-off-by: David Vrabel <david.vrabel@citrix.com>

diff --git a/xen/include/asm-arm/system.h b/xen/include/asm-arm/system.h
index ce3d38a..f037e84 100644
--- a/xen/include/asm-arm/system.h
+++ b/xen/include/asm-arm/system.h
@@ -51,6 +51,10 @@
 # error "unknown ARM variant"
 #endif
 
+#ifndef xadd
+#  define xadd(x, v) __sync_fetch_and_add(x, v)
+#endif
+
 extern struct vcpu *__context_switch(struct vcpu *prev, struct vcpu *next);
 
 #endif

Ian.



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