Re: [Qemu-devel] [PATCH v2 1/5] rcu: Add automatically released rcu_read_lock variant
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
Re: [Qemu-devel] [PATCH v2 1/5] rcu: Add automatically released rcu_read_lock variant
* Paolo Bonzini (pbonz...@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 / dgilb...@redhat.com / Manchester, UK
Re: [Qemu-devel] [PATCH v2 1/5] rcu: Add automatically released rcu_read_lock variant
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
Re: [Qemu-devel] [PATCH v2 1/5] rcu: Add automatically released rcu_read_lock variant
* Paolo Bonzini (pbonz...@redhat.com) wrote: > On 11/09/19 21:06, Dr. David Alan Gilbert (git) wrote: > > From: "Dr. David Alan Gilbert" > > > > 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 > > --- > > 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 / dgilb...@redhat.com / Manchester, UK
Re: [Qemu-devel] [PATCH v2 1/5] rcu: Add automatically released rcu_read_lock variant
On 11/09/19 21:06, Dr. David Alan Gilbert (git) wrote: > From: "Dr. David Alan Gilbert" > > 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 > --- > 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
Re: [Qemu-devel] [PATCH v2 1/5] rcu: Add automatically released rcu_read_lock variant
On Wed, Sep 11, 2019 at 08:06:18PM +0100, Dr. David Alan Gilbert (git) wrote: > From: "Dr. David Alan Gilbert" > > 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 > --- > include/qemu/rcu.h | 18 ++ > 1 file changed, 18 insertions(+) Reviewed-by: Daniel P. Berrangé 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 :|
[Qemu-devel] [PATCH v2 1/5] rcu: Add automatically released rcu_read_lock variant
From: "Dr. David Alan Gilbert" 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 --- 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