[v2,1/5] rcu: Add automatically released rcu_read_lock variant

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(+)

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

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