[Xen-devel] [PATCH v3 0/2] rwlock: allow recursive read locking when already locked in write mode

[Xen-devel] [PATCH v3 0/2] rwlock: allow recursive read locking when already locked in write mode

[Roger Pau Monne]

Hello,

The following series implement support for read-locking a lock already
taken in write mode by the same CPU. Patch #1 add an atomic_and
operation to be used by patch #2 which implements the mentioned
behavior.

The series has been build tested on Arm by gitlab:

https://gitlab.com/xen-project/people/royger/xen/pipelines/120144901

Thanks, Roger.

Roger Pau Monne (2):
  atomic: add atomic_and operations
  rwlock: allow recursive read locking when already locked in write mode

 xen/common/rwlock.c                |  4 +--
 xen/include/asm-arm/arm32/atomic.h | 17 ++++++++++
 xen/include/asm-arm/arm64/atomic.h | 14 ++++++++
 xen/include/asm-x86/atomic.h       |  8 +++++
 xen/include/xen/rwlock.h           | 52 ++++++++++++++++++------------
 5 files changed, 73 insertions(+), 22 deletions(-)

-- 
2.25.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v3 1/2] atomic: add atomic_and operations

[Roger Pau Monne]

To x86 and Arm. This performs an atomic AND operation against an
atomic_t variable with the provided mask.

Requested-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/include/asm-arm/arm32/atomic.h | 17 +++++++++++++++++
 xen/include/asm-arm/arm64/atomic.h | 14 ++++++++++++++
 xen/include/asm-x86/atomic.h       |  8 ++++++++
 3 files changed, 39 insertions(+)

diff --git a/xen/include/asm-arm/arm32/atomic.h b/xen/include/asm-arm/arm32/atomic.h
index c03eb684cd..4637381bcc 100644
--- a/xen/include/asm-arm/arm32/atomic.h
+++ b/xen/include/asm-arm/arm32/atomic.h
@@ -96,6 +96,23 @@ static inline int atomic_sub_return(int i, atomic_t *v)
 	return result;
 }
 
+static inline void atomic_and(unsigned int m, atomic_t *v)
+{
+	unsigned long tmp;
+	int result;
+
+	prefetchw(&v->counter);
+	__asm__ __volatile__("@ atomic_and\n"
+"1:	ldrex	%0, [%3]\n"
+"	and	%0, %0, %4\n"
+"	strex	%1, %0, [%3]\n"
+"	teq	%1, #0\n"
+"	bne	1b"
+	: "=&r" (result), "=&r" (tmp), "+Qo" (v->counter)
+	: "r" (&v->counter), "Ir" (m)
+	: "cc");
+}
+
 static inline int atomic_cmpxchg(atomic_t *ptr, int old, int new)
 {
 	int oldval;
diff --git a/xen/include/asm-arm/arm64/atomic.h b/xen/include/asm-arm/arm64/atomic.h
index bce38d4ca2..2f906f0265 100644
--- a/xen/include/asm-arm/arm64/atomic.h
+++ b/xen/include/asm-arm/arm64/atomic.h
@@ -91,6 +91,20 @@ static inline int atomic_sub_return(int i, atomic_t *v)
 	return result;
 }
 
+static inline void atomic_and(unsigned int m, atomic_t *v)
+{
+	unsigned long tmp;
+	int result;
+
+	asm volatile("// atomic_and\n"
+"1:	ldxr	%w0, %2\n"
+"	and	%w0, %w0, %w3\n"
+"	stxr	%w1, %w0, %2\n"
+"	cbnz	%w1, 1b"
+	: "=&r" (result), "=&r" (tmp), "+Q" (v->counter)
+	: "Ir" (m));
+}
+
 static inline int atomic_cmpxchg(atomic_t *ptr, int old, int new)
 {
 	unsigned long tmp;
diff --git a/xen/include/asm-x86/atomic.h b/xen/include/asm-x86/atomic.h
index 682bcf91b1..21794bfe7b 100644
--- a/xen/include/asm-x86/atomic.h
+++ b/xen/include/asm-x86/atomic.h
@@ -224,6 +224,14 @@ static inline int atomic_add_unless(atomic_t *v, int a, int u)
     return c;
 }
 
+static inline void atomic_and(unsigned int m, atomic_t *v)
+{
+    asm volatile (
+        "lock; andl %1,%0"
+        : "=m" (*(volatile int *)&v->counter)
+        : "ir" (m), "m" (*(volatile int *)&v->counter) );
+}
+
 #define atomic_xchg(v, new) (xchg(&((v)->counter), new))
 
 #endif /* __ARCH_X86_ATOMIC__ */
-- 
2.25.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v3 2/2] rwlock: allow recursive read locking when already locked in write mode

[Roger Pau Monne]

Allow a CPU already holding the lock in write mode to also lock it in
read mode. There's no harm in allowing read locking a rwlock that's
already owned by the caller (ie: CPU) in write mode. Allowing such
accesses is required at least for the CPU maps use-case.

In order to do this reserve 12bits of the lock, this allows to support
up to 4096 CPUs. Also reduce the write lock mask to 2 bits: one to
signal there are pending writers waiting on the lock and the other to
signal the lock is owned in write mode.

This reduces the maximum number of concurrent readers from 16777216 to
262144, I think this should still be enough, or else the lock field
can be expanded from 32 to 64bits if all architectures support atomic
operations on 64bit integers.

Fixes: 5872c83b42c608 ('smp: convert the cpu maps lock into a rw lock')
Reported-by: Jan Beulich <jbeulich@suse.com>
Reported-by: Jürgen Groß <jgross@suse.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v2:
 - Use atomic_and to clear the writers part of the lock when
   write-unlocking.
 - Reduce writer-related area to 14bits.
 - Include xen/smp.h for Arm64.

Changes since v1:
 - Move the processor ID field to start at bit 0 and hence avoid the
   shift when checking it.
 - Enlarge write related structures to use 16bit, and hence allow them
   to be cleared using an atomic write.
---
 xen/common/rwlock.c      |  4 ++--
 xen/include/xen/rwlock.h | 52 ++++++++++++++++++++++++----------------
 2 files changed, 34 insertions(+), 22 deletions(-)

diff --git a/xen/common/rwlock.c b/xen/common/rwlock.c
index d568bbf6de..dadab372b5 100644
--- a/xen/common/rwlock.c
+++ b/xen/common/rwlock.c
@@ -69,7 +69,7 @@ void queue_write_lock_slowpath(rwlock_t *lock)
 
     /* Try to acquire the lock directly if no reader is present. */
     if ( !atomic_read(&lock->cnts) &&
-         (atomic_cmpxchg(&lock->cnts, 0, _QW_LOCKED) == 0) )
+         (atomic_cmpxchg(&lock->cnts, 0, _write_lock_val()) == 0) )
         goto unlock;
 
     /*
@@ -93,7 +93,7 @@ void queue_write_lock_slowpath(rwlock_t *lock)
         cnts = atomic_read(&lock->cnts);
         if ( (cnts == _QW_WAITING) &&
              (atomic_cmpxchg(&lock->cnts, _QW_WAITING,
-                             _QW_LOCKED) == _QW_WAITING) )
+                             _write_lock_val()) == _QW_WAITING) )
             break;
 
         cpu_relax();
diff --git a/xen/include/xen/rwlock.h b/xen/include/xen/rwlock.h
index 3dfea1ac2a..82af36b1b4 100644
--- a/xen/include/xen/rwlock.h
+++ b/xen/include/xen/rwlock.h
@@ -2,6 +2,7 @@
 #define __RWLOCK_H__
 
 #include <xen/percpu.h>
+#include <xen/smp.h>
 #include <xen/spinlock.h>
 
 #include <asm/atomic.h>
@@ -20,21 +21,30 @@ typedef struct {
 #define DEFINE_RWLOCK(l) rwlock_t l = RW_LOCK_UNLOCKED
 #define rwlock_init(l) (*(l) = (rwlock_t)RW_LOCK_UNLOCKED)
 
-/*
- * Writer states & reader shift and bias.
- *
- * Writer field is 8 bit to allow for potential optimisation, see
- * _write_unlock().
- */
-#define    _QW_WAITING  1               /* A writer is waiting     */
-#define    _QW_LOCKED   0xff            /* A writer holds the lock */
-#define    _QW_WMASK    0xff            /* Writer mask.*/
-#define    _QR_SHIFT    8               /* Reader count shift      */
+/* Writer states & reader shift and bias. */
+#define    _QW_CPUMASK  0xfff               /* Writer CPU mask */
+#define    _QW_SHIFT    12                  /* Writer flags shift */
+#define    _QW_WAITING  (1u << _QW_SHIFT)   /* A writer is waiting */
+#define    _QW_LOCKED   (3u << _QW_SHIFT)   /* A writer holds the lock */
+#define    _QW_WMASK    (3u << _QW_SHIFT)   /* Writer mask */
+#define    _QR_SHIFT    14                  /* Reader count shift */
 #define    _QR_BIAS     (1U << _QR_SHIFT)
 
 void queue_read_lock_slowpath(rwlock_t *lock);
 void queue_write_lock_slowpath(rwlock_t *lock);
 
+static inline bool _is_write_locked_by_me(uint32_t cnts)
+{
+    BUILD_BUG_ON(_QW_CPUMASK < NR_CPUS);
+    return (cnts & _QW_WMASK) == _QW_LOCKED &&
+           (cnts & _QW_CPUMASK) == smp_processor_id();
+}
+
+static inline bool _can_read_lock(uint32_t cnts)
+{
+    return !(cnts & _QW_WMASK) || _is_write_locked_by_me(cnts);
+}
+
 /*
  * _read_trylock - try to acquire read lock of a queue rwlock.
  * @lock : Pointer to queue rwlock structure.
@@ -45,10 +55,10 @@ static inline int _read_trylock(rwlock_t *lock)
     u32 cnts;
 
     cnts = atomic_read(&lock->cnts);
-    if ( likely(!(cnts & _QW_WMASK)) )
+    if ( likely(_can_read_lock(cnts)) )
     {
         cnts = (u32)atomic_add_return(_QR_BIAS, &lock->cnts);
-        if ( likely(!(cnts & _QW_WMASK)) )
+        if ( likely(_can_read_lock(cnts)) )
             return 1;
         atomic_sub(_QR_BIAS, &lock->cnts);
     }
@@ -64,7 +74,7 @@ static inline void _read_lock(rwlock_t *lock)
     u32 cnts;
 
     cnts = atomic_add_return(_QR_BIAS, &lock->cnts);
-    if ( likely(!(cnts & _QW_WMASK)) )
+    if ( likely(_can_read_lock(cnts)) )
         return;
 
     /* The slowpath will decrement the reader count, if necessary. */
@@ -115,6 +125,11 @@ static inline int _rw_is_locked(rwlock_t *lock)
     return atomic_read(&lock->cnts);
 }
 
+static inline uint32_t _write_lock_val(void)
+{
+    return _QW_LOCKED | smp_processor_id();
+}
+
 /*
  * queue_write_lock - acquire write lock of a queue rwlock.
  * @lock : Pointer to queue rwlock structure.
@@ -122,7 +137,7 @@ static inline int _rw_is_locked(rwlock_t *lock)
 static inline void _write_lock(rwlock_t *lock)
 {
     /* Optimize for the unfair lock case where the fair flag is 0. */
-    if ( atomic_cmpxchg(&lock->cnts, 0, _QW_LOCKED) == 0 )
+    if ( atomic_cmpxchg(&lock->cnts, 0, _write_lock_val()) == 0 )
         return;
 
     queue_write_lock_slowpath(lock);
@@ -157,16 +172,13 @@ static inline int _write_trylock(rwlock_t *lock)
     if ( unlikely(cnts) )
         return 0;
 
-    return likely(atomic_cmpxchg(&lock->cnts, 0, _QW_LOCKED) == 0);
+    return likely(atomic_cmpxchg(&lock->cnts, 0, _write_lock_val()) == 0);
 }
 
 static inline void _write_unlock(rwlock_t *lock)
 {
-    /*
-     * If the writer field is atomic, it can be cleared directly.
-     * Otherwise, an atomic subtraction will be used to clear it.
-     */
-    atomic_sub(_QW_LOCKED, &lock->cnts);
+    ASSERT(_is_write_locked_by_me(atomic_read(&lock->cnts)));
+    atomic_and(~(_QW_CPUMASK | _QW_WMASK), &lock->cnts);
 }
 
 static inline void _write_unlock_irq(rwlock_t *lock)
-- 
2.25.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 1/2] atomic: add atomic_and operations

[Julien Grall]

Hi Roger,

The logic for Arm64 and Arm32 looks good to me. I just have one question 
below.

On 24/02/2020 08:43, Roger Pau Monne wrote:
> To x86 and Arm. This performs an atomic AND operation against an
> atomic_t variable with the provided mask.
> 
> Requested-by: Jan Beulich <jbeulich@suse.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>   xen/include/asm-arm/arm32/atomic.h | 17 +++++++++++++++++
>   xen/include/asm-arm/arm64/atomic.h | 14 ++++++++++++++
>   xen/include/asm-x86/atomic.h       |  8 ++++++++
>   3 files changed, 39 insertions(+)
> 
> diff --git a/xen/include/asm-arm/arm32/atomic.h b/xen/include/asm-arm/arm32/atomic.h
> index c03eb684cd..4637381bcc 100644
> --- a/xen/include/asm-arm/arm32/atomic.h
> +++ b/xen/include/asm-arm/arm32/atomic.h
> @@ -96,6 +96,23 @@ static inline int atomic_sub_return(int i, atomic_t *v)
>   	return result;
>   }
>   
> +static inline void atomic_and(unsigned int m, atomic_t *v)

All the atomic helpers have taken a signed int so far because the 
counter is an int. Any reason to diverge from that?

> +{
> +	unsigned long tmp;
> +	int result;
> +
> +	prefetchw(&v->counter);
> +	__asm__ __volatile__("@ atomic_and\n"
> +"1:	ldrex	%0, [%3]\n"
> +"	and	%0, %0, %4\n"
> +"	strex	%1, %0, [%3]\n"
> +"	teq	%1, #0\n"
> +"	bne	1b"
> +	: "=&r" (result), "=&r" (tmp), "+Qo" (v->counter)
> +	: "r" (&v->counter), "Ir" (m)
> +	: "cc");
> +}
> +
>   static inline int atomic_cmpxchg(atomic_t *ptr, int old, int new)
>   {
>   	int oldval;
> diff --git a/xen/include/asm-arm/arm64/atomic.h b/xen/include/asm-arm/arm64/atomic.h
> index bce38d4ca2..2f906f0265 100644
> --- a/xen/include/asm-arm/arm64/atomic.h
> +++ b/xen/include/asm-arm/arm64/atomic.h
> @@ -91,6 +91,20 @@ static inline int atomic_sub_return(int i, atomic_t *v)
>   	return result;
>   }
>   
> +static inline void atomic_and(unsigned int m, atomic_t *v)
> +{
> +	unsigned long tmp;
> +	int result;
> +
> +	asm volatile("// atomic_and\n"
> +"1:	ldxr	%w0, %2\n"
> +"	and	%w0, %w0, %w3\n"
> +"	stxr	%w1, %w0, %2\n"
> +"	cbnz	%w1, 1b"
> +	: "=&r" (result), "=&r" (tmp), "+Q" (v->counter)
> +	: "Ir" (m));
> +}
> +
>   static inline int atomic_cmpxchg(atomic_t *ptr, int old, int new)
>   {
>   	unsigned long tmp;
> diff --git a/xen/include/asm-x86/atomic.h b/xen/include/asm-x86/atomic.h
> index 682bcf91b1..21794bfe7b 100644
> --- a/xen/include/asm-x86/atomic.h
> +++ b/xen/include/asm-x86/atomic.h
> @@ -224,6 +224,14 @@ static inline int atomic_add_unless(atomic_t *v, int a, int u)
>       return c;
>   }
>   
> +static inline void atomic_and(unsigned int m, atomic_t *v)
> +{
> +    asm volatile (
> +        "lock; andl %1,%0"
> +        : "=m" (*(volatile int *)&v->counter)
> +        : "ir" (m), "m" (*(volatile int *)&v->counter) );
> +}
> +
>   #define atomic_xchg(v, new) (xchg(&((v)->counter), new))
>   
>   #endif /* __ARCH_X86_ATOMIC__ */
> 

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 2/2] rwlock: allow recursive read locking when already locked in write mode

[Julien Grall]

Hi Roger,

On 24/02/2020 08:44, Roger Pau Monne wrote:
> Allow a CPU already holding the lock in write mode to also lock it in
> read mode. There's no harm in allowing read locking a rwlock that's
> already owned by the caller (ie: CPU) in write mode. Allowing such
> accesses is required at least for the CPU maps use-case.
> 
> In order to do this reserve 12bits of the lock, this allows to support
> up to 4096 CPUs. Also reduce the write lock mask to 2 bits: one to
> signal there are pending writers waiting on the lock and the other to
> signal the lock is owned in write mode.
> 
> This reduces the maximum number of concurrent readers from 16777216 to
> 262144, I think this should still be enough, or else the lock field
> can be expanded from 32 to 64bits if all architectures support atomic
> operations on 64bit integers.
> 
> Fixes: 5872c83b42c608 ('smp: convert the cpu maps lock into a rw lock')
> Reported-by: Jan Beulich <jbeulich@suse.com>
> Reported-by: Jürgen Groß <jgross@suse.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Changes since v2:
>   - Use atomic_and to clear the writers part of the lock when
>     write-unlocking.
>   - Reduce writer-related area to 14bits.
>   - Include xen/smp.h for Arm64.

OOI, is it to use smp_processor_id()?

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 1/2] atomic: add atomic_and operations

[Roger Pau Monné]

On Mon, Feb 24, 2020 at 10:02:53AM +0000, Julien Grall wrote:
> Hi Roger,
> 
> The logic for Arm64 and Arm32 looks good to me. I just have one question
> below.
> 
> On 24/02/2020 08:43, Roger Pau Monne wrote:
> > To x86 and Arm. This performs an atomic AND operation against an
> > atomic_t variable with the provided mask.
> > 
> > Requested-by: Jan Beulich <jbeulich@suse.com>
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> >   xen/include/asm-arm/arm32/atomic.h | 17 +++++++++++++++++
> >   xen/include/asm-arm/arm64/atomic.h | 14 ++++++++++++++
> >   xen/include/asm-x86/atomic.h       |  8 ++++++++
> >   3 files changed, 39 insertions(+)
> > 
> > diff --git a/xen/include/asm-arm/arm32/atomic.h b/xen/include/asm-arm/arm32/atomic.h
> > index c03eb684cd..4637381bcc 100644
> > --- a/xen/include/asm-arm/arm32/atomic.h
> > +++ b/xen/include/asm-arm/arm32/atomic.h
> > @@ -96,6 +96,23 @@ static inline int atomic_sub_return(int i, atomic_t *v)
> >   	return result;
> >   }
> > +static inline void atomic_and(unsigned int m, atomic_t *v)
> 
> All the atomic helpers have taken a signed int so far because the counter is
> an int. Any reason to diverge from that?

Since this is not an arithmetic operation I felt unsigned int was a
more suitable type to describe a bitmask: it felt weird to pass a
bitmask with type int, because signedness doesn't make sense when
referring to a mask.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 2/2] rwlock: allow recursive read locking when already locked in write mode

[Roger Pau Monné]

On Mon, Feb 24, 2020 at 10:05:39AM +0000, Julien Grall wrote:
> Hi Roger,
> 
> On 24/02/2020 08:44, Roger Pau Monne wrote:
> > Allow a CPU already holding the lock in write mode to also lock it in
> > read mode. There's no harm in allowing read locking a rwlock that's
> > already owned by the caller (ie: CPU) in write mode. Allowing such
> > accesses is required at least for the CPU maps use-case.
> > 
> > In order to do this reserve 12bits of the lock, this allows to support
> > up to 4096 CPUs. Also reduce the write lock mask to 2 bits: one to
> > signal there are pending writers waiting on the lock and the other to
> > signal the lock is owned in write mode.
> > 
> > This reduces the maximum number of concurrent readers from 16777216 to
> > 262144, I think this should still be enough, or else the lock field
> > can be expanded from 32 to 64bits if all architectures support atomic
> > operations on 64bit integers.
> > 
> > Fixes: 5872c83b42c608 ('smp: convert the cpu maps lock into a rw lock')
> > Reported-by: Jan Beulich <jbeulich@suse.com>
> > Reported-by: Jürgen Groß <jgross@suse.com>
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> > Changes since v2:
> >   - Use atomic_and to clear the writers part of the lock when
> >     write-unlocking.
> >   - Reduce writer-related area to 14bits.
> >   - Include xen/smp.h for Arm64.
> 
> OOI, is it to use smp_processor_id()?

Yes, or else I would get errors when building asm-offsets on Arm64
IIRC.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 1/2] atomic: add atomic_and operations

[Julien Grall]

Hi,

On 24/02/2020 10:09, Roger Pau Monné wrote:
> On Mon, Feb 24, 2020 at 10:02:53AM +0000, Julien Grall wrote:
>> Hi Roger,
>>
>> The logic for Arm64 and Arm32 looks good to me. I just have one question
>> below.
>>
>> On 24/02/2020 08:43, Roger Pau Monne wrote:
>>> To x86 and Arm. This performs an atomic AND operation against an
>>> atomic_t variable with the provided mask.
>>>
>>> Requested-by: Jan Beulich <jbeulich@suse.com>
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>> ---
>>>    xen/include/asm-arm/arm32/atomic.h | 17 +++++++++++++++++
>>>    xen/include/asm-arm/arm64/atomic.h | 14 ++++++++++++++
>>>    xen/include/asm-x86/atomic.h       |  8 ++++++++
>>>    3 files changed, 39 insertions(+)
>>>
>>> diff --git a/xen/include/asm-arm/arm32/atomic.h b/xen/include/asm-arm/arm32/atomic.h
>>> index c03eb684cd..4637381bcc 100644
>>> --- a/xen/include/asm-arm/arm32/atomic.h
>>> +++ b/xen/include/asm-arm/arm32/atomic.h
>>> @@ -96,6 +96,23 @@ static inline int atomic_sub_return(int i, atomic_t *v)
>>>    	return result;
>>>    }
>>> +static inline void atomic_and(unsigned int m, atomic_t *v)
>>
>> All the atomic helpers have taken a signed int so far because the counter is
>> an int. Any reason to diverge from that?
> 
> Since this is not an arithmetic operation I felt unsigned int was a
> more suitable type to describe a bitmask: it felt weird to pass a
> bitmask with type int, because signedness doesn't make sense when
> referring to a mask.

At some point I would like to have macro generating all the atomics in 
on Arm in the same way a Linux (see asm-generic/atomic.h). This is to 
avoid duplication and make easy to introduce Armv8.1 LSE atomics. So I 
would like to avoid introducing difference in the prototype unless it is 
stricly necessary.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 1/2] atomic: add atomic_and operations

[Roger Pau Monné]

On Mon, Feb 24, 2020 at 10:19:44AM +0000, Julien Grall wrote:
> Hi,
> 
> On 24/02/2020 10:09, Roger Pau Monné wrote:
> > On Mon, Feb 24, 2020 at 10:02:53AM +0000, Julien Grall wrote:
> > > Hi Roger,
> > > 
> > > The logic for Arm64 and Arm32 looks good to me. I just have one question
> > > below.
> > > 
> > > On 24/02/2020 08:43, Roger Pau Monne wrote:
> > > > To x86 and Arm. This performs an atomic AND operation against an
> > > > atomic_t variable with the provided mask.
> > > > 
> > > > Requested-by: Jan Beulich <jbeulich@suse.com>
> > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > > > ---
> > > >    xen/include/asm-arm/arm32/atomic.h | 17 +++++++++++++++++
> > > >    xen/include/asm-arm/arm64/atomic.h | 14 ++++++++++++++
> > > >    xen/include/asm-x86/atomic.h       |  8 ++++++++
> > > >    3 files changed, 39 insertions(+)
> > > > 
> > > > diff --git a/xen/include/asm-arm/arm32/atomic.h b/xen/include/asm-arm/arm32/atomic.h
> > > > index c03eb684cd..4637381bcc 100644
> > > > --- a/xen/include/asm-arm/arm32/atomic.h
> > > > +++ b/xen/include/asm-arm/arm32/atomic.h
> > > > @@ -96,6 +96,23 @@ static inline int atomic_sub_return(int i, atomic_t *v)
> > > >    	return result;
> > > >    }
> > > > +static inline void atomic_and(unsigned int m, atomic_t *v)
> > > 
> > > All the atomic helpers have taken a signed int so far because the counter is
> > > an int. Any reason to diverge from that?
> > 
> > Since this is not an arithmetic operation I felt unsigned int was a
> > more suitable type to describe a bitmask: it felt weird to pass a
> > bitmask with type int, because signedness doesn't make sense when
> > referring to a mask.
> 
> At some point I would like to have macro generating all the atomics in on
> Arm in the same way a Linux (see asm-generic/atomic.h). This is to avoid
> duplication and make easy to introduce Armv8.1 LSE atomics. So I would like
> to avoid introducing difference in the prototype unless it is stricly
> necessary.

Why not have the macro generator function get passed the type of the
parameter?

Easily enough you could even have a second wrapper around that
generator macro, ie: GEN_ATOMIC_{ARITHMETIC/BITMASK} or some such that
hides the parameter type underneath.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 1/2] atomic: add atomic_and operations

[Julien Grall]

Hi,

On 24/02/2020 10:23, Roger Pau Monné wrote:
> On Mon, Feb 24, 2020 at 10:19:44AM +0000, Julien Grall wrote:
>> Hi,
>>
>> On 24/02/2020 10:09, Roger Pau Monné wrote:
>>> On Mon, Feb 24, 2020 at 10:02:53AM +0000, Julien Grall wrote:
>>>> Hi Roger,
>>>>
>>>> The logic for Arm64 and Arm32 looks good to me. I just have one question
>>>> below.
>>>>
>>>> On 24/02/2020 08:43, Roger Pau Monne wrote:
>>>>> To x86 and Arm. This performs an atomic AND operation against an
>>>>> atomic_t variable with the provided mask.
>>>>>
>>>>> Requested-by: Jan Beulich <jbeulich@suse.com>
>>>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>>>> ---
>>>>>     xen/include/asm-arm/arm32/atomic.h | 17 +++++++++++++++++
>>>>>     xen/include/asm-arm/arm64/atomic.h | 14 ++++++++++++++
>>>>>     xen/include/asm-x86/atomic.h       |  8 ++++++++
>>>>>     3 files changed, 39 insertions(+)
>>>>>
>>>>> diff --git a/xen/include/asm-arm/arm32/atomic.h b/xen/include/asm-arm/arm32/atomic.h
>>>>> index c03eb684cd..4637381bcc 100644
>>>>> --- a/xen/include/asm-arm/arm32/atomic.h
>>>>> +++ b/xen/include/asm-arm/arm32/atomic.h
>>>>> @@ -96,6 +96,23 @@ static inline int atomic_sub_return(int i, atomic_t *v)
>>>>>     	return result;
>>>>>     }
>>>>> +static inline void atomic_and(unsigned int m, atomic_t *v)
>>>>
>>>> All the atomic helpers have taken a signed int so far because the counter is
>>>> an int. Any reason to diverge from that?
>>>
>>> Since this is not an arithmetic operation I felt unsigned int was a
>>> more suitable type to describe a bitmask: it felt weird to pass a
>>> bitmask with type int, because signedness doesn't make sense when
>>> referring to a mask.
>>
>> At some point I would like to have macro generating all the atomics in on
>> Arm in the same way a Linux (see asm-generic/atomic.h). This is to avoid
>> duplication and make easy to introduce Armv8.1 LSE atomics. So I would like
>> to avoid introducing difference in the prototype unless it is stricly
>> necessary.
> 
> Why not have the macro generator function get passed the type of the
> parameter?

It is not worth it for a simple operation and I don't want to diverge 
too much of atomics from Linux.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 1/2] atomic: add atomic_and operations

[Roger Pau Monné]

On Mon, Feb 24, 2020 at 10:29:40AM +0000, Julien Grall wrote:
> Hi,
> 
> On 24/02/2020 10:23, Roger Pau Monné wrote:
> > On Mon, Feb 24, 2020 at 10:19:44AM +0000, Julien Grall wrote:
> > > Hi,
> > > 
> > > On 24/02/2020 10:09, Roger Pau Monné wrote:
> > > > On Mon, Feb 24, 2020 at 10:02:53AM +0000, Julien Grall wrote:
> > > > > Hi Roger,
> > > > > 
> > > > > The logic for Arm64 and Arm32 looks good to me. I just have one question
> > > > > below.
> > > > > 
> > > > > On 24/02/2020 08:43, Roger Pau Monne wrote:
> > > > > > To x86 and Arm. This performs an atomic AND operation against an
> > > > > > atomic_t variable with the provided mask.
> > > > > > 
> > > > > > Requested-by: Jan Beulich <jbeulich@suse.com>
> > > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > > > > > ---
> > > > > >     xen/include/asm-arm/arm32/atomic.h | 17 +++++++++++++++++
> > > > > >     xen/include/asm-arm/arm64/atomic.h | 14 ++++++++++++++
> > > > > >     xen/include/asm-x86/atomic.h       |  8 ++++++++
> > > > > >     3 files changed, 39 insertions(+)
> > > > > > 
> > > > > > diff --git a/xen/include/asm-arm/arm32/atomic.h b/xen/include/asm-arm/arm32/atomic.h
> > > > > > index c03eb684cd..4637381bcc 100644
> > > > > > --- a/xen/include/asm-arm/arm32/atomic.h
> > > > > > +++ b/xen/include/asm-arm/arm32/atomic.h
> > > > > > @@ -96,6 +96,23 @@ static inline int atomic_sub_return(int i, atomic_t *v)
> > > > > >     	return result;
> > > > > >     }
> > > > > > +static inline void atomic_and(unsigned int m, atomic_t *v)
> > > > > 
> > > > > All the atomic helpers have taken a signed int so far because the counter is
> > > > > an int. Any reason to diverge from that?
> > > > 
> > > > Since this is not an arithmetic operation I felt unsigned int was a
> > > > more suitable type to describe a bitmask: it felt weird to pass a
> > > > bitmask with type int, because signedness doesn't make sense when
> > > > referring to a mask.
> > > 
> > > At some point I would like to have macro generating all the atomics in on
> > > Arm in the same way a Linux (see asm-generic/atomic.h). This is to avoid
> > > duplication and make easy to introduce Armv8.1 LSE atomics. So I would like
> > > to avoid introducing difference in the prototype unless it is stricly
> > > necessary.
> > 
> > Why not have the macro generator function get passed the type of the
> > parameter?
> 
> It is not worth it for a simple operation and I don't want to diverge too
> much of atomics from Linux.

Hm, I'm would rather keep it as unsigned for type coherency, but if
x86 maintainers are also happy to have the type changed to int on x86
then I won't oppose to it.

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 1/2] atomic: add atomic_and operations

[Jan Beulich]

On 24.02.2020 09:43, Roger Pau Monne wrote:> --- a/xen/include/asm-x86/atomic.h
> +++ b/xen/include/asm-x86/atomic.h
> @@ -224,6 +224,14 @@ static inline int atomic_add_unless(atomic_t *v, int a, int u)
>      return c;
>  }
>  
> +static inline void atomic_and(unsigned int m, atomic_t *v)
> +{
> +    asm volatile (
> +        "lock; andl %1,%0"

I realize this is in sync with other atomic_*(), but I'd prefer if
the stray semicolon after "lock" was dropped. Without it the
assembler can actually diagnose a bad use (the destination not
being a memory operand). I'm unaware of reasons why the semicolons
would have been put there.

> +        : "=m" (*(volatile int *)&v->counter)
> +        : "ir" (m), "m" (*(volatile int *)&v->counter) );

Similarly despite its use elsewhere I'm afraid "i" is not the best
choice here for the constraint. Together with switching to plain
int as the function's first parameter type, "e" would seem better
even if the difference would only manifest for atomic64_t. As to
the choice of parameter type, Linux too uses plain int, so while
I agree with your reasoning I think I also agree with Julien to
use plain int here for consistency.

Finally, yet another improvement over existing code would be to
use just a single output "+m", with no paralleling input "m".

With the first and last, but not necessarily the middle one taken
care of (and I'd be happy to take care of them while committing)
Reviewed-by: Jan Beulich <jbeulich@suse.com>
If, otoh, you disagree on some, then let's see where we can
find common grounds.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 2/2] rwlock: allow recursive read locking when already locked in write mode

[Jan Beulich]

On 24.02.2020 09:44, Roger Pau Monne wrote:
> @@ -20,21 +21,30 @@ typedef struct {
>  #define DEFINE_RWLOCK(l) rwlock_t l = RW_LOCK_UNLOCKED
>  #define rwlock_init(l) (*(l) = (rwlock_t)RW_LOCK_UNLOCKED)
>  
> -/*
> - * Writer states & reader shift and bias.
> - *
> - * Writer field is 8 bit to allow for potential optimisation, see
> - * _write_unlock().
> - */
> -#define    _QW_WAITING  1               /* A writer is waiting     */
> -#define    _QW_LOCKED   0xff            /* A writer holds the lock */
> -#define    _QW_WMASK    0xff            /* Writer mask.*/
> -#define    _QR_SHIFT    8               /* Reader count shift      */
> +/* Writer states & reader shift and bias. */
> +#define    _QW_CPUMASK  0xfff               /* Writer CPU mask */
> +#define    _QW_SHIFT    12                  /* Writer flags shift */
> +#define    _QW_WAITING  (1u << _QW_SHIFT)   /* A writer is waiting */
> +#define    _QW_LOCKED   (3u << _QW_SHIFT)   /* A writer holds the lock */
> +#define    _QW_WMASK    (3u << _QW_SHIFT)   /* Writer mask */
> +#define    _QR_SHIFT    14                  /* Reader count shift */

In particular with the suggested change of atomic_and()'s first
parameter's type, perhaps the u suffixes want dropping here?

> +static inline bool _is_write_locked_by_me(uint32_t cnts)

Both here and ...

> +{
> +    BUILD_BUG_ON(_QW_CPUMASK < NR_CPUS);
> +    return (cnts & _QW_WMASK) == _QW_LOCKED &&
> +           (cnts & _QW_CPUMASK) == smp_processor_id();
> +}
> +
> +static inline bool _can_read_lock(uint32_t cnts)

... here, is a fixed width type really needed? I'd prefer if
"unsigned int" was used, and if eventually we'd then also
replace ...

> @@ -45,10 +55,10 @@ static inline int _read_trylock(rwlock_t *lock)
>      u32 cnts;

... this and ...

> @@ -64,7 +74,7 @@ static inline void _read_lock(rwlock_t *lock)
>      u32 cnts;

... this (and possible others).

> @@ -115,6 +125,11 @@ static inline int _rw_is_locked(rwlock_t *lock)
>      return atomic_read(&lock->cnts);
>  }
>  
> +static inline uint32_t _write_lock_val(void)

Same here then.

With these taken care of (as long as you agree, and likely doable
again while committing)
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 2/2] rwlock: allow recursive read locking when already locked in write mode

[Julien Grall]

On 24/02/2020 10:10, Roger Pau Monné wrote:
> On Mon, Feb 24, 2020 at 10:05:39AM +0000, Julien Grall wrote:
>> Hi Roger,
>>
>> On 24/02/2020 08:44, Roger Pau Monne wrote:
>>> Allow a CPU already holding the lock in write mode to also lock it in
>>> read mode. There's no harm in allowing read locking a rwlock that's
>>> already owned by the caller (ie: CPU) in write mode. Allowing such
>>> accesses is required at least for the CPU maps use-case.
>>>
>>> In order to do this reserve 12bits of the lock, this allows to support
>>> up to 4096 CPUs. Also reduce the write lock mask to 2 bits: one to
>>> signal there are pending writers waiting on the lock and the other to
>>> signal the lock is owned in write mode.
>>>
>>> This reduces the maximum number of concurrent readers from 16777216 to
>>> 262144, I think this should still be enough, or else the lock field
>>> can be expanded from 32 to 64bits if all architectures support atomic
>>> operations on 64bit integers.
>>>
>>> Fixes: 5872c83b42c608 ('smp: convert the cpu maps lock into a rw lock')
>>> Reported-by: Jan Beulich <jbeulich@suse.com>
>>> Reported-by: Jürgen Groß <jgross@suse.com>
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>> ---
>>> Changes since v2:
>>>    - Use atomic_and to clear the writers part of the lock when
>>>      write-unlocking.
>>>    - Reduce writer-related area to 14bits.
>>>    - Include xen/smp.h for Arm64.
>>
>> OOI, is it to use smp_processor_id()?
> 
> Yes, or else I would get errors when building asm-offsets on Arm64
> IIRC.

Thank you for the confirmation.

Reviewed-by: Julien Grall <julien@xen.org>

Cheers,

> 
> Thanks, Roger.
> 

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 1/2] atomic: add atomic_and operations

[Roger Pau Monné]

On Tue, Feb 25, 2020 at 04:12:56PM +0100, Jan Beulich wrote:
> On 24.02.2020 09:43, Roger Pau Monne wrote:> --- a/xen/include/asm-x86/atomic.h
> > +++ b/xen/include/asm-x86/atomic.h
> > @@ -224,6 +224,14 @@ static inline int atomic_add_unless(atomic_t *v, int a, int u)
> >      return c;
> >  }
> >  
> > +static inline void atomic_and(unsigned int m, atomic_t *v)
> > +{
> > +    asm volatile (
> > +        "lock; andl %1,%0"
> 
> I realize this is in sync with other atomic_*(), but I'd prefer if
> the stray semicolon after "lock" was dropped. Without it the
> assembler can actually diagnose a bad use (the destination not
> being a memory operand). I'm unaware of reasons why the semicolons
> would have been put there.
> 
> > +        : "=m" (*(volatile int *)&v->counter)
> > +        : "ir" (m), "m" (*(volatile int *)&v->counter) );
> 
> Similarly despite its use elsewhere I'm afraid "i" is not the best
> choice here for the constraint. Together with switching to plain
> int as the function's first parameter type, "e" would seem better
> even if the difference would only manifest for atomic64_t.

Well, "e" is indeed specific to x86 32bit integers, but since we are
already using "andl" I guess using "i" is equally fine?

I don't have a preference really, so if you prefer "e" I'm fine to
have it changed.

> As to
> the choice of parameter type, Linux too uses plain int, so while
> I agree with your reasoning I think I also agree with Julien to
> use plain int here for consistency.

Ack.

> Finally, yet another improvement over existing code would be to
> use just a single output "+m", with no paralleling input "m".

Oh, sure.

> With the first and last, but not necessarily the middle one taken
> care of (and I'd be happy to take care of them while committing)
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> If, otoh, you disagree on some, then let's see where we can
> find common grounds.

Thanks, that's fine, please take care while committing if you don't
mind.

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 2/2] rwlock: allow recursive read locking when already locked in write mode

[Roger Pau Monné]

On Tue, Feb 25, 2020 at 04:23:34PM +0100, Jan Beulich wrote:
> On 24.02.2020 09:44, Roger Pau Monne wrote:
> > @@ -20,21 +21,30 @@ typedef struct {
> >  #define DEFINE_RWLOCK(l) rwlock_t l = RW_LOCK_UNLOCKED
> >  #define rwlock_init(l) (*(l) = (rwlock_t)RW_LOCK_UNLOCKED)
> >  
> > -/*
> > - * Writer states & reader shift and bias.
> > - *
> > - * Writer field is 8 bit to allow for potential optimisation, see
> > - * _write_unlock().
> > - */
> > -#define    _QW_WAITING  1               /* A writer is waiting     */
> > -#define    _QW_LOCKED   0xff            /* A writer holds the lock */
> > -#define    _QW_WMASK    0xff            /* Writer mask.*/
> > -#define    _QR_SHIFT    8               /* Reader count shift      */
> > +/* Writer states & reader shift and bias. */
> > +#define    _QW_CPUMASK  0xfff               /* Writer CPU mask */
> > +#define    _QW_SHIFT    12                  /* Writer flags shift */
> > +#define    _QW_WAITING  (1u << _QW_SHIFT)   /* A writer is waiting */
> > +#define    _QW_LOCKED   (3u << _QW_SHIFT)   /* A writer holds the lock */
> > +#define    _QW_WMASK    (3u << _QW_SHIFT)   /* Writer mask */
> > +#define    _QR_SHIFT    14                  /* Reader count shift */
> 
> In particular with the suggested change of atomic_and()'s first
> parameter's type, perhaps the u suffixes want dropping here?

That would be fine. But seeing as we are planning on handling the
result of atomic_read as an unsigned int I'm not sure if it wold be
better to also keep those as unsigned ints.

> > +static inline bool _is_write_locked_by_me(uint32_t cnts)
> 
> Both here and ...
> 
> > +{
> > +    BUILD_BUG_ON(_QW_CPUMASK < NR_CPUS);
> > +    return (cnts & _QW_WMASK) == _QW_LOCKED &&
> > +           (cnts & _QW_CPUMASK) == smp_processor_id();
> > +}
> > +
> > +static inline bool _can_read_lock(uint32_t cnts)
> 
> ... here, is a fixed width type really needed? I'd prefer if
> "unsigned int" was used, and if eventually we'd then also
> replace ...

The code uniformly uses uint32_t as the cnts type, I'm fine with
switching to unsigned int, I've used uint32_t to keep it coherent with
the rest of the code.

> > @@ -45,10 +55,10 @@ static inline int _read_trylock(rwlock_t *lock)
> >      u32 cnts;
> 
> ... this and ...
> 
> > @@ -64,7 +74,7 @@ static inline void _read_lock(rwlock_t *lock)
> >      u32 cnts;
> 
> ... this (and possible others).
> 
> > @@ -115,6 +125,11 @@ static inline int _rw_is_locked(rwlock_t *lock)
> >      return atomic_read(&lock->cnts);
> >  }
> >  
> > +static inline uint32_t _write_lock_val(void)
> 
> Same here then.
> 
> With these taken care of (as long as you agree, and likely doable
> again while committing)
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks, feel free to adjust on commit, or else I can send a new
version.

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 1/2] atomic: add atomic_and operations

[Jan Beulich]

On 24.02.2020 11:29, Julien Grall wrote:
> On 24/02/2020 10:23, Roger Pau Monné wrote:
>> On Mon, Feb 24, 2020 at 10:19:44AM +0000, Julien Grall wrote:
>>> On 24/02/2020 10:09, Roger Pau Monné wrote:
>>>> On Mon, Feb 24, 2020 at 10:02:53AM +0000, Julien Grall wrote:
>>>>> On 24/02/2020 08:43, Roger Pau Monne wrote:
>>>>>> --- a/xen/include/asm-arm/arm32/atomic.h
>>>>>> +++ b/xen/include/asm-arm/arm32/atomic.h
>>>>>> @@ -96,6 +96,23 @@ static inline int atomic_sub_return(int i, atomic_t *v)
>>>>>>     	return result;
>>>>>>     }
>>>>>> +static inline void atomic_and(unsigned int m, atomic_t *v)
>>>>>
>>>>> All the atomic helpers have taken a signed int so far because the counter is
>>>>> an int. Any reason to diverge from that?
>>>>
>>>> Since this is not an arithmetic operation I felt unsigned int was a
>>>> more suitable type to describe a bitmask: it felt weird to pass a
>>>> bitmask with type int, because signedness doesn't make sense when
>>>> referring to a mask.
>>>
>>> At some point I would like to have macro generating all the atomics in on
>>> Arm in the same way a Linux (see asm-generic/atomic.h). This is to avoid
>>> duplication and make easy to introduce Armv8.1 LSE atomics. So I would like
>>> to avoid introducing difference in the prototype unless it is stricly
>>> necessary.
>>
>> Why not have the macro generator function get passed the type of the
>> parameter?
> 
> It is not worth it for a simple operation and I don't want to diverge 
> too much of atomics from Linux.

So, having reached agreement to use plain int, would you be willing
to give your ack provided the adjustments get made while committing,
to save another round trip?

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 1/2] atomic: add atomic_and operations

[Julien Grall]

On 25/02/2020 16:15, Jan Beulich wrote:
> On 24.02.2020 11:29, Julien Grall wrote:
>> On 24/02/2020 10:23, Roger Pau Monné wrote:
>>> On Mon, Feb 24, 2020 at 10:19:44AM +0000, Julien Grall wrote:
>>>> On 24/02/2020 10:09, Roger Pau Monné wrote:
>>>>> On Mon, Feb 24, 2020 at 10:02:53AM +0000, Julien Grall wrote:
>>>>>> On 24/02/2020 08:43, Roger Pau Monne wrote:
>>>>>>> --- a/xen/include/asm-arm/arm32/atomic.h
>>>>>>> +++ b/xen/include/asm-arm/arm32/atomic.h
>>>>>>> @@ -96,6 +96,23 @@ static inline int atomic_sub_return(int i, atomic_t *v)
>>>>>>>      	return result;
>>>>>>>      }
>>>>>>> +static inline void atomic_and(unsigned int m, atomic_t *v)
>>>>>>
>>>>>> All the atomic helpers have taken a signed int so far because the counter is
>>>>>> an int. Any reason to diverge from that?
>>>>>
>>>>> Since this is not an arithmetic operation I felt unsigned int was a
>>>>> more suitable type to describe a bitmask: it felt weird to pass a
>>>>> bitmask with type int, because signedness doesn't make sense when
>>>>> referring to a mask.
>>>>
>>>> At some point I would like to have macro generating all the atomics in on
>>>> Arm in the same way a Linux (see asm-generic/atomic.h). This is to avoid
>>>> duplication and make easy to introduce Armv8.1 LSE atomics. So I would like
>>>> to avoid introducing difference in the prototype unless it is stricly
>>>> necessary.
>>>
>>> Why not have the macro generator function get passed the type of the
>>> parameter?
>>
>> It is not worth it for a simple operation and I don't want to diverge
>> too much of atomics from Linux.
> 
> So, having reached agreement to use plain int, would you be willing
> to give your ack provided the adjustments get made while committing,
> to save another round trip?

Yes, the implementation of the atomic for arm looked correct:

Reviewed-by: Julien Grall <julien@xen.org>

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel