From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
RCU_READ_LOCK_AUTO takes the rcu_read_lock and then uses glib's
g_auto infrastructure (and thus whatever the compiler's hooks are) to
release it on all exits of the block.
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
include/qemu/rcu.h | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/include/qemu/rcu.h b/include/qemu/rcu.h
index 22876d1428..8768a7b60a 100644
--- a/include/qemu/rcu.h
+++ b/include/qemu/rcu.h
@@ -154,6 +154,24 @@ extern void call_rcu1(struct rcu_head *head, RCUCBFunc *func);
}), \
(RCUCBFunc *)g_free);
+typedef void RCUReadAuto;
+static inline RCUReadAuto *rcu_read_auto_lock(void)
+{
+ rcu_read_lock();
+ /* Anything non-NULL causes the cleanup function to be called */
+ return (void *)0x1;
+}
+
+static inline void rcu_read_auto_unlock(RCUReadAuto *r)
+{
+ rcu_read_unlock();
+}
+
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(RCUReadAuto, rcu_read_auto_unlock)
+
+#define RCU_READ_LOCK_AUTO \
+ g_autoptr(RCUReadAuto) _rcu_read_auto = rcu_read_auto_lock()
+
#ifdef __cplusplus
}
#endif
--
2.21.0
On Wed, Sep 11, 2019 at 08:06:18PM +0100, Dr. David Alan Gilbert (git) wrote: > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > RCU_READ_LOCK_AUTO takes the rcu_read_lock and then uses glib's > g_auto infrastructure (and thus whatever the compiler's hooks are) to > release it on all exits of the block. > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > --- > include/qemu/rcu.h | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 11/09/19 21:06, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> RCU_READ_LOCK_AUTO takes the rcu_read_lock and then uses glib's
> g_auto infrastructure (and thus whatever the compiler's hooks are) to
> release it on all exits of the block.
>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
> include/qemu/rcu.h | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
> diff --git a/include/qemu/rcu.h b/include/qemu/rcu.h
> index 22876d1428..8768a7b60a 100644
> --- a/include/qemu/rcu.h
> +++ b/include/qemu/rcu.h
> @@ -154,6 +154,24 @@ extern void call_rcu1(struct rcu_head *head, RCUCBFunc *func);
> }), \
> (RCUCBFunc *)g_free);
>
> +typedef void RCUReadAuto;
> +static inline RCUReadAuto *rcu_read_auto_lock(void)
> +{
> + rcu_read_lock();
> + /* Anything non-NULL causes the cleanup function to be called */
> + return (void *)0x1;
Doesn't this cause a warning (should be "(void *)(uintptr_t)1" instead)?
> +}
> +
> +static inline void rcu_read_auto_unlock(RCUReadAuto *r)
> +{
> + rcu_read_unlock();
> +}
> +
> +G_DEFINE_AUTOPTR_CLEANUP_FUNC(RCUReadAuto, rcu_read_auto_unlock)
> +
> +#define RCU_READ_LOCK_AUTO \
> + g_autoptr(RCUReadAuto) _rcu_read_auto = rcu_read_auto_lock()
> +
> #ifdef __cplusplus
> }
> #endif
>
Is it possible to make this a scope, like
WITH_RCU_READ_LOCK() {
}
? Perhaps something like
#define WITH_RCU_READ_LOCK() \
WITH_RCU_READ_LOCK_(_rcu_read_auto##__COUNTER__)
#define WITH_RCU_READ_LOCK_(var) \
for (g_autoptr(RCUReadAuto) var = rcu_read_auto_lock();
(var); rcu_read_auto_unlock(), (var) = NULL)
where the dummy variable doubles as an execute-once guard and the gauto
cleanup is still used in case of a "break". I'm not sure if the above
raises a warning because of the variable declaration inside the for
loop, though.
Thanks,
Paolo
* Paolo Bonzini (pbonzini@redhat.com) wrote:
> On 11/09/19 21:06, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >
> > RCU_READ_LOCK_AUTO takes the rcu_read_lock and then uses glib's
> > g_auto infrastructure (and thus whatever the compiler's hooks are) to
> > release it on all exits of the block.
> >
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > ---
> > include/qemu/rcu.h | 18 ++++++++++++++++++
> > 1 file changed, 18 insertions(+)
> >
> > diff --git a/include/qemu/rcu.h b/include/qemu/rcu.h
> > index 22876d1428..8768a7b60a 100644
> > --- a/include/qemu/rcu.h
> > +++ b/include/qemu/rcu.h
> > @@ -154,6 +154,24 @@ extern void call_rcu1(struct rcu_head *head, RCUCBFunc *func);
> > }), \
> > (RCUCBFunc *)g_free);
> >
> > +typedef void RCUReadAuto;
> > +static inline RCUReadAuto *rcu_read_auto_lock(void)
> > +{
> > + rcu_read_lock();
> > + /* Anything non-NULL causes the cleanup function to be called */
> > + return (void *)0x1;
>
> Doesn't this cause a warning (should be "(void *)(uintptr_t)1" instead)?
Apparently not, but I'll change it anyway.
> > +}
> > +
> > +static inline void rcu_read_auto_unlock(RCUReadAuto *r)
> > +{
> > + rcu_read_unlock();
> > +}
> > +
> > +G_DEFINE_AUTOPTR_CLEANUP_FUNC(RCUReadAuto, rcu_read_auto_unlock)
> > +
> > +#define RCU_READ_LOCK_AUTO \
> > + g_autoptr(RCUReadAuto) _rcu_read_auto = rcu_read_auto_lock()
> > +
> > #ifdef __cplusplus
> > }
> > #endif
> >
>
> Is it possible to make this a scope, like
>
> WITH_RCU_READ_LOCK() {
> }
>
> ? Perhaps something like
>
> #define WITH_RCU_READ_LOCK() \
> WITH_RCU_READ_LOCK_(_rcu_read_auto##__COUNTER__)
>
> #define WITH_RCU_READ_LOCK_(var) \
> for (g_autoptr(RCUReadAuto) var = rcu_read_auto_lock();
> (var); rcu_read_auto_unlock(), (var) = NULL)
>
> where the dummy variable doubles as an execute-once guard and the gauto
> cleanup is still used in case of a "break". I'm not sure if the above
> raises a warning because of the variable declaration inside the for
> loop, though.
Yes, that works - I'm not seeing any warnings.
Do you think it's best to use the block version for all cases
or to use the non-block version by taste?
The block version is quite nice, but that turns most of the patches
into 'indent everything'.
Dave
> Thanks,
>
> Paolo
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 12/09/19 19:45, Dr. David Alan Gilbert wrote: > Do you think it's best to use the block version for all cases > or to use the non-block version by taste? > The block version is quite nice, but that turns most of the patches > into 'indent everything'. I don't really know myself. On first glance I didn't like too much the non-block version and thought it was because our coding standards does not include variables declared in the middle of a block. However, I think what really bothering me is "AUTO" in the name. What do you think about "RCU_READ_LOCK_GUARD()"? The block version would have the additional prefix "WITH_". We could also add LOCK_GUARD(lock) and WITH_LOCK_GUARD(lock), using QemuLockable for polymorphism. I even had patches a while ago (though they used something like LOCK_GUARD(guard_var, lock). I think we dropped them because of fear that the API was a bit over-engineered. Paolo
* Paolo Bonzini (pbonzini@redhat.com) wrote: > On 12/09/19 19:45, Dr. David Alan Gilbert wrote: > > Do you think it's best to use the block version for all cases > > or to use the non-block version by taste? > > The block version is quite nice, but that turns most of the patches > > into 'indent everything'. > > I don't really know myself. OK, new version coming up with a mix - the diffs do look a lot more hectic with the block version. > On first glance I didn't like too much the non-block version and thought > it was because our coding standards does not include variables declared > in the middle of a block. I took that as being a coding standard to avoid confusing humans and since it wasn't visible it didn't matter too much. > However, I think what really bothering me is > "AUTO" in the name. What do you think about "RCU_READ_LOCK_GUARD()"? > The block version would have the additional prefix "WITH_". Oh well, if it's just the name we can fix that. > We could also add LOCK_GUARD(lock) and WITH_LOCK_GUARD(lock), using > QemuLockable for polymorphism. I even had patches a while ago (though > they used something like LOCK_GUARD(guard_var, lock). I think we > dropped them because of fear that the API was a bit over-engineered. Probably a separate set. Dave > Paolo -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 13/09/19 12:24, Dr. David Alan Gilbert wrote: >> We could also add LOCK_GUARD(lock) and WITH_LOCK_GUARD(lock), using >> QemuLockable for polymorphism. I even had patches a while ago (though >> they used something like LOCK_GUARD(guard_var, lock). I think we >> dropped them because of fear that the API was a bit over-engineered. > Probably a separate set. Definitely so. Thanks! Paolo
© 2016 - 2026 Red Hat, Inc.