[PATCH] Xen/atomic: use static inlines instead of macros

[PATCH] Xen/atomic: use static inlines instead of macros

[Andrew Cooper]

This is some coverity-inspired tidying.

Coverity has some grief analysing the call sites of atomic_read().  This is
believed to be a bug in Coverity itself when expanding the nested macros, but
there is no legitimate reason for it to be a macro in the first place.

This patch changes {,_}atomic_{read,set}() from being macros to being static
inline functions, thus gaining some type safety.

One issue which is not immediatly obvious is that the non-atomic varients take
their atomic_t at a different level of indirection to the atomic varients.

This is not suitable for _atomic_set() (when used to initialise an atomic_t)
which is converted to take its parameter as a pointer.  One callsite of
_atomic_set() is updated, while the other two callsites are updated to
ATOMIC_INIT().

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Stefano Stabellini <stefano.stabellini@citrix.com>
CC: Tim Deegan <tim@xen.org>

---

This is compile-tested on arm32 and 64, and functionally tested on x86_64
---
 xen/common/domain.c          |    5 ++---
 xen/include/asm-arm/atomic.h |   22 +++++++++++++++++----
 xen/include/asm-x86/atomic.h |   43 +++++++++++++++++++++++++++++++++++-------
 xen/include/xen/sched.h      |    2 +-
 4 files changed, 57 insertions(+), 15 deletions(-)

diff --git a/xen/common/domain.c b/xen/common/domain.c
index 2636fc9..a1483e4 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -759,13 +759,12 @@ static void complete_domain_destroy(struct rcu_head *head)
 void domain_destroy(struct domain *d)
 {
     struct domain **pd;
-    atomic_t      old, new;
+    atomic_t old = ATOMIC_INIT(0);
+    atomic_t new = ATOMIC_INIT(DOMAIN_DESTROYED);
 
     BUG_ON(!d->is_dying);
 
     /* May be already destroyed, or get_domain() can race us. */
-    _atomic_set(old, 0);
-    _atomic_set(new, DOMAIN_DESTROYED);
     old = atomic_compareandswap(old, new, &d->refcnt);
     if ( _atomic_read(old) != 0 )
         return;
diff --git a/xen/include/asm-arm/atomic.h b/xen/include/asm-arm/atomic.h
index 69c8f3f..14eacd0 100644
--- a/xen/include/asm-arm/atomic.h
+++ b/xen/include/asm-arm/atomic.h
@@ -83,11 +83,25 @@ typedef struct { int counter; } atomic_t;
  * strex/ldrex monitor on some implementations. The reason we can use it for
  * atomic_set() is the clrex or dummy strex done on every exception return.
  */
-#define _atomic_read(v) ((v).counter)
-#define atomic_read(v)  (*(volatile int *)&(v)->counter)
+static inline int atomic_read(atomic_t *v)
+{
+    return *(volatile int *)&(v)->counter;
+}
+
+static inline int _atomic_read(atomic_t v)
+{
+    return v.counter;
+}
 
-#define _atomic_set(v,i) (((v).counter) = (i))
-#define atomic_set(v,i) (((v)->counter) = (i))
+static inline void atomic_set(atomic_t *v, int i)
+{
+    v->counter = i;
+}
+
+static inline void _atomic_set(atomic_t *v, int i)
+{
+    v->counter = i;
+}
 
 #if defined(CONFIG_ARM_32)
 # include <asm/arm32/atomic.h>
diff --git a/xen/include/asm-x86/atomic.h b/xen/include/asm-x86/atomic.h
index e476ab5..8972463 100644
--- a/xen/include/asm-x86/atomic.h
+++ b/xen/include/asm-x86/atomic.h
@@ -66,21 +66,50 @@ typedef struct { int counter; } atomic_t;
 /**
  * atomic_read - read atomic variable
  * @v: pointer of type atomic_t
- * 
+ *
  * Atomically reads the value of @v.
  */
-#define _atomic_read(v)  ((v).counter)
-#define atomic_read(v)   read_atomic(&((v)->counter))
+static inline int atomic_read(atomic_t *v)
+{
+    return read_atomic(&v->counter);
+}
+
+/**
+ * _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)
+{
+    return v.counter;
+}
+
 
 /**
  * atomic_set - set atomic variable
  * @v: pointer of type atomic_t
  * @i: required value
- * 
+ *
  * Atomically sets the value of @v to @i.
- */ 
-#define _atomic_set(v,i) (((v).counter) = (i))
-#define atomic_set(v,i)  write_atomic(&((v)->counter), (i))
+ */
+static inline void atomic_set(atomic_t *v, int i)
+{
+    write_atomic(&v->counter, 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)
+{
+    v->counter = i;
+}
+
 
 /**
  * atomic_add - add integer to atomic variable
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index fb8bd36..e4b5cae 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -468,7 +468,7 @@ static always_inline int get_domain(struct domain *d)
         old = seen;
         if ( unlikely(_atomic_read(old) & DOMAIN_DESTROYED) )
             return 0;
-        _atomic_set(new, _atomic_read(old) + 1);
+        _atomic_set(&new, _atomic_read(old) + 1);
         seen = atomic_compareandswap(old, new, &d->refcnt);
     }
     while ( unlikely(_atomic_read(seen) != _atomic_read(old)) );
-- 
1.7.10.4

Re: [PATCH] Xen/atomic: use static inlines instead of macros

[Jan Beulich]

>>> On 21.02.14 at 21:41, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> This is some coverity-inspired tidying.
> 
> Coverity has some grief analysing the call sites of atomic_read().  This is
> believed to be a bug in Coverity itself when expanding the nested macros, 
> but
> there is no legitimate reason for it to be a macro in the first place.
> 
> This patch changes {,_}atomic_{read,set}() from being macros to being static
> inline functions, thus gaining some type safety.
> 
> One issue which is not immediatly obvious is that the non-atomic varients 
> take
> their atomic_t at a different level of indirection to the atomic varients.
> 
> This is not suitable for _atomic_set() (when used to initialise an atomic_t)
> which is converted to take its parameter as a pointer.  One callsite of
> _atomic_set() is updated, while the other two callsites are updated to
> ATOMIC_INIT().

Did you consider leaving these "non-atomic atomic ops" untouched
(as they don't involve macro nesting), altering only the "real" ones?

Jan

Re: [PATCH] Xen/atomic: use static inlines instead of macros

[Andrew Cooper]

On 24/02/14 10:02, Jan Beulich wrote:
>>>> On 21.02.14 at 21:41, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> This is some coverity-inspired tidying.
>>
>> Coverity has some grief analysing the call sites of atomic_read().  This is
>> believed to be a bug in Coverity itself when expanding the nested macros, 
>> but
>> there is no legitimate reason for it to be a macro in the first place.
>>
>> This patch changes {,_}atomic_{read,set}() from being macros to being static
>> inline functions, thus gaining some type safety.
>>
>> One issue which is not immediatly obvious is that the non-atomic varients 
>> take
>> their atomic_t at a different level of indirection to the atomic varients.
>>
>> This is not suitable for _atomic_set() (when used to initialise an atomic_t)
>> which is converted to take its parameter as a pointer.  One callsite of
>> _atomic_set() is updated, while the other two callsites are updated to
>> ATOMIC_INIT().
> Did you consider leaving these "non-atomic atomic ops" untouched
> (as they don't involve macro nesting), altering only the "real" ones?
>
> Jan
>

Yes, but for the sake of three updates at callsites, I felt the benefits
outweighed the costs.

~Andrew

Re: [PATCH] Xen/atomic: use static inlines instead of macros

[Jan Beulich]

>>> On 24.02.14 at 11:26, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> On 24/02/14 10:02, Jan Beulich wrote:
>>>>> On 21.02.14 at 21:41, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>> This is some coverity-inspired tidying.
>>>
>>> Coverity has some grief analysing the call sites of atomic_read().  This is
>>> believed to be a bug in Coverity itself when expanding the nested macros, 
>>> but
>>> there is no legitimate reason for it to be a macro in the first place.
>>>
>>> This patch changes {,_}atomic_{read,set}() from being macros to being static
>>> inline functions, thus gaining some type safety.
>>>
>>> One issue which is not immediatly obvious is that the non-atomic varients 
>>> take
>>> their atomic_t at a different level of indirection to the atomic varients.
>>>
>>> This is not suitable for _atomic_set() (when used to initialise an atomic_t)
>>> which is converted to take its parameter as a pointer.  One callsite of
>>> _atomic_set() is updated, while the other two callsites are updated to
>>> ATOMIC_INIT().
>> Did you consider leaving these "non-atomic atomic ops" untouched
>> (as they don't involve macro nesting), altering only the "real" ones?
> 
> Yes, but for the sake of three updates at callsites, I felt the benefits
> outweighed the costs.

Except that I don't really see much of a benefit here - the type safety
argument doesn't really count all that much, considering that a wrongly
used type would need to have a suitable field named "counter", which
is unlikely enough to not worry much.

Jan

Re: [PATCH] Xen/atomic: use static inlines instead of macros

[Andrew Cooper]

On 24/02/14 10:54, Jan Beulich wrote:
>>>> On 24.02.14 at 11:26, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> On 24/02/14 10:02, Jan Beulich wrote:
>>>>>> On 21.02.14 at 21:41, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>>> This is some coverity-inspired tidying.
>>>>
>>>> Coverity has some grief analysing the call sites of atomic_read().  This is
>>>> believed to be a bug in Coverity itself when expanding the nested macros, 
>>>> but
>>>> there is no legitimate reason for it to be a macro in the first place.
>>>>
>>>> This patch changes {,_}atomic_{read,set}() from being macros to being static
>>>> inline functions, thus gaining some type safety.
>>>>
>>>> One issue which is not immediatly obvious is that the non-atomic varients 
>>>> take
>>>> their atomic_t at a different level of indirection to the atomic varients.
>>>>
>>>> This is not suitable for _atomic_set() (when used to initialise an atomic_t)
>>>> which is converted to take its parameter as a pointer.  One callsite of
>>>> _atomic_set() is updated, while the other two callsites are updated to
>>>> ATOMIC_INIT().
>>> Did you consider leaving these "non-atomic atomic ops" untouched
>>> (as they don't involve macro nesting), altering only the "real" ones?
>> Yes, but for the sake of three updates at callsites, I felt the benefits
>> outweighed the costs.
> Except that I don't really see much of a benefit here - the type safety
> argument doesn't really count all that much, considering that a wrongly
> used type would need to have a suitable field named "counter", which
> is unlikely enough to not worry much.
>
> Jan
>

An error message of "Expeted atomic_t *, got <something else>" is
substantially more useful than "<something> doesn't have .counter" or .
being an invalid operator in context.

~Andrew

Re: [PATCH] Xen/atomic: use static inlines instead of macros

[Ian Campbell]

On Fri, 2014-02-21 at 20:41 +0000, Andrew Cooper wrote:
> This is some coverity-inspired tidying.
> 
> Coverity has some grief analysing the call sites of atomic_read().  This is
> believed to be a bug in Coverity itself when expanding the nested macros, but
> there is no legitimate reason for it to be a macro in the first place.
> 
> This patch changes {,_}atomic_{read,set}() from being macros to being static
> inline functions, thus gaining some type safety.
> 
> One issue which is not immediatly obvious is that the non-atomic varients take
> their atomic_t at a different level of indirection to the atomic varients.

"variants" and "immediately"

> This is not suitable for _atomic_set() (when used to initialise an atomic_t)
> which is converted to take its parameter as a pointer.  One callsite of
> _atomic_set() is updated, while the other two callsites are updated to
> ATOMIC_INIT().
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Ian Campbell <ian.campbell@citrix.com>
> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
> CC: Tim Deegan <tim@xen.org>
> 
> ---
> 
> This is compile-tested on arm32 and 64

Thanks!

> +static inline void atomic_set(atomic_t *v, int i)
> +{
> +    v->counter = i;
> +}
> +
> +static inline void _atomic_set(atomic_t *v, int i)
> +{
> +    v->counter = i;
> +}

Are these now the same on purpose? (previously one took a pointer and
the other the actual variable).

I don't have any especially strong feelings on the patch generally. If
x86 is going to change then I suppose ARM might as well do so for
consistency.

Ian.

Re: [PATCH] Xen/atomic: use static inlines instead of macros

[Andrew Cooper]

On 24/02/14 11:50, Ian Campbell wrote:
> On Fri, 2014-02-21 at 20:41 +0000, Andrew Cooper wrote:
>> This is some coverity-inspired tidying.
>>
>> Coverity has some grief analysing the call sites of atomic_read().  This is
>> believed to be a bug in Coverity itself when expanding the nested macros, but
>> there is no legitimate reason for it to be a macro in the first place.
>>
>> This patch changes {,_}atomic_{read,set}() from being macros to being static
>> inline functions, thus gaining some type safety.
>>
>> One issue which is not immediatly obvious is that the non-atomic varients take
>> their atomic_t at a different level of indirection to the atomic varients.
> "variants" and "immediately"

Oops - I did correct these, then sent the previous patch.

>
>> This is not suitable for _atomic_set() (when used to initialise an atomic_t)
>> which is converted to take its parameter as a pointer.  One callsite of
>> _atomic_set() is updated, while the other two callsites are updated to
>> ATOMIC_INIT().
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> CC: Keir Fraser <keir@xen.org>
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Ian Campbell <ian.campbell@citrix.com>
>> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
>> CC: Tim Deegan <tim@xen.org>
>>
>> ---
>>
>> This is compile-tested on arm32 and 64
> Thanks!
>
>> +static inline void atomic_set(atomic_t *v, int i)
>> +{
>> +    v->counter = i;
>> +}
>> +
>> +static inline void _atomic_set(atomic_t *v, int i)
>> +{
>> +    v->counter = i;
>> +}
> Are these now the same on purpose? (previously one took a pointer and
> the other the actual variable).
>
> I don't have any especially strong feelings on the patch generally. If
> x86 is going to change then I suppose ARM might as well do so for
> consistency.
>
> Ian.
>
>
>

_atomic_set(stack_var, val)  makes it a functional noop, and crucially,
leaves the caller with an untouchted atomic_t

I had to change the indirection to make x86 work, before checking arm
and finding that the two were identical.  They are used in common code
where the difference is required in x86.

~Andrew