Re: [Qemu-devel] [PATCH 2/5] lock-guard: add scoped lock implementation

[Stefan Hajnoczi]

On Mon, Dec 11, 2017 at 07:51:43AM -0600, Eric Blake wrote:
> On 12/11/2017 04:16 AM, Stefan Hajnoczi wrote:
> 
> >>> I don't understand the need for the qemu_lock_guard_is_taken()
> >>> condition, why not do the following?
> >>>
> >>>   for (QEMU_LOCK_GUARD(type, name, lock);
> >>>;
> >>>qemu_lock_guard_unlock())
> >>
> >> Because that would be an infinite loop. :)
> > 
> > Sorry, I mean for (...; false; ...).  Is there any reason to do
> > qemu_lock_guard_is_taken()?
> 
> You need the loop to execute at least once ;)
> 
> But I proposed an alternative that doesn't need is_taken, by:
> 
> for (bool name##done = false, QEMU_LOCK_GUARD(type, name, lock);
>  ! name##done; name##done = true)
> 
> if we still like the form that declares a for-loop scope.

Okay guys, for loops are hard.  I give up programming.

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH 2/5] lock-guard: add scoped lock implementation

[Eric Blake]

On 12/11/2017 04:16 AM, Stefan Hajnoczi wrote:

>>> I don't understand the need for the qemu_lock_guard_is_taken()
>>> condition, why not do the following?
>>>
>>>   for (QEMU_LOCK_GUARD(type, name, lock);
>>>;
>>>qemu_lock_guard_unlock())
>>
>> Because that would be an infinite loop. :)
> 
> Sorry, I mean for (...; false; ...).  Is there any reason to do
> qemu_lock_guard_is_taken()?

You need the loop to execute at least once ;)

But I proposed an alternative that doesn't need is_taken, by:

for (bool name##done = false, QEMU_LOCK_GUARD(type, name, lock);
 ! name##done; name##done = true)

if we still like the form that declares a for-loop scope.

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 2/5] lock-guard: add scoped lock implementation

[Stefan Hajnoczi]

On Fri, Dec 08, 2017 at 06:56:12PM +0100, Paolo Bonzini wrote:
> On 08/12/2017 16:30, Stefan Hajnoczi wrote:
> > On Fri, Dec 08, 2017 at 11:55:50AM +0100, Paolo Bonzini wrote:
> > 
> > The implementation is somewhat complex.  Please structure the header
> > file so the public interfaces are clear and documented.  Move the
> > implementation out of the way and mark it private.  That will make it
> > easier for someone to figure out "how do I use lock-guard.h".
> 
> Right, unfortunately you pretty much have to place it in the header to
> guarantee that everything is inlined.

Yes, that's fine.  I think the important part is to put public
interfaces next to each other and document them.

> >> +#define QEMU_WITH_LOCK(type, name, lock)  
> >>  \
> >> +for (QEMU_LOCK_GUARD(type, name, lock);   
> >>  \
> >> + qemu_lock_guard_is_taken(); 
> >>  \
> >> + qemu_lock_guard_unlock())
> > 
> > I don't understand the need for the qemu_lock_guard_is_taken()
> > condition, why not do the following?
> > 
> >   for (QEMU_LOCK_GUARD(type, name, lock);
> >;
> >qemu_lock_guard_unlock())
> 
> Because that would be an infinite loop. :)

Sorry, I mean for (...; false; ...).  Is there any reason to do
qemu_lock_guard_is_taken()?


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH 2/5] lock-guard: add scoped lock implementation

[Eric Blake]

On 12/08/2017 11:56 AM, Paolo Bonzini wrote:

>>> +typedef void QemuLockGuardFunc(void *);
>>> +typedef struct QemuLockGuard {
>>> +QemuLockGuardFunc *p_lock_fn, *p_unlock_fn;
>>> +void *lock;
>>> +int locked;
>>
>> bool?
> 
> Yes.
> 
>>> +#define QEMU_WITH_LOCK(type, name, lock)   
>>> \
>>> +for (QEMU_LOCK_GUARD(type, name, lock);
>>> \
>>> + qemu_lock_guard_is_taken();  
>>> \
>>> + qemu_lock_guard_unlock())
>>
>> I don't understand the need for the qemu_lock_guard_is_taken()
>> condition, why not do the following?
>>
>>   for (QEMU_LOCK_GUARD(type, name, lock);
>>;
>>qemu_lock_guard_unlock())
> 
> Because that would be an infinite loop. :)

Do we really need 'locked' to belong to the lockguard, just for our use
in for loops?  Or can we just declare it in the for loop proper, as in

#define QEMU_WITH_LOCK(type, name, lock) \
for (bool name##_done = false, QEMU_LOCK_GUARD(type, name, lock); \
 !name##_done; \
 name##_done = true)

and let the automatic scope exit at the conclusion of the one-shot loop
thus unlock things on our behalf, and now we don't have to futz around
with guard->locked everywhere else?

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 2/5] lock-guard: add scoped lock implementation

[Paolo Bonzini]

On 08/12/2017 16:30, Stefan Hajnoczi wrote:
> On Fri, Dec 08, 2017 at 11:55:50AM +0100, Paolo Bonzini wrote:
> 
> The implementation is somewhat complex.  Please structure the header
> file so the public interfaces are clear and documented.  Move the
> implementation out of the way and mark it private.  That will make it
> easier for someone to figure out "how do I use lock-guard.h".

Right, unfortunately you pretty much have to place it in the header to
guarantee that everything is inlined.

>> diff --git a/include/qemu/lock-guard.h b/include/qemu/lock-guard.h
>> new file mode 100644
>> index 00..e6a83bf9ee
>> --- /dev/null
>> +++ b/include/qemu/lock-guard.h
>> @@ -0,0 +1,103 @@
>> +#ifndef QEMU_LOCK_GUARD_H
>> +#define QEMU_LOCK_GUARD_H 1
>> +
>> +typedef void QemuLockGuardFunc(void *);
>> +typedef struct QemuLockGuard {
>> +QemuLockGuardFunc *p_lock_fn, *p_unlock_fn;
>> +void *lock;
>> +int locked;
> 
> bool?

Yes.

>> +#define QEMU_WITH_LOCK(type, name, lock)   \
>> +for (QEMU_LOCK_GUARD(type, name, lock);\
>> + qemu_lock_guard_is_taken();  \
>> + qemu_lock_guard_unlock())
> 
> I don't understand the need for the qemu_lock_guard_is_taken()
> condition, why not do the following?
> 
>   for (QEMU_LOCK_GUARD(type, name, lock);
>;
>qemu_lock_guard_unlock())

Because that would be an infinite loop. :)

Paolo

> Also, the for loop means that break statements do not work inside
> QEMU_WITH_LOCK() { ... }.  This needs to be documented.

Re: [Qemu-devel] [PATCH 2/5] lock-guard: add scoped lock implementation

[Stefan Hajnoczi]

On Fri, Dec 08, 2017 at 11:55:50AM +0100, Paolo Bonzini wrote:

The implementation is somewhat complex.  Please structure the header
file so the public interfaces are clear and documented.  Move the
implementation out of the way and mark it private.  That will make it
easier for someone to figure out "how do I use lock-guard.h".

> diff --git a/include/qemu/lock-guard.h b/include/qemu/lock-guard.h
> new file mode 100644
> index 00..e6a83bf9ee
> --- /dev/null
> +++ b/include/qemu/lock-guard.h
> @@ -0,0 +1,103 @@
> +#ifndef QEMU_LOCK_GUARD_H
> +#define QEMU_LOCK_GUARD_H 1
> +
> +typedef void QemuLockGuardFunc(void *);
> +typedef struct QemuLockGuard {
> +QemuLockGuardFunc *p_lock_fn, *p_unlock_fn;
> +void *lock;
> +int locked;

bool?

> +#define QEMU_WITH_LOCK(type, name, lock)   \
> +for (QEMU_LOCK_GUARD(type, name, lock);\
> + qemu_lock_guard_is_taken();  \
> + qemu_lock_guard_unlock())

I don't understand the need for the qemu_lock_guard_is_taken()
condition, why not do the following?

  for (QEMU_LOCK_GUARD(type, name, lock);
   ;
   qemu_lock_guard_unlock())

Also, the for loop means that break statements do not work inside
QEMU_WITH_LOCK() { ... }.  This needs to be documented.


signature.asc
Description: PGP signature

[Qemu-devel] [PATCH 2/5] lock-guard: add scoped lock implementation

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini 
---
 include/qemu/coroutine.h  |  4 ++
 include/qemu/lock-guard.h | 99 +++
 include/qemu/thread.h |  7 
 util/Makefile.objs|  1 +
 util/qemu-thread.c| 17 
 5 files changed, 128 insertions(+)
 create mode 100644 include/qemu/lock-guard.h
 create mode 100644 util/qemu-thread.c

diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h
index 9aff9a735e..8b48803fa8 100644
--- a/include/qemu/coroutine.h
+++ b/include/qemu/coroutine.h
@@ -17,6 +17,7 @@
 
 #include "qemu/queue.h"
 #include "qemu/timer.h"
+#include "qemu/lock-guard.h"
 
 /**
  * Coroutines are a mechanism for stack switching and can be used for
@@ -162,6 +163,9 @@ void coroutine_fn qemu_co_mutex_lock(CoMutex *mutex);
  */
 void coroutine_fn qemu_co_mutex_unlock(CoMutex *mutex);
 
+#define QEMU_LOCK_GUARD_FUNCS_CoMutex \
+QEMU_INIT_LOCK_GUARD(CoMutex, qemu_co_mutex_lock, qemu_co_mutex_unlock)
+
 
 /**
  * CoQueues are a mechanism to queue coroutines in order to continue executing
diff --git a/include/qemu/lock-guard.h b/include/qemu/lock-guard.h
new file mode 100644
index 00..e6a83bf9ee
--- /dev/null
+++ b/include/qemu/lock-guard.h
@@ -0,0 +1,103 @@
+#ifndef QEMU_LOCK_GUARD_H
+#define QEMU_LOCK_GUARD_H 1
+
+typedef void QemuLockGuardFunc(void *);
+typedef struct QemuLockGuard {
+QemuLockGuardFunc *p_lock_fn, *p_unlock_fn;
+void *lock;
+int locked;
+} QemuLockGuard;
+
+static inline void qemu_lock_guard_lock(QemuLockGuard *lock_guard)
+{
+assert(!lock_guard->locked);
+lock_guard->p_lock_fn(lock_guard->lock);
+lock_guard->locked = true;
+}
+
+static inline void qemu_lock_guard_unlock(QemuLockGuard *lock_guard)
+{
+assert(lock_guard->locked);
+lock_guard->locked = false;
+lock_guard->p_unlock_fn(lock_guard->lock);
+}
+
+static inline bool qemu_lock_guard_is_taken(QemuLockGuard *lock_guard)
+{
+return lock_guard->locked;
+}
+
+static inline void qemu_lock_guard_release(QemuLockGuard *lock_guard)
+{
+lock_guard->lock = NULL;
+lock_guard->locked = false;
+}
+
+inline void qemu_lock_guard_pass(void *ptr)
+{
+QemuLockGuard *lock_guard = ptr;
+assert(lock_guard->locked || !lock_guard->lock);
+}
+
+inline void qemu_lock_guard_cleanup(void *ptr)
+{
+QemuLockGuard *lock_guard = ptr;
+if (likely(lock_guard->locked)) {
+lock_guard->p_unlock_fn(lock_guard->lock);
+}
+}
+
+static inline QemuLockGuard qemu_lock_guard_init(QemuLockGuard lock_guard)
+{
+qemu_lock_guard_lock(_guard);
+return lock_guard;
+}
+
+#define QEMU_INIT_LOCK_GUARD(type, lock_fn, unlock_fn) \
+.p_lock_fn   = (QemuLockGuardFunc *) (void (*) (type *)) lock_fn,  \
+.p_unlock_fn = (QemuLockGuardFunc *) (void (*) (type *)) unlock_fn
+
+#define QEMU_LOCK_GUARD_(type, lock, locked)   \
+(QemuLockGuard) {  \
+QEMU_LOCK_GUARD_FUNCS_##type,  \
+lock + type_check(typeof(*lock), type),\
+locked \
+}
+
+/* Take a lock that will be unlocked on returning */
+#define QEMU_LOCK_GUARD(type, name, lock)  \
+QemuLockGuard __attribute__((cleanup(qemu_lock_guard_cleanup))) name = \
+qemu_lock_guard_init(QEMU_LOCK_GUARD_(type, lock, false))
+
+#define QEMU_WITH_LOCK(type, name, lock)   \
+for (QEMU_LOCK_GUARD(type, name, lock);\
+ qemu_lock_guard_is_taken();  \
+ qemu_lock_guard_unlock())
+
+/* Create a QemuLockGuard for a lock that is taken and will be unlocked on
+ * returning
+ */
+#define QEMU_ADOPT_LOCK(type, name, lock)  \
+QemuLockGuard __attribute__((cleanup(qemu_lock_guard_cleanup))) name = \
+QEMU_LOCK_GUARD_(type, lock, true)
+
+#define QEMU_WITH_ADOPTED_LOCK(type, name, lock)   \
+for (QEMU_ADOPT_LOCK(type, name, lock);\
+ qemu_lock_guard_is_taken();  \
+ qemu_lock_guard_unlock())
+
+/* Take a lock and create a QemuLockGuard for it, asserting that it will
+ * be locked when returning.
+ */
+#define QEMU_RETURN_LOCK(type, name, lock) \
+QemuLockGuard __attribute__((cleanup(qemu_lock_guard_pass))) name =\
+qemu_lock_guard_init(QEMU_LOCK_GUARD_(type, lock, false))
+
+/* Create a QemuLockGuard for a lock that is taken and must be locked
+ * when returning
+ */
+#define QEMU_TAKEN_LOCK(type, name, lock)  \
+QemuLockGuard __attribute__((cleanup(qemu_lock_guard_pass))) name =\
+QEMU_LOCK_GUARD_(type, lock, true)
+
+#endif