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

* [PATCH v2] Xen/atomic: use static inlines instead of macros
@ 2014-03-28 10:55 Andrew Cooper
  2014-03-28 11:05 ` Jan Beulich
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Andrew Cooper @ 2014-03-28 10:55 UTC (permalink / raw)
  To: Xen-devel
  Cc: Keir Fraser, Ian Campbell, Andrew Cooper, Tim Deegan,
	Stefano Stabellini, Jan Beulich

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 immediately obvious is that the non-atomic variants take
their atomic_t at a different level of indirection to the atomic variants.

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

v2: spelling fixes in commit message, and remove some redundant brackets
---
 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 ad8a1b6..b414a7d 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -754,13 +754,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..cb72547 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 b9ba379..92b4846 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -484,7 +484,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

^ permalink raw reply related	[flat|nested] 6+ messages in thread