Re: [Xen-devel] [PATCH 1/2] asm/atomic.h: common prototyping (add xen/atomic.h)

2016-07-13 Thread Andrew Cooper 
On 13/07/16 14:20, Corneliu ZUZU wrote:
> On 7/13/2016 3:12 PM, Andrew Cooper wrote:
>> On 13/07/16 12:23, Corneliu ZUZU wrote:
>>> +typedef struct { int counter; } atomic_t;
>>> +
>>> +#define ATOMIC_INIT(i) { (i) }
>>> +
>>> +/**
>>> + * atomic_read - read atomic variable
>>> + * @v: pointer of type atomic_t
>>> + *
>>> + * Atomically reads the value of @v.
>>> + */
>>> +static inline int atomic_read(atomic_t *v);
>>> +
>>> +/**
>>> + * _atomic_read - read atomic variable non-atomically
>>> + * @v atomic_t
>>> + *
>>> + * Non-atomically reads the value of @v
>>> + */
>>> +static inline int _atomic_read(atomic_t v);
>>> +
>>> +/**
>>> + * atomic_set - set atomic variable
>>> + * @v: pointer of type atomic_t
>>> + * @i: required value
>>> + *
>>> + * Atomically sets the value of @v to @i.
>>> + */
>>> +static inline void atomic_set(atomic_t *v, int i);
>>> +
>>> +/**
>>> + * _atomic_set - set atomic variable non-atomically
>>> + * @v: pointer of type atomic_t
>>> + * @i: required value
>>> + *
>>> + * Non-atomically sets the value of @v to @i.
>>> + */
>>> +static inline void _atomic_set(atomic_t *v, int i);
>>> +
>>
>
> Is it ok if I also omit repeating the comments which are already @
> xen/atomic.h from the arch-side asm/atomic.h ?

Yes - that should be fine.  No point having identical comments twice.

~Andrew

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 1/2] asm/atomic.h: common prototyping (add xen/atomic.h)

2016-07-13 Thread Corneliu ZUZU 

On 7/13/2016 3:12 PM, Andrew Cooper wrote:

On 13/07/16 12:23, Corneliu ZUZU wrote:

+typedef struct { int counter; } atomic_t;
+
+#define ATOMIC_INIT(i) { (i) }
+
+/**
+ * atomic_read - read atomic variable
+ * @v: pointer of type atomic_t
+ *
+ * Atomically reads the value of @v.
+ */
+static inline int atomic_read(atomic_t *v);
+
+/**
+ * _atomic_read - read atomic variable non-atomically
+ * @v atomic_t
+ *
+ * Non-atomically reads the value of @v
+ */
+static inline int _atomic_read(atomic_t v);
+
+/**
+ * atomic_set - set atomic variable
+ * @v: pointer of type atomic_t
+ * @i: required value
+ *
+ * Atomically sets the value of @v to @i.
+ */
+static inline void atomic_set(atomic_t *v, int i);
+
+/**
+ * _atomic_set - set atomic variable non-atomically
+ * @v: pointer of type atomic_t
+ * @i: required value
+ *
+ * Non-atomically sets the value of @v to @i.
+ */
+static inline void _atomic_set(atomic_t *v, int i);
+




Is it ok if I also omit repeating the comments which are already @ 
xen/atomic.h from the arch-side asm/atomic.h ?


Thanks,
Zuzu C.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 1/2] asm/atomic.h: common prototyping (add xen/atomic.h)

2016-07-13 Thread Corneliu ZUZU 

Hi Julien,

On 7/13/2016 3:14 PM, Julien Grall wrote:

Hi Corneliu,

On 13/07/2016 12:23, Corneliu ZUZU wrote:
Following Andrew Cooper's suggestion, create a common-side 
 to
establish, among others, prototypes of atomic functions called from 
common-code.
Done to avoid introducing inconsistencies between arch-side 


headers when we make subtle changes to one of them.

Some arm-side macros had to be turned into inline functions in the 
process.


You forgot to mention that you moved the code from 
asm-arm/arm{32,64}/atomic.h to asm-arm/arm.h.


Furthermore, this change should really be a separate patch.


Noted, will do.





Also includes a minor adjustment asm-x86/atomic.h: reorder 
atomic_inc_and_test()

to follow after atomic_inc().

Signed-off-by: Corneliu ZUZU 


[...]


diff --git a/xen/include/asm-arm/atomic.h b/xen/include/asm-arm/atomic.h
index 29ab265..8e8c5d1 100644
--- a/xen/include/asm-arm/atomic.h
+++ b/xen/include/asm-arm/atomic.h
@@ -2,6 +2,7 @@
 #define __ARCH_ARM_ATOMIC__

 #include 
+#include 
 #include 
 #include 

@@ -95,15 +96,6 @@ void __bad_atomic_size(void);
 default: __bad_atomic_size(); 
break;\

} \
 })
-
-/*
- * NB. I've pushed the volatile qualifier into the operations. This 
allows
- * fast accessors such as _atomic_read() and _atomic_set() which 
don't give

- * the compiler a fit.
- */
-typedef struct { int counter; } atomic_t;
-
-#define ATOMIC_INIT(i) { (i) }

 /*
  * On ARM, ordinary assignment (str instruction) doesn't clear the 
local

@@ -138,6 +130,85 @@ static inline void _atomic_set(atomic_t *v, int i)
 # error "unknown ARM variant"
 #endif

+#define atomic_inc_return(v)(atomic_add_return(1, v))
+#define atomic_dec_return(v)(atomic_sub_return(1, v))
+
+/**
+ * atomic_sub_and_test - subtract value from variable and test result
+ * @i: integer value to subtract
+ * @v: pointer of type atomic_t
+ *
+ * Atomically subtracts @i from @v and returns
+ * true if the result is zero, or false for all
+ * other cases.
+ */
+static inline int atomic_sub_and_test(int i, atomic_t *v)
+{
+return 0 == atomic_sub_return(i, v);
+}


Please don't use yoda condition. It's less readable and compiler have 
safety nowadays to prevent using "=".


Oh yeah, given that Xen's compiled with -Wall -Werror that's not 
necessary. Ack.





+
+/**
+ * atomic_inc - increment atomic variable
+ * @v: pointer of type atomic_t
+ *
+ * Atomically increments @v by 1.
+ */
+static inline void atomic_inc(atomic_t *v)
+{
+atomic_add(1, v);
+}


Regards,


Thanks,
Corneliu.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 1/2] asm/atomic.h: common prototyping (add xen/atomic.h)

2016-07-13 Thread Corneliu ZUZU 

On 7/13/2016 3:12 PM, Andrew Cooper wrote:

On 13/07/16 12:23, Corneliu ZUZU wrote:

Following Andrew Cooper's suggestion, create a common-side  to
establish, among others, prototypes of atomic functions called from common-code.
Done to avoid introducing inconsistencies between arch-side 
headers when we make subtle changes to one of them.

Some arm-side macros had to be turned into inline functions in the process.

Also includes a minor adjustment asm-x86/atomic.h: reorder atomic_inc_and_test()
to follow after atomic_inc().

Suggested-by: Andrew Cooper 


Nice, didn't know that.





Signed-off-by: Corneliu ZUZU 

Thanks for doing this!


diff --git a/xen/include/xen/atomic.h b/xen/include/xen/atomic.h
new file mode 100644
index 000..5d5c051
--- /dev/null
+++ b/xen/include/xen/atomic.h
@@ -0,0 +1,155 @@
+/*
+ * include/xen/atomic.h
+ *
+ * Common atomic operations entities (atomic_t, function prototypes).
+ * Include _from_ arch-side .
+ *
+ * Copyright (c) 2016 Bitdefender S.R.L.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; If not, see .
+ */
+
+#ifndef __XEN_ATOMIC_H__
+#define __XEN_ATOMIC_H__
+
+/*
+ * NB. I've pushed the volatile qualifier into the operations. This allows
+ * fast accessors such as _atomic_read() and _atomic_set() which don't give
+ * the compiler a fit.
+ */

I would recommend simply dropping this paragraph.  Is is very out of date.


Ack.




+typedef struct { int counter; } atomic_t;
+
+#define ATOMIC_INIT(i) { (i) }
+
+/**
+ * atomic_read - read atomic variable
+ * @v: pointer of type atomic_t
+ *
+ * Atomically reads the value of @v.
+ */
+static inline int atomic_read(atomic_t *v);
+
+/**
+ * _atomic_read - read atomic variable non-atomically
+ * @v atomic_t
+ *
+ * Non-atomically reads the value of @v
+ */
+static inline int _atomic_read(atomic_t v);
+
+/**
+ * atomic_set - set atomic variable
+ * @v: pointer of type atomic_t
+ * @i: required value
+ *
+ * Atomically sets the value of @v to @i.
+ */
+static inline void atomic_set(atomic_t *v, int i);
+
+/**
+ * _atomic_set - set atomic variable non-atomically
+ * @v: pointer of type atomic_t
+ * @i: required value
+ *
+ * Non-atomically sets the value of @v to @i.
+ */
+static inline void _atomic_set(atomic_t *v, int i);
+

/**
  * atomic_cmpxchg - compare and exchange an atomic variable
  *...
  */


Ack.




+static inline int atomic_cmpxchg(atomic_t *v, int old, int new);

With those changes, Reviewed-by: Andrew Cooper 


Thanks too for the prompt review,
Zuzu C.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 1/2] asm/atomic.h: common prototyping (add xen/atomic.h)

2016-07-13 Thread Julien Grall 

Hi Corneliu,

On 13/07/2016 12:23, Corneliu ZUZU wrote:

Following Andrew Cooper's suggestion, create a common-side  to
establish, among others, prototypes of atomic functions called from common-code.
Done to avoid introducing inconsistencies between arch-side 
headers when we make subtle changes to one of them.

Some arm-side macros had to be turned into inline functions in the process.


You forgot to mention that you moved the code from 
asm-arm/arm{32,64}/atomic.h to asm-arm/arm.h.


Furthermore, this change should really be a separate patch.



Also includes a minor adjustment asm-x86/atomic.h: reorder atomic_inc_and_test()
to follow after atomic_inc().

Signed-off-by: Corneliu ZUZU 


[...]


diff --git a/xen/include/asm-arm/atomic.h b/xen/include/asm-arm/atomic.h
index 29ab265..8e8c5d1 100644
--- a/xen/include/asm-arm/atomic.h
+++ b/xen/include/asm-arm/atomic.h
@@ -2,6 +2,7 @@
 #define __ARCH_ARM_ATOMIC__

 #include 
+#include 
 #include 
 #include 

@@ -95,15 +96,6 @@ void __bad_atomic_size(void);
 default: __bad_atomic_size(); break;\
 }   \
 })
-
-/*
- * NB. I've pushed the volatile qualifier into the operations. This allows
- * fast accessors such as _atomic_read() and _atomic_set() which don't give
- * the compiler a fit.
- */
-typedef struct { int counter; } atomic_t;
-
-#define ATOMIC_INIT(i) { (i) }

 /*
  * On ARM, ordinary assignment (str instruction) doesn't clear the local
@@ -138,6 +130,85 @@ static inline void _atomic_set(atomic_t *v, int i)
 # error "unknown ARM variant"
 #endif

+#define atomic_inc_return(v)(atomic_add_return(1, v))
+#define atomic_dec_return(v)(atomic_sub_return(1, v))
+
+/**
+ * atomic_sub_and_test - subtract value from variable and test result
+ * @i: integer value to subtract
+ * @v: pointer of type atomic_t
+ *
+ * Atomically subtracts @i from @v and returns
+ * true if the result is zero, or false for all
+ * other cases.
+ */
+static inline int atomic_sub_and_test(int i, atomic_t *v)
+{
+return 0 == atomic_sub_return(i, v);
+}


Please don't use yoda condition. It's less readable and compiler have 
safety nowadays to prevent using "=".



+
+/**
+ * atomic_inc - increment atomic variable
+ * @v: pointer of type atomic_t
+ *
+ * Atomically increments @v by 1.
+ */
+static inline void atomic_inc(atomic_t *v)
+{
+atomic_add(1, v);
+}


Regards,


--
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 1/2] asm/atomic.h: common prototyping (add xen/atomic.h)

2016-07-13 Thread Andrew Cooper 
On 13/07/16 12:23, Corneliu ZUZU wrote:
> Following Andrew Cooper's suggestion, create a common-side  to
> establish, among others, prototypes of atomic functions called from 
> common-code.
> Done to avoid introducing inconsistencies between arch-side 
> headers when we make subtle changes to one of them.
>
> Some arm-side macros had to be turned into inline functions in the process.
>
> Also includes a minor adjustment asm-x86/atomic.h: reorder 
> atomic_inc_and_test()
> to follow after atomic_inc().

Suggested-by: Andrew Cooper 


> Signed-off-by: Corneliu ZUZU 

Thanks for doing this!

> diff --git a/xen/include/xen/atomic.h b/xen/include/xen/atomic.h
> new file mode 100644
> index 000..5d5c051
> --- /dev/null
> +++ b/xen/include/xen/atomic.h
> @@ -0,0 +1,155 @@
> +/*
> + * include/xen/atomic.h
> + *
> + * Common atomic operations entities (atomic_t, function prototypes).
> + * Include _from_ arch-side .
> + *
> + * Copyright (c) 2016 Bitdefender S.R.L.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along 
> with
> + * this program; If not, see .
> + */
> +
> +#ifndef __XEN_ATOMIC_H__
> +#define __XEN_ATOMIC_H__
> +
> +/*
> + * NB. I've pushed the volatile qualifier into the operations. This allows
> + * fast accessors such as _atomic_read() and _atomic_set() which don't give
> + * the compiler a fit.
> + */

I would recommend simply dropping this paragraph.  Is is very out of date.

> +typedef struct { int counter; } atomic_t;
> +
> +#define ATOMIC_INIT(i) { (i) }
> +
> +/**
> + * atomic_read - read atomic variable
> + * @v: pointer of type atomic_t
> + *
> + * Atomically reads the value of @v.
> + */
> +static inline int atomic_read(atomic_t *v);
> +
> +/**
> + * _atomic_read - read atomic variable non-atomically
> + * @v atomic_t
> + *
> + * Non-atomically reads the value of @v
> + */
> +static inline int _atomic_read(atomic_t v);
> +
> +/**
> + * atomic_set - set atomic variable
> + * @v: pointer of type atomic_t
> + * @i: required value
> + *
> + * Atomically sets the value of @v to @i.
> + */
> +static inline void atomic_set(atomic_t *v, int i);
> +
> +/**
> + * _atomic_set - set atomic variable non-atomically
> + * @v: pointer of type atomic_t
> + * @i: required value
> + *
> + * Non-atomically sets the value of @v to @i.
> + */
> +static inline void _atomic_set(atomic_t *v, int i);
> +

/**
 * atomic_cmpxchg - compare and exchange an atomic variable
 *...
 */

> +static inline int atomic_cmpxchg(atomic_t *v, int old, int new);

With those changes, Reviewed-by: Andrew Cooper 

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [PATCH 1/2] asm/atomic.h: common prototyping (add xen/atomic.h)

2016-07-13 Thread Corneliu ZUZU 
Following Andrew Cooper's suggestion, create a common-side  to
establish, among others, prototypes of atomic functions called from common-code.
Done to avoid introducing inconsistencies between arch-side 
headers when we make subtle changes to one of them.

Some arm-side macros had to be turned into inline functions in the process.

Also includes a minor adjustment asm-x86/atomic.h: reorder atomic_inc_and_test()
to follow after atomic_inc().

Signed-off-by: Corneliu ZUZU 
---
 xen/include/asm-arm/arm32/atomic.h |  11 ---
 xen/include/asm-arm/arm64/atomic.h |  11 ---
 xen/include/asm-arm/atomic.h   |  89 ++---
 xen/include/asm-x86/atomic.h   |  49 +---
 xen/include/xen/atomic.h   | 155 +
 5 files changed, 255 insertions(+), 60 deletions(-)
 create mode 100644 xen/include/xen/atomic.h

diff --git a/xen/include/asm-arm/arm32/atomic.h 
b/xen/include/asm-arm/arm32/atomic.h
index 7ec712f..be08ff1 100644
--- a/xen/include/asm-arm/arm32/atomic.h
+++ b/xen/include/asm-arm/arm32/atomic.h
@@ -149,17 +149,6 @@ static inline int __atomic_add_unless(atomic_t *v, int a, 
int u)
 
 #define atomic_xchg(v, new) (xchg(&((v)->counter), new))
 
-#define atomic_inc(v)  atomic_add(1, v)
-#define atomic_dec(v)  atomic_sub(1, v)
-
-#define atomic_inc_and_test(v) (atomic_add_return(1, v) == 0)
-#define atomic_dec_and_test(v) (atomic_sub_return(1, v) == 0)
-#define atomic_inc_return(v)(atomic_add_return(1, v))
-#define atomic_dec_return(v)(atomic_sub_return(1, v))
-#define atomic_sub_and_test(i, v) (atomic_sub_return(i, v) == 0)
-
-#define atomic_add_negative(i,v) (atomic_add_return(i, v) < 0)
-
 #endif /* __ARCH_ARM_ARM32_ATOMIC__ */
 /*
  * Local variables:
diff --git a/xen/include/asm-arm/arm64/atomic.h 
b/xen/include/asm-arm/arm64/atomic.h
index b49219e..80d07bf 100644
--- a/xen/include/asm-arm/arm64/atomic.h
+++ b/xen/include/asm-arm/arm64/atomic.h
@@ -125,17 +125,6 @@ static inline int __atomic_add_unless(atomic_t *v, int a, 
int u)
return c;
 }
 
-#define atomic_inc(v)  atomic_add(1, v)
-#define atomic_dec(v)  atomic_sub(1, v)
-
-#define atomic_inc_and_test(v) (atomic_add_return(1, v) == 0)
-#define atomic_dec_and_test(v) (atomic_sub_return(1, v) == 0)
-#define atomic_inc_return(v)(atomic_add_return(1, v))
-#define atomic_dec_return(v)(atomic_sub_return(1, v))
-#define atomic_sub_and_test(i, v) (atomic_sub_return(i, v) == 0)
-
-#define atomic_add_negative(i,v) (atomic_add_return(i, v) < 0)
-
 #endif
 /*
  * Local variables:
diff --git a/xen/include/asm-arm/atomic.h b/xen/include/asm-arm/atomic.h
index 29ab265..8e8c5d1 100644
--- a/xen/include/asm-arm/atomic.h
+++ b/xen/include/asm-arm/atomic.h
@@ -2,6 +2,7 @@
 #define __ARCH_ARM_ATOMIC__
 
 #include 
+#include 
 #include 
 #include 
 
@@ -95,15 +96,6 @@ void __bad_atomic_size(void);
 default: __bad_atomic_size(); break;\
 }   \
 })
-
-/*
- * NB. I've pushed the volatile qualifier into the operations. This allows
- * fast accessors such as _atomic_read() and _atomic_set() which don't give
- * the compiler a fit.
- */
-typedef struct { int counter; } atomic_t;
-
-#define ATOMIC_INIT(i) { (i) }
 
 /*
  * On ARM, ordinary assignment (str instruction) doesn't clear the local
@@ -138,6 +130,85 @@ static inline void _atomic_set(atomic_t *v, int i)
 # error "unknown ARM variant"
 #endif
 
+#define atomic_inc_return(v)(atomic_add_return(1, v))
+#define atomic_dec_return(v)(atomic_sub_return(1, v))
+
+/**
+ * atomic_sub_and_test - subtract value from variable and test result
+ * @i: integer value to subtract
+ * @v: pointer of type atomic_t
+ *
+ * Atomically subtracts @i from @v and returns
+ * true if the result is zero, or false for all
+ * other cases.
+ */
+static inline int atomic_sub_and_test(int i, atomic_t *v)
+{
+return 0 == atomic_sub_return(i, v);
+}
+
+/**
+ * atomic_inc - increment atomic variable
+ * @v: pointer of type atomic_t
+ *
+ * Atomically increments @v by 1.
+ */
+static inline void atomic_inc(atomic_t *v)
+{
+atomic_add(1, v);
+}
+
+/**
+ * atomic_inc_and_test - increment and test
+ * @v: pointer of type atomic_t
+ *
+ * Atomically increments @v by 1
+ * and returns true if the result is zero, or false for all
+ * other cases.
+ */
+static inline int atomic_inc_and_test(atomic_t *v)
+{
+return 0 == atomic_add_return(1, v);
+}
+
+/**
+ * atomic_dec - decrement atomic variable
+ * @v: pointer of type atomic_t
+ *
+ * Atomically decrements @v by 1.
+ */
+static inline void atomic_dec(atomic_t *v)
+{
+atomic_sub(1, v);
+}
+
+/**
+ * atomic_dec_and_test - decrement and test
+ * @v: pointer of type atomic_t
+ *
+ * Atomically decrements @v by 1 and
+ * returns true if the result is 0, or false for all other
+ * cases.
+ */
+static inline int atomic_dec_and_test(atomic_t *v)
+{
+