Re: [Qemu-devel] [PATCH 2/5] lockable: add QemuLockable

[Paolo Bonzini]

On 03/02/2018 21:44, Richard Henderson wrote:
> On 02/03/2018 07:39 AM, Paolo Bonzini wrote:
>> +/* This function gives an error if an invalid, non-NULL pointer type is 
>> passed
>> + * to QEMU_MAKE_LOCKABLE.  For optimized builds, we can rely on dead-code 
>> elimination
>> + * from the compiler, and give the errors already at link time.
>> + */
>> +#ifdef __OPTIMIZE__
>> +void unknown_lock_type(void *);
>> +#else
>> +static inline void unknown_lock_type(void *unused)
>> +{
>> +abort();
>> +}
> 
> Since you're using __builtin_choose_expr, I'm surprised you would need to test
> __OPTIMZE__.  The nature of the builtin is such that it *must* eliminate the
> other branch; otherwise the types don't even match up.

__builtin_choose_expr works fine.  However we also have

return x ? __builtin_choose_expr(..., unknown_lock_type) : NULL;

and if "x" is NULL then its type is a void*, and the LHS will refer to
unknown_lock_type.  I was expecting __always_inline__ to be enough to
avoid this, but apparently this is not the case.

Paolo

> It might be worth using __attribute__((error("message"))) on the function too.
> 
> Otherwise,
> 
> Reviewed-by: Richard Henderson 
> 
> 
> r~
> 

Re: [Qemu-devel] [PATCH 2/5] lockable: add QemuLockable

[Richard Henderson]

On 02/03/2018 07:39 AM, Paolo Bonzini wrote:
> +/* This function gives an error if an invalid, non-NULL pointer type is 
> passed
> + * to QEMU_MAKE_LOCKABLE.  For optimized builds, we can rely on dead-code 
> elimination
> + * from the compiler, and give the errors already at link time.
> + */
> +#ifdef __OPTIMIZE__
> +void unknown_lock_type(void *);
> +#else
> +static inline void unknown_lock_type(void *unused)
> +{
> +abort();
> +}

Since you're using __builtin_choose_expr, I'm surprised you would need to test
__OPTIMZE__.  The nature of the builtin is such that it *must* eliminate the
other branch; otherwise the types don't even match up.

It might be worth using __attribute__((error("message"))) on the function too.

Otherwise,

Reviewed-by: Richard Henderson 


r~

[Qemu-devel] [PATCH 2/5] lockable: add QemuLockable

[Paolo Bonzini]

QemuLockable is a polymorphic lock type that takes an object and
knows which function to use for locking and unlocking.  The
implementation could use C11 _Generic, but since the support is
not very widespread I am instead using __builtin_choose_expr and
__builtin_types_compatible_p, which are already used by
include/qemu/atomic.h.

QemuLockable can be used to implement lock guards, or to pass around
a lock in such a way that a function can release it and re-acquire it.
The next patch will do this for CoQueue.

Signed-off-by: Paolo Bonzini 
---
 include/qemu/compiler.h  | 40 
 include/qemu/coroutine.h |  4 +-
 include/qemu/lockable.h  | 96 
 include/qemu/thread.h|  5 +--
 include/qemu/typedefs.h  |  4 ++
 tests/test-coroutine.c   | 25 +
 6 files changed, 169 insertions(+), 5 deletions(-)
 create mode 100644 include/qemu/lockable.h

diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
index 340e5fdc09..5179bedb1e 100644
--- a/include/qemu/compiler.h
+++ b/include/qemu/compiler.h
@@ -111,4 +111,44 @@
 #define GCC_FMT_ATTR(n, m)
 #endif
 
+/* Implement C11 _Generic via GCC builtins.  Example:
+ *
+ *QEMU_GENERIC(x, (float, sinf), (long double, sinl), sin) (x)
+ *
+ * The first argument is the discriminator.  The last is the default value.
+ * The middle ones are tuples in "(type, expansion)" format.
+ */
+
+/* First, find out the number of generic cases.  */
+#define QEMU_GENERIC(x, ...) \
+QEMU_GENERIC_(typeof(x), __VA_ARGS__, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 0)
+
+/* There will be extra arguments, but they are not used.  */
+#define QEMU_GENERIC_(x, a0, a1, a2, a3, a4, a5, a6, a7, a8, a9, count, ...) \
+QEMU_GENERIC##count(x, a0, a1, a2, a3, a4, a5, a6, a7, a8, a9)
+
+/* Two more helper macros, this time to extract items from a parenthesized
+ * list.
+ */
+#define QEMU_FIRST_(a, b) a
+#define QEMU_SECOND_(a, b) b
+
+/* ... and a final one for the common part of the "recursion".  */
+#define QEMU_GENERIC_IF(x, type_then, else_)   
\
+__builtin_choose_expr(__builtin_types_compatible_p(x,  
\
+   QEMU_FIRST_ type_then), 
\
+  QEMU_SECOND_ type_then, else_)
+
+/* CPP poor man's "recursion".  */
+#define QEMU_GENERIC1(x, a0, ...) (a0)
+#define QEMU_GENERIC2(x, a0, ...) QEMU_GENERIC_IF(x, a0, QEMU_GENERIC1(x, 
__VA_ARGS__))
+#define QEMU_GENERIC3(x, a0, ...) QEMU_GENERIC_IF(x, a0, QEMU_GENERIC2(x, 
__VA_ARGS__))
+#define QEMU_GENERIC4(x, a0, ...) QEMU_GENERIC_IF(x, a0, QEMU_GENERIC3(x, 
__VA_ARGS__))
+#define QEMU_GENERIC5(x, a0, ...) QEMU_GENERIC_IF(x, a0, QEMU_GENERIC4(x, 
__VA_ARGS__))
+#define QEMU_GENERIC6(x, a0, ...) QEMU_GENERIC_IF(x, a0, QEMU_GENERIC5(x, 
__VA_ARGS__))
+#define QEMU_GENERIC7(x, a0, ...) QEMU_GENERIC_IF(x, a0, QEMU_GENERIC6(x, 
__VA_ARGS__))
+#define QEMU_GENERIC8(x, a0, ...) QEMU_GENERIC_IF(x, a0, QEMU_GENERIC7(x, 
__VA_ARGS__))
+#define QEMU_GENERIC9(x, a0, ...) QEMU_GENERIC_IF(x, a0, QEMU_GENERIC8(x, 
__VA_ARGS__))
+#define QEMU_GENERIC10(x, a0, ...) QEMU_GENERIC_IF(x, a0, QEMU_GENERIC9(x, 
__VA_ARGS__))
+
 #endif /* COMPILER_H */
diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h
index ce2eb73670..8a5129741c 100644
--- a/include/qemu/coroutine.h
+++ b/include/qemu/coroutine.h
@@ -121,7 +121,7 @@ bool qemu_coroutine_entered(Coroutine *co);
  * Provides a mutex that can be used to synchronise coroutines
  */
 struct CoWaitRecord;
-typedef struct CoMutex {
+struct CoMutex {
 /* Count of pending lockers; 0 for a free mutex, 1 for an
  * uncontended mutex.
  */
@@ -142,7 +142,7 @@ typedef struct CoMutex {
 unsigned handoff, sequence;
 
 Coroutine *holder;
-} CoMutex;
+};
 
 /**
  * Initialises a CoMutex. This must be called before any other operation is 
used
diff --git a/include/qemu/lockable.h b/include/qemu/lockable.h
new file mode 100644
index 00..826ac3f675
--- /dev/null
+++ b/include/qemu/lockable.h
@@ -0,0 +1,96 @@
+/*
+ * Polymorphic locking functions (aka poor man templates)
+ *
+ * Copyright Red Hat, Inc. 2017, 2018
+ *
+ * Author: Paolo Bonzini 
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+
+#ifndef QEMU_LOCKABLE_H
+#define QEMU_LOCKABLE_H
+
+#include "qemu/coroutine.h"
+#include "qemu/thread.h"
+
+typedef void QemuLockUnlockFunc(void *);
+
+struct QemuLockable {
+void *object;
+QemuLockUnlockFunc *lock;
+QemuLockUnlockFunc *unlock;
+};
+
+/* This function gives an error if an invalid, non-NULL pointer type is passed
+ * to QEMU_MAKE_LOCKABLE.  For optimized builds, we can rely on dead-code 
elimination
+ * from the compiler, and give the errors already at link time.
+ */
+#ifdef __OPTIMIZE__
+void unknown_lock_type(void *);
+#else
+static inline 

[Qemu-devel] [PATCH 2/5] lockable: add QemuLockable

[Paolo Bonzini]

QemuLockable is a polymorphic lock type that takes an object and
knows which function to use for locking and unlocking.  The
implementation could use C11 _Generic, but since the support is
not very widespread I am instead using __builtin_choose_expr and
__builtin_types_compatible_p, which are already used by
include/qemu/atomic.h.

QemuLockable can be used to implement lock guards, or to pass around
a lock in such a way that a function can release it and re-acquire it.
The next patch will do this for CoQueue.

Signed-off-by: Paolo Bonzini 
---
 include/qemu/compiler.h  | 40 
 include/qemu/coroutine.h |  4 +-
 include/qemu/lockable.h  | 96 
 include/qemu/thread.h|  5 +--
 include/qemu/typedefs.h  |  4 ++
 tests/test-coroutine.c   | 25 +
 6 files changed, 169 insertions(+), 5 deletions(-)
 create mode 100644 include/qemu/lockable.h

diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
index 340e5fdc09..5179bedb1e 100644
--- a/include/qemu/compiler.h
+++ b/include/qemu/compiler.h
@@ -111,4 +111,44 @@
 #define GCC_FMT_ATTR(n, m)
 #endif
 
+/* Implement C11 _Generic via GCC builtins.  Example:
+ *
+ *QEMU_GENERIC(x, (float, sinf), (long double, sinl), sin) (x)
+ *
+ * The first argument is the discriminator.  The last is the default value.
+ * The middle ones are tuples in "(type, expansion)" format.
+ */
+
+/* First, find out the number of generic cases.  */
+#define QEMU_GENERIC(x, ...) \
+QEMU_GENERIC_(typeof(x), __VA_ARGS__, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 0)
+
+/* There will be extra arguments, but they are not used.  */
+#define QEMU_GENERIC_(x, a0, a1, a2, a3, a4, a5, a6, a7, a8, a9, count, ...) \
+QEMU_GENERIC##count(x, a0, a1, a2, a3, a4, a5, a6, a7, a8, a9)
+
+/* Two more helper macros, this time to extract items from a parenthesized
+ * list.
+ */
+#define QEMU_FIRST_(a, b) a
+#define QEMU_SECOND_(a, b) b
+
+/* ... and a final one for the common part of the "recursion".  */
+#define QEMU_GENERIC_IF(x, type_then, else_)   
\
+__builtin_choose_expr(__builtin_types_compatible_p(x,  
\
+   QEMU_FIRST_ type_then), 
\
+  QEMU_SECOND_ type_then, else_)
+
+/* CPP poor man's "recursion".  */
+#define QEMU_GENERIC1(x, a0, ...) (a0)
+#define QEMU_GENERIC2(x, a0, ...) QEMU_GENERIC_IF(x, a0, QEMU_GENERIC1(x, 
__VA_ARGS__))
+#define QEMU_GENERIC3(x, a0, ...) QEMU_GENERIC_IF(x, a0, QEMU_GENERIC2(x, 
__VA_ARGS__))
+#define QEMU_GENERIC4(x, a0, ...) QEMU_GENERIC_IF(x, a0, QEMU_GENERIC3(x, 
__VA_ARGS__))
+#define QEMU_GENERIC5(x, a0, ...) QEMU_GENERIC_IF(x, a0, QEMU_GENERIC4(x, 
__VA_ARGS__))
+#define QEMU_GENERIC6(x, a0, ...) QEMU_GENERIC_IF(x, a0, QEMU_GENERIC5(x, 
__VA_ARGS__))
+#define QEMU_GENERIC7(x, a0, ...) QEMU_GENERIC_IF(x, a0, QEMU_GENERIC6(x, 
__VA_ARGS__))
+#define QEMU_GENERIC8(x, a0, ...) QEMU_GENERIC_IF(x, a0, QEMU_GENERIC7(x, 
__VA_ARGS__))
+#define QEMU_GENERIC9(x, a0, ...) QEMU_GENERIC_IF(x, a0, QEMU_GENERIC8(x, 
__VA_ARGS__))
+#define QEMU_GENERIC10(x, a0, ...) QEMU_GENERIC_IF(x, a0, QEMU_GENERIC9(x, 
__VA_ARGS__))
+
 #endif /* COMPILER_H */
diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h
index ce2eb73670..8a5129741c 100644
--- a/include/qemu/coroutine.h
+++ b/include/qemu/coroutine.h
@@ -121,7 +121,7 @@ bool qemu_coroutine_entered(Coroutine *co);
  * Provides a mutex that can be used to synchronise coroutines
  */
 struct CoWaitRecord;
-typedef struct CoMutex {
+struct CoMutex {
 /* Count of pending lockers; 0 for a free mutex, 1 for an
  * uncontended mutex.
  */
@@ -142,7 +142,7 @@ typedef struct CoMutex {
 unsigned handoff, sequence;
 
 Coroutine *holder;
-} CoMutex;
+};
 
 /**
  * Initialises a CoMutex. This must be called before any other operation is 
used
diff --git a/include/qemu/lockable.h b/include/qemu/lockable.h
new file mode 100644
index 00..826ac3f675
--- /dev/null
+++ b/include/qemu/lockable.h
@@ -0,0 +1,96 @@
+/*
+ * Polymorphic locking functions (aka poor man templates)
+ *
+ * Copyright Red Hat, Inc. 2017, 2018
+ *
+ * Author: Paolo Bonzini 
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+
+#ifndef QEMU_LOCKABLE_H
+#define QEMU_LOCKABLE_H
+
+#include "qemu/coroutine.h"
+#include "qemu/thread.h"
+
+typedef void QemuLockUnlockFunc(void *);
+
+struct QemuLockable {
+void *object;
+QemuLockUnlockFunc *lock;
+QemuLockUnlockFunc *unlock;
+};
+
+/* This function gives an error if an invalid, non-NULL pointer type is passed
+ * to QEMU_MAKE_LOCKABLE.  For optimized builds, we can rely on dead-code 
elimination
+ * from the compiler, and give the errors already at link time.
+ */
+#ifdef __OPTIMIZE__
+void unknown_lock_type(void *);
+#else
+static inline 

Re: [Qemu-devel] [PATCH 2/5] lockable: add QemuLockable

[Paolo Bonzini]

On 29/01/2018 12:42, Stefan Hajnoczi wrote:
> On Thu, Jan 25, 2018 at 06:59:46PM +0100, Paolo Bonzini wrote:
>> +struct QemuLockable {
>> +void *object;
>> +QemuLockUnlockFunc *lock;
>> +QemuLockUnlockFunc *unlock;
>> +};
> ...
>> +/* In C, compound literals have the lifetime of an automatic variable.
>> + * In C++ it would be different, but then C++ wouldn't need QemuLockable
>> + * either...
>> + */
>> +#define QEMU_MAKE_LOCKABLE_(x) qemu_make_lockable((x), &(QemuLockable) {
>> \
>> +.object = (x),   \
>> +.lock = QEMU_LOCK_FUNC(x),   \
>> +.unlock = QEMU_UNLOCK_FUNC(x),   \
>> +})
> 
> After all these tricks we still end up with a struct containing function
> pointers.  That's a little sad because the power of generics is
> specializing code at compile time.
> 
> IMO the generics usage here doesn't have a big pay-off.  The generated
> code is more or less the same as without generics!
> 
> It makes me wonder if the API would be more maintainable with:
> 
>   typedef struct {
>   QemuLockUnlockFunc *lock;
>   QemuLockUnlockFunc *unlock;
>   } LockableOps;
> 
>   extern const LockableOps co_mutex_lockable_ops;
>   extern const LockableOps qemu_spin_lockable_ops;
>   ...
> 
> The user passes in the appropriate LockableOps instance for their type
> and generics are not needed.
> 
> This approach means future changes do not require digging through the
> macros to understand how this stuff works.

The difference between the two is that, once we use it for lock guards,
the compiler is able to see constant function pointers and inline
everything.  If you use LockableOps, it doesn't.

Paolo

> Maybe I've missed something?
> 
> Stefan
> 

Re: [Qemu-devel] [PATCH 2/5] lockable: add QemuLockable

[Stefan Hajnoczi]

On Thu, Jan 25, 2018 at 06:59:46PM +0100, Paolo Bonzini wrote:
> +struct QemuLockable {
> +void *object;
> +QemuLockUnlockFunc *lock;
> +QemuLockUnlockFunc *unlock;
> +};
...
> +/* In C, compound literals have the lifetime of an automatic variable.
> + * In C++ it would be different, but then C++ wouldn't need QemuLockable
> + * either...
> + */
> +#define QEMU_MAKE_LOCKABLE_(x) qemu_make_lockable((x), &(QemuLockable) {\
> +.object = (x),   \
> +.lock = QEMU_LOCK_FUNC(x),   \
> +.unlock = QEMU_UNLOCK_FUNC(x),   \
> +})

After all these tricks we still end up with a struct containing function
pointers.  That's a little sad because the power of generics is
specializing code at compile time.

IMO the generics usage here doesn't have a big pay-off.  The generated
code is more or less the same as without generics!

It makes me wonder if the API would be more maintainable with:

  typedef struct {
  QemuLockUnlockFunc *lock;
  QemuLockUnlockFunc *unlock;
  } LockableOps;

  extern const LockableOps co_mutex_lockable_ops;
  extern const LockableOps qemu_spin_lockable_ops;
  ...

The user passes in the appropriate LockableOps instance for their type
and generics are not needed.

This approach means future changes do not require digging through the
macros to understand how this stuff works.

Maybe I've missed something?

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH 2/5] lockable: add QemuLockable

[Fam Zheng]

On Fri, Jan 26, 2018 at 1:59 AM, Paolo Bonzini  wrote:
> QemuLockable is a polymorphic lock type that takes an object and
> knows which function to use for locking and unlocking.  The
> implementation could use C11 _Generic, but since the support is
> not very widespread I am instead using __builtin_choose_expr and
> __builtin_types_compatible_p, which are already used by
> include/qemu/atomic.h.
>
> QemuLockable can be used to implement lock guards, or to pass around
> a lock in such a way that a function can release it and re-acquire it.
> The next patch will do this for CoQueue.
>
> Signed-off-by: Paolo Bonzini 
> ---
> v2->v3: now it works :(  Also, QEMU_MAKE_LOCKABLE(NULL) returns NULL.
> The argument of QEMU_MAKE_LOCKABLE is expected to be a 
> constant,
> so the test is optimized away.
>
>  include/qemu/compiler.h  | 40 ++
>  include/qemu/coroutine.h |  4 +--
>  include/qemu/lockable.h  | 88 
> 
>  include/qemu/thread.h|  5 ++-
>  include/qemu/typedefs.h  |  4 +++
>  tests/test-coroutine.c   | 25 ++
>  6 files changed, 161 insertions(+), 5 deletions(-)
>  create mode 100644 include/qemu/lockable.h
>
> diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
> index 340e5fdc09..5179bedb1e 100644
> --- a/include/qemu/compiler.h
> +++ b/include/qemu/compiler.h
> @@ -111,4 +111,44 @@
>  #define GCC_FMT_ATTR(n, m)
>  #endif
>
> +/* Implement C11 _Generic via GCC builtins.  Example:
> + *
> + *QEMU_GENERIC(x, (float, sinf), (long double, sinl), sin) (x)
> + *
> + * The first argument is the discriminator.  The last is the default value.
> + * The middle ones are tuples in "(type, expansion)" format.
> + */
> +
> +/* First, find out the number of generic cases.  */
> +#define QEMU_GENERIC(x, ...) \
> +QEMU_GENERIC_(typeof(x), __VA_ARGS__, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 0)
> +
> +/* There will be extra arguments, but they are not used.  */
> +#define QEMU_GENERIC_(x, a0, a1, a2, a3, a4, a5, a6, a7, a8, a9, count, ...) 
> \
> +QEMU_GENERIC##count(x, a0, a1, a2, a3, a4, a5, a6, a7, a8, a9)
> +
> +/* Two more helper macros, this time to extract items from a parenthesized
> + * list.
> + */
> +#define QEMU_FIRST_(a, b) a
> +#define QEMU_SECOND_(a, b) b
> +
> +/* ... and a final one for the common part of the "recursion".  */
> +#define QEMU_GENERIC_IF(x, type_then, else_) 
>   \
> +__builtin_choose_expr(__builtin_types_compatible_p(x,
>   \
> +   QEMU_FIRST_ 
> type_then), \
> +  QEMU_SECOND_ type_then, else_)
> +
> +/* CPP poor man's "recursion".  */
> +#define QEMU_GENERIC1(x, a0, ...) (a0)
> +#define QEMU_GENERIC2(x, a0, ...) QEMU_GENERIC_IF(x, a0, QEMU_GENERIC1(x, 
> __VA_ARGS__))
> +#define QEMU_GENERIC3(x, a0, ...) QEMU_GENERIC_IF(x, a0, QEMU_GENERIC2(x, 
> __VA_ARGS__))
> +#define QEMU_GENERIC4(x, a0, ...) QEMU_GENERIC_IF(x, a0, QEMU_GENERIC3(x, 
> __VA_ARGS__))
> +#define QEMU_GENERIC5(x, a0, ...) QEMU_GENERIC_IF(x, a0, QEMU_GENERIC4(x, 
> __VA_ARGS__))
> +#define QEMU_GENERIC6(x, a0, ...) QEMU_GENERIC_IF(x, a0, QEMU_GENERIC5(x, 
> __VA_ARGS__))
> +#define QEMU_GENERIC7(x, a0, ...) QEMU_GENERIC_IF(x, a0, QEMU_GENERIC6(x, 
> __VA_ARGS__))
> +#define QEMU_GENERIC8(x, a0, ...) QEMU_GENERIC_IF(x, a0, QEMU_GENERIC7(x, 
> __VA_ARGS__))
> +#define QEMU_GENERIC9(x, a0, ...) QEMU_GENERIC_IF(x, a0, QEMU_GENERIC8(x, 
> __VA_ARGS__))
> +#define QEMU_GENERIC10(x, a0, ...) QEMU_GENERIC_IF(x, a0, QEMU_GENERIC9(x, 
> __VA_ARGS__))
> +
>  #endif /* COMPILER_H */
> diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h
> index ce2eb73670..8a5129741c 100644
> --- a/include/qemu/coroutine.h
> +++ b/include/qemu/coroutine.h
> @@ -121,7 +121,7 @@ bool qemu_coroutine_entered(Coroutine *co);
>   * Provides a mutex that can be used to synchronise coroutines
>   */
>  struct CoWaitRecord;
> -typedef struct CoMutex {
> +struct CoMutex {
>  /* Count of pending lockers; 0 for a free mutex, 1 for an
>   * uncontended mutex.
>   */
> @@ -142,7 +142,7 @@ typedef struct CoMutex {
>  unsigned handoff, sequence;
>
>  Coroutine *holder;
> -} CoMutex;
> +};
>
>  /**
>   * Initialises a CoMutex. This must be called before any other operation is 
> used
> diff --git a/include/qemu/lockable.h b/include/qemu/lockable.h
> new file mode 100644
> index 00..f527d0ddb2
> --- /dev/null
> +++ b/include/qemu/lockable.h
> @@ -0,0 +1,88 @@
> +/*
> + * Polymorphic locking functions (aka poor man templates)
> + *
> + * Copyright Red Hat, Inc. 2017
> + *
> + * Author: Paolo Bonzini 
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2 or later.
> + * See the COPYING.LIB file in the top-level directory.
> + *
> + */
> +
> +#ifndef QEMU_LOCKABLE_H
> +#define 

Re: [Qemu-devel] [PATCH 2/5] lockable: add QemuLockable

[Fam Zheng]

On Fri, Jan 26, 2018 at 1:59 AM, Paolo Bonzini  wrote:
> diff --git a/include/qemu/lockable.h b/include/qemu/lockable.h
> new file mode 100644
> index 00..f527d0ddb2
> --- /dev/null
> +++ b/include/qemu/lockable.h
> @@ -0,0 +1,88 @@
> +/*
> + * Polymorphic locking functions (aka poor man templates)
> + *
> + * Copyright Red Hat, Inc. 2017

It's curious why then change you made in v2 (2017 - 2018) is reverted
in v3 (similar to the commit message). I can fix this and other typos
when applying.

Fam

Re: [Qemu-devel] [PATCH 2/5] lockable: add QemuLockable

[Eric Blake]

On 01/25/2018 11:59 AM, Paolo Bonzini wrote:
> QemuLockable is a polymorphic lock type that takes an object and
> knows which function to use for locking and unlocking.  The
> implementation could use C11 _Generic, but since the support is
> not very widespread I am instead using __builtin_choose_expr and
> __builtin_types_compatible_p, which are already used by
> include/qemu/atomic.h.
> 
> QemuLockable can be used to implement lock guards, or to pass around
> a lock in such a way that a function can release it and re-acquire it.
> The next patch will do this for CoQueue.
> 
> Signed-off-by: Paolo Bonzini 
> ---

> +
> +static inline __attribute__((__always_inline__)) QemuLockable *
> +qemu_make_lockable(void *x, QemuLockable *lockable)
> +{
> +/* We cannot test this in a macro, otherwise we get * compiler

Spurious '*' ?

> + * warnings like "the address of 'm' will always evaluate as 'true'".
> + */
> +return x ? lockable : NULL;
> +}
> +

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [PATCH 2/5] lockable: add QemuLockable

[Paolo Bonzini]

QemuLockable is a polymorphic lock type that takes an object and
knows which function to use for locking and unlocking.  The
implementation could use C11 _Generic, but since the support is
not very widespread I am instead using __builtin_choose_expr and
__builtin_types_compatible_p, which are already used by
include/qemu/atomic.h.

QemuLockable can be used to implement lock guards, or to pass around
a lock in such a way that a function can release it and re-acquire it.
The next patch will do this for CoQueue.

Signed-off-by: Paolo Bonzini 
---
v2->v3: now it works :(  Also, QEMU_MAKE_LOCKABLE(NULL) returns NULL.
The argument of QEMU_MAKE_LOCKABLE is expected to be a constant,
so the test is optimized away.

 include/qemu/compiler.h  | 40 ++
 include/qemu/coroutine.h |  4 +--
 include/qemu/lockable.h  | 88 
 include/qemu/thread.h|  5 ++-
 include/qemu/typedefs.h  |  4 +++
 tests/test-coroutine.c   | 25 ++
 6 files changed, 161 insertions(+), 5 deletions(-)
 create mode 100644 include/qemu/lockable.h

diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
index 340e5fdc09..5179bedb1e 100644
--- a/include/qemu/compiler.h
+++ b/include/qemu/compiler.h
@@ -111,4 +111,44 @@
 #define GCC_FMT_ATTR(n, m)
 #endif
 
+/* Implement C11 _Generic via GCC builtins.  Example:
+ *
+ *QEMU_GENERIC(x, (float, sinf), (long double, sinl), sin) (x)
+ *
+ * The first argument is the discriminator.  The last is the default value.
+ * The middle ones are tuples in "(type, expansion)" format.
+ */
+
+/* First, find out the number of generic cases.  */
+#define QEMU_GENERIC(x, ...) \
+QEMU_GENERIC_(typeof(x), __VA_ARGS__, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 0)
+
+/* There will be extra arguments, but they are not used.  */
+#define QEMU_GENERIC_(x, a0, a1, a2, a3, a4, a5, a6, a7, a8, a9, count, ...) \
+QEMU_GENERIC##count(x, a0, a1, a2, a3, a4, a5, a6, a7, a8, a9)
+
+/* Two more helper macros, this time to extract items from a parenthesized
+ * list.
+ */
+#define QEMU_FIRST_(a, b) a
+#define QEMU_SECOND_(a, b) b
+
+/* ... and a final one for the common part of the "recursion".  */
+#define QEMU_GENERIC_IF(x, type_then, else_)   
\
+__builtin_choose_expr(__builtin_types_compatible_p(x,  
\
+   QEMU_FIRST_ type_then), 
\
+  QEMU_SECOND_ type_then, else_)
+
+/* CPP poor man's "recursion".  */
+#define QEMU_GENERIC1(x, a0, ...) (a0)
+#define QEMU_GENERIC2(x, a0, ...) QEMU_GENERIC_IF(x, a0, QEMU_GENERIC1(x, 
__VA_ARGS__))
+#define QEMU_GENERIC3(x, a0, ...) QEMU_GENERIC_IF(x, a0, QEMU_GENERIC2(x, 
__VA_ARGS__))
+#define QEMU_GENERIC4(x, a0, ...) QEMU_GENERIC_IF(x, a0, QEMU_GENERIC3(x, 
__VA_ARGS__))
+#define QEMU_GENERIC5(x, a0, ...) QEMU_GENERIC_IF(x, a0, QEMU_GENERIC4(x, 
__VA_ARGS__))
+#define QEMU_GENERIC6(x, a0, ...) QEMU_GENERIC_IF(x, a0, QEMU_GENERIC5(x, 
__VA_ARGS__))
+#define QEMU_GENERIC7(x, a0, ...) QEMU_GENERIC_IF(x, a0, QEMU_GENERIC6(x, 
__VA_ARGS__))
+#define QEMU_GENERIC8(x, a0, ...) QEMU_GENERIC_IF(x, a0, QEMU_GENERIC7(x, 
__VA_ARGS__))
+#define QEMU_GENERIC9(x, a0, ...) QEMU_GENERIC_IF(x, a0, QEMU_GENERIC8(x, 
__VA_ARGS__))
+#define QEMU_GENERIC10(x, a0, ...) QEMU_GENERIC_IF(x, a0, QEMU_GENERIC9(x, 
__VA_ARGS__))
+
 #endif /* COMPILER_H */
diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h
index ce2eb73670..8a5129741c 100644
--- a/include/qemu/coroutine.h
+++ b/include/qemu/coroutine.h
@@ -121,7 +121,7 @@ bool qemu_coroutine_entered(Coroutine *co);
  * Provides a mutex that can be used to synchronise coroutines
  */
 struct CoWaitRecord;
-typedef struct CoMutex {
+struct CoMutex {
 /* Count of pending lockers; 0 for a free mutex, 1 for an
  * uncontended mutex.
  */
@@ -142,7 +142,7 @@ typedef struct CoMutex {
 unsigned handoff, sequence;
 
 Coroutine *holder;
-} CoMutex;
+};
 
 /**
  * Initialises a CoMutex. This must be called before any other operation is 
used
diff --git a/include/qemu/lockable.h b/include/qemu/lockable.h
new file mode 100644
index 00..f527d0ddb2
--- /dev/null
+++ b/include/qemu/lockable.h
@@ -0,0 +1,88 @@
+/*
+ * Polymorphic locking functions (aka poor man templates)
+ *
+ * Copyright Red Hat, Inc. 2017
+ *
+ * Author: Paolo Bonzini 
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+
+#ifndef QEMU_LOCKABLE_H
+#define QEMU_LOCKABLE_H
+
+#include "qemu/coroutine.h"
+#include "qemu/thread.h"
+
+typedef void QemuLockUnlockFunc(void *);
+
+struct QemuLockable {
+void *object;
+QemuLockUnlockFunc *lock;
+QemuLockUnlockFunc *unlock;
+};
+
+/* This function gives link-time errors if an invalid, non-NULL
+ * pointer type is passed to