diff mbox series

[1/3] rcu: Add automatically released rcu_read_lock variant

Message ID 20190911164202.31136-2-dgilbert@redhat.com
State New
Headers show
Series Automatic RCU read unlock | expand

Commit Message

Dr. David Alan Gilbert Sept. 11, 2019, 4:42 p.m. UTC 
From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

RCU_READ_LOCK_AUTO takes the rcu_read_lock  and then uses glib's
g_auto infrastrcture (and thus whatever the compilers hooks are) to
release it on all exits of the block.

Note this macro has a variable declaration in, and hence is not in
a while loop.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 include/qemu/rcu.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Daniel P. Berrangé Sept. 11, 2019, 4:56 p.m. UTC | #1 
On Wed, Sep 11, 2019 at 05:42:00PM +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 infrastrcture (and thus whatever the compilers hooks are) to
> release it on all exits of the block.
> 
> Note this macro has a variable declaration in, and hence is not in
> a while loop.
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  include/qemu/rcu.h | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/include/qemu/rcu.h b/include/qemu/rcu.h
> index 22876d1428..6a25b27d28 100644
> --- a/include/qemu/rcu.h
> +++ b/include/qemu/rcu.h
> @@ -154,6 +154,18 @@ extern void call_rcu1(struct rcu_head *head, RCUCBFunc *func);
>        }),                                                                \
>        (RCUCBFunc *)g_free);
>  
> +typedef char rcu_read_auto_t;
> +static inline void rcu_read_auto_unlock(rcu_read_auto_t *r)
> +{
> +  rcu_read_unlock();
> +}
> +
> +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(rcu_read_auto_t, rcu_read_auto_unlock)
>
> +#define RCU_READ_LOCK_AUTO g_auto(rcu_read_auto_t) \
> +    _rcu_read_auto = 'x'; \
> +    rcu_read_lock();
> +

Functionally this works, but my gut feeling would be to follow
the design of GMutexLocker as-is:

  https://developer.gnome.org/glib/stable/glib-Threads.html#g-mutex-locker-new

so you get a use pattern of

  g_autoptr(rcu_read_locker) locker = rcu_read_locker_new();

This makes it explicit that the code is creating a variable here, which
in turns means it is clear to force unlock early with

  g_clear_pointer(&locker, rcu_read_locker_free)


Regards,
Daniel
Dr. David Alan Gilbert Sept. 11, 2019, 5:04 p.m. UTC | #2 
* Daniel P. Berrangé (berrange@redhat.com) wrote:
> On Wed, Sep 11, 2019 at 05:42:00PM +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 infrastrcture (and thus whatever the compilers hooks are) to
> > release it on all exits of the block.
> > 
> > Note this macro has a variable declaration in, and hence is not in
> > a while loop.
> > 
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > ---
> >  include/qemu/rcu.h | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/include/qemu/rcu.h b/include/qemu/rcu.h
> > index 22876d1428..6a25b27d28 100644
> > --- a/include/qemu/rcu.h
> > +++ b/include/qemu/rcu.h
> > @@ -154,6 +154,18 @@ extern void call_rcu1(struct rcu_head *head, RCUCBFunc *func);
> >        }),                                                                \
> >        (RCUCBFunc *)g_free);
> >  
> > +typedef char rcu_read_auto_t;
> > +static inline void rcu_read_auto_unlock(rcu_read_auto_t *r)
> > +{
> > +  rcu_read_unlock();
> > +}
> > +
> > +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(rcu_read_auto_t, rcu_read_auto_unlock)
> >
> > +#define RCU_READ_LOCK_AUTO g_auto(rcu_read_auto_t) \
> > +    _rcu_read_auto = 'x'; \
> > +    rcu_read_lock();
> > +
> 
> Functionally this works, but my gut feeling would be to follow
> the design of GMutexLocker as-is:
> 
>   https://developer.gnome.org/glib/stable/glib-Threads.html#g-mutex-locker-new
> 
> so you get a use pattern of
> 
>   g_autoptr(rcu_read_locker) locker = rcu_read_locker_new();
> 
> This makes it explicit that the code is creating a variable here, which
> in turns means it is clear to force unlock early with
> 
>   g_clear_pointer(&locker, rcu_read_locker_free)

The difference compared to the g-mutex-locker is that I don't have
another object to use as my pointer; that uses the address of the GMutex
as the dummy pointer value.  I did try an experiment with g_autoptr
and found that it did need to return a non-NULL value for it to work,
which then lead me to think what value to use - while it seems to work
if I return (void *)1 it makes me nervous.

Dave

> 
> 
> 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 :|
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Daniel P. Berrangé Sept. 11, 2019, 5:09 p.m. UTC | #3 
On Wed, Sep 11, 2019 at 06:04:23PM +0100, Dr. David Alan Gilbert wrote:
> * Daniel P. Berrangé (berrange@redhat.com) wrote:
> > On Wed, Sep 11, 2019 at 05:42:00PM +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 infrastrcture (and thus whatever the compilers hooks are) to
> > > release it on all exits of the block.
> > > 
> > > Note this macro has a variable declaration in, and hence is not in
> > > a while loop.
> > > 
> > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > > ---
> > >  include/qemu/rcu.h | 12 ++++++++++++
> > >  1 file changed, 12 insertions(+)
> > > 
> > > diff --git a/include/qemu/rcu.h b/include/qemu/rcu.h
> > > index 22876d1428..6a25b27d28 100644
> > > --- a/include/qemu/rcu.h
> > > +++ b/include/qemu/rcu.h
> > > @@ -154,6 +154,18 @@ extern void call_rcu1(struct rcu_head *head, RCUCBFunc *func);
> > >        }),                                                                \
> > >        (RCUCBFunc *)g_free);
> > >  
> > > +typedef char rcu_read_auto_t;
> > > +static inline void rcu_read_auto_unlock(rcu_read_auto_t *r)
> > > +{
> > > +  rcu_read_unlock();
> > > +}
> > > +
> > > +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(rcu_read_auto_t, rcu_read_auto_unlock)
> > >
> > > +#define RCU_READ_LOCK_AUTO g_auto(rcu_read_auto_t) \
> > > +    _rcu_read_auto = 'x'; \
> > > +    rcu_read_lock();
> > > +
> > 
> > Functionally this works, but my gut feeling would be to follow
> > the design of GMutexLocker as-is:
> > 
> >   https://developer.gnome.org/glib/stable/glib-Threads.html#g-mutex-locker-new
> > 
> > so you get a use pattern of
> > 
> >   g_autoptr(rcu_read_locker) locker = rcu_read_locker_new();
> > 
> > This makes it explicit that the code is creating a variable here, which
> > in turns means it is clear to force unlock early with
> > 
> >   g_clear_pointer(&locker, rcu_read_locker_free)
> 
> The difference compared to the g-mutex-locker is that I don't have
> another object to use as my pointer; that uses the address of the GMutex
> as the dummy pointer value.  I did try an experiment with g_autoptr
> and found that it did need to return a non-NULL value for it to work,
> which then lead me to think what value to use - while it seems to work
> if I return (void *)1 it makes me nervous.

Yeah, '(void*)1' would have been what I'd pick. The only thing that the
value is used for is to pass to the rcu_read_locker_free() function
which ignores it, which seems safe enough.

Regards,
Daniel
Dr. David Alan Gilbert Sept. 11, 2019, 5:10 p.m. UTC | #4 
* Daniel P. Berrangé (berrange@redhat.com) wrote:
> On Wed, Sep 11, 2019 at 06:04:23PM +0100, Dr. David Alan Gilbert wrote:
> > * Daniel P. Berrangé (berrange@redhat.com) wrote:
> > > On Wed, Sep 11, 2019 at 05:42:00PM +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 infrastrcture (and thus whatever the compilers hooks are) to
> > > > release it on all exits of the block.
> > > > 
> > > > Note this macro has a variable declaration in, and hence is not in
> > > > a while loop.
> > > > 
> > > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > > > ---
> > > >  include/qemu/rcu.h | 12 ++++++++++++
> > > >  1 file changed, 12 insertions(+)
> > > > 
> > > > diff --git a/include/qemu/rcu.h b/include/qemu/rcu.h
> > > > index 22876d1428..6a25b27d28 100644
> > > > --- a/include/qemu/rcu.h
> > > > +++ b/include/qemu/rcu.h
> > > > @@ -154,6 +154,18 @@ extern void call_rcu1(struct rcu_head *head, RCUCBFunc *func);
> > > >        }),                                                                \
> > > >        (RCUCBFunc *)g_free);
> > > >  
> > > > +typedef char rcu_read_auto_t;
> > > > +static inline void rcu_read_auto_unlock(rcu_read_auto_t *r)
> > > > +{
> > > > +  rcu_read_unlock();
> > > > +}
> > > > +
> > > > +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(rcu_read_auto_t, rcu_read_auto_unlock)
> > > >
> > > > +#define RCU_READ_LOCK_AUTO g_auto(rcu_read_auto_t) \
> > > > +    _rcu_read_auto = 'x'; \
> > > > +    rcu_read_lock();
> > > > +
> > > 
> > > Functionally this works, but my gut feeling would be to follow
> > > the design of GMutexLocker as-is:
> > > 
> > >   https://developer.gnome.org/glib/stable/glib-Threads.html#g-mutex-locker-new
> > > 
> > > so you get a use pattern of
> > > 
> > >   g_autoptr(rcu_read_locker) locker = rcu_read_locker_new();
> > > 
> > > This makes it explicit that the code is creating a variable here, which
> > > in turns means it is clear to force unlock early with
> > > 
> > >   g_clear_pointer(&locker, rcu_read_locker_free)
> > 
> > The difference compared to the g-mutex-locker is that I don't have
> > another object to use as my pointer; that uses the address of the GMutex
> > as the dummy pointer value.  I did try an experiment with g_autoptr
> > and found that it did need to return a non-NULL value for it to work,
> > which then lead me to think what value to use - while it seems to work
> > if I return (void *)1 it makes me nervous.
> 
> Yeah, '(void*)1' would have been what I'd pick. The only thing that the
> value is used for is to pass to the rcu_read_locker_free() function
> which ignores it, which seems safe enough.

glib seems to be at least checking it; if you pass NULL the free'r
doesn't get called; so it worries me that we'd be relying on the current
definition.

Dave

> 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 :|
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Daniel P. Berrangé Sept. 11, 2019, 5:16 p.m. UTC | #5 
On Wed, Sep 11, 2019 at 06:10:28PM +0100, Dr. David Alan Gilbert wrote:
> * Daniel P. Berrangé (berrange@redhat.com) wrote:
> > On Wed, Sep 11, 2019 at 06:04:23PM +0100, Dr. David Alan Gilbert wrote:
> > > * Daniel P. Berrangé (berrange@redhat.com) wrote:
> > > > On Wed, Sep 11, 2019 at 05:42:00PM +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 infrastrcture (and thus whatever the compilers hooks are) to
> > > > > release it on all exits of the block.
> > > > > 
> > > > > Note this macro has a variable declaration in, and hence is not in
> > > > > a while loop.
> > > > > 
> > > > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > > > > ---
> > > > >  include/qemu/rcu.h | 12 ++++++++++++
> > > > >  1 file changed, 12 insertions(+)
> > > > > 
> > > > > diff --git a/include/qemu/rcu.h b/include/qemu/rcu.h
> > > > > index 22876d1428..6a25b27d28 100644
> > > > > --- a/include/qemu/rcu.h
> > > > > +++ b/include/qemu/rcu.h
> > > > > @@ -154,6 +154,18 @@ extern void call_rcu1(struct rcu_head *head, RCUCBFunc *func);
> > > > >        }),                                                                \
> > > > >        (RCUCBFunc *)g_free);
> > > > >  
> > > > > +typedef char rcu_read_auto_t;
> > > > > +static inline void rcu_read_auto_unlock(rcu_read_auto_t *r)
> > > > > +{
> > > > > +  rcu_read_unlock();
> > > > > +}
> > > > > +
> > > > > +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(rcu_read_auto_t, rcu_read_auto_unlock)
> > > > >
> > > > > +#define RCU_READ_LOCK_AUTO g_auto(rcu_read_auto_t) \
> > > > > +    _rcu_read_auto = 'x'; \
> > > > > +    rcu_read_lock();
> > > > > +
> > > > 
> > > > Functionally this works, but my gut feeling would be to follow
> > > > the design of GMutexLocker as-is:
> > > > 
> > > >   https://developer.gnome.org/glib/stable/glib-Threads.html#g-mutex-locker-new
> > > > 
> > > > so you get a use pattern of
> > > > 
> > > >   g_autoptr(rcu_read_locker) locker = rcu_read_locker_new();
> > > > 
> > > > This makes it explicit that the code is creating a variable here, which
> > > > in turns means it is clear to force unlock early with
> > > > 
> > > >   g_clear_pointer(&locker, rcu_read_locker_free)
> > > 
> > > The difference compared to the g-mutex-locker is that I don't have
> > > another object to use as my pointer; that uses the address of the GMutex
> > > as the dummy pointer value.  I did try an experiment with g_autoptr
> > > and found that it did need to return a non-NULL value for it to work,
> > > which then lead me to think what value to use - while it seems to work
> > > if I return (void *)1 it makes me nervous.
> > 
> > Yeah, '(void*)1' would have been what I'd pick. The only thing that the
> > value is used for is to pass to the rcu_read_locker_free() function
> > which ignores it, which seems safe enough.
> 
> glib seems to be at least checking it; if you pass NULL the free'r
> doesn't get called; so it worries me that we'd be relying on the current
> definition.

This NULL check is part of the API semantics defined for
G_DEFINE_AUTO_CLEANUO_FREE_FUNC. It lets you define
what the "empty" value is, typically 'NULL', but
in fact you don't need to use a pointer type at all. You
can use an 'int', for example, and declare that '-1'
is your "empty" value:

  https://developer.gnome.org/glib/stable/glib-Miscellaneous-Macros.html#G-DEFINE-AUTO-CLEANUP-FREE-FUNC:CAPS


Regards,
Daniel
Dr. David Alan Gilbert Sept. 11, 2019, 5:18 p.m. UTC | #6 
* Daniel P. Berrangé (berrange@redhat.com) wrote:
> On Wed, Sep 11, 2019 at 06:10:28PM +0100, Dr. David Alan Gilbert wrote:
> > * Daniel P. Berrangé (berrange@redhat.com) wrote:
> > > On Wed, Sep 11, 2019 at 06:04:23PM +0100, Dr. David Alan Gilbert wrote:
> > > > * Daniel P. Berrangé (berrange@redhat.com) wrote:
> > > > > On Wed, Sep 11, 2019 at 05:42:00PM +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 infrastrcture (and thus whatever the compilers hooks are) to
> > > > > > release it on all exits of the block.
> > > > > > 
> > > > > > Note this macro has a variable declaration in, and hence is not in
> > > > > > a while loop.
> > > > > > 
> > > > > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > > > > > ---
> > > > > >  include/qemu/rcu.h | 12 ++++++++++++
> > > > > >  1 file changed, 12 insertions(+)
> > > > > > 
> > > > > > diff --git a/include/qemu/rcu.h b/include/qemu/rcu.h
> > > > > > index 22876d1428..6a25b27d28 100644
> > > > > > --- a/include/qemu/rcu.h
> > > > > > +++ b/include/qemu/rcu.h
> > > > > > @@ -154,6 +154,18 @@ extern void call_rcu1(struct rcu_head *head, RCUCBFunc *func);
> > > > > >        }),                                                                \
> > > > > >        (RCUCBFunc *)g_free);
> > > > > >  
> > > > > > +typedef char rcu_read_auto_t;
> > > > > > +static inline void rcu_read_auto_unlock(rcu_read_auto_t *r)
> > > > > > +{
> > > > > > +  rcu_read_unlock();
> > > > > > +}
> > > > > > +
> > > > > > +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(rcu_read_auto_t, rcu_read_auto_unlock)
> > > > > >
> > > > > > +#define RCU_READ_LOCK_AUTO g_auto(rcu_read_auto_t) \
> > > > > > +    _rcu_read_auto = 'x'; \
> > > > > > +    rcu_read_lock();
> > > > > > +
> > > > > 
> > > > > Functionally this works, but my gut feeling would be to follow
> > > > > the design of GMutexLocker as-is:
> > > > > 
> > > > >   https://developer.gnome.org/glib/stable/glib-Threads.html#g-mutex-locker-new
> > > > > 
> > > > > so you get a use pattern of
> > > > > 
> > > > >   g_autoptr(rcu_read_locker) locker = rcu_read_locker_new();
> > > > > 
> > > > > This makes it explicit that the code is creating a variable here, which
> > > > > in turns means it is clear to force unlock early with
> > > > > 
> > > > >   g_clear_pointer(&locker, rcu_read_locker_free)
> > > > 
> > > > The difference compared to the g-mutex-locker is that I don't have
> > > > another object to use as my pointer; that uses the address of the GMutex
> > > > as the dummy pointer value.  I did try an experiment with g_autoptr
> > > > and found that it did need to return a non-NULL value for it to work,
> > > > which then lead me to think what value to use - while it seems to work
> > > > if I return (void *)1 it makes me nervous.
> > > 
> > > Yeah, '(void*)1' would have been what I'd pick. The only thing that the
> > > value is used for is to pass to the rcu_read_locker_free() function
> > > which ignores it, which seems safe enough.
> > 
> > glib seems to be at least checking it; if you pass NULL the free'r
> > doesn't get called; so it worries me that we'd be relying on the current
> > definition.
> 
> This NULL check is part of the API semantics defined for
> G_DEFINE_AUTO_CLEANUO_FREE_FUNC. It lets you define
> what the "empty" value is, typically 'NULL', but
> in fact you don't need to use a pointer type at all. You
> can use an 'int', for example, and declare that '-1'
> is your "empty" value:
> 
>   https://developer.gnome.org/glib/stable/glib-Miscellaneous-Macros.html#G-DEFINE-AUTO-CLEANUP-FREE-FUNC:CAPS

Ah OK, yep that makes sense; I'll flip it around.

Dave

> 
> 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 :|
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Eric Blake Sept. 11, 2019, 5:40 p.m. UTC | #7 
On 9/11/19 11:42 AM, 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 infrastrcture (and thus whatever the compilers hooks are) to
> release it on all exits of the block.
> 
> Note this macro has a variable declaration in, and hence is not in
> a while loop.
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  include/qemu/rcu.h | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/include/qemu/rcu.h b/include/qemu/rcu.h
> index 22876d1428..6a25b27d28 100644
> --- a/include/qemu/rcu.h
> +++ b/include/qemu/rcu.h
> @@ -154,6 +154,18 @@ extern void call_rcu1(struct rcu_head *head, RCUCBFunc *func);
>        }),                                                                \
>        (RCUCBFunc *)g_free);
>  
> +typedef char rcu_read_auto_t;

Declaring new types ending in _t collides with the namespace reserved by
POSIX.  While I don't think it will bite us, it's still worth
considering if a different name is better.

> +static inline void rcu_read_auto_unlock(rcu_read_auto_t *r)
> +{
> +  rcu_read_unlock();
> +}
> +
> +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(rcu_read_auto_t, rcu_read_auto_unlock)
> +
> +#define RCU_READ_LOCK_AUTO g_auto(rcu_read_auto_t) \
> +    _rcu_read_auto = 'x'; \

I'm a bit lost at where _rcu_read_auto is declared.  (I could understand
if an earlier macro had created that typedef via concatenating _ with
rcu_read_auto_t, but making the preprocessor drop _t is not possible. Is
this a typo, and if so, why did the compiler not complain?)

> +    rcu_read_lock();
> +
>  #ifdef __cplusplus
>  }
>  #endif
>
Eric Blake Sept. 11, 2019, 5:49 p.m. UTC | #8 
On 9/11/19 12:40 PM, Eric Blake wrote:

>> +
>> +#define RCU_READ_LOCK_AUTO g_auto(rcu_read_auto_t) \
>> +    _rcu_read_auto = 'x'; \
> 
> I'm a bit lost at where _rcu_read_auto is declared.  (I could understand
> if an earlier macro had created that typedef via concatenating _ with
> rcu_read_auto_t, but making the preprocessor drop _t is not possible. Is
> this a typo, and if so, why did the compiler not complain?)

Okay, I read it wrong.  This rendering would be easier for me to
understand (you are declaring a dummy variable right here):

#define RCU_READ_LOCK_AUTO \
    g_auto(rcu_read_auto_t) _rcu_read_auto = 'x'; \
...

In other words, I'm not used to expecting a split between type and
variable name across two lines, especially when the type is itself a
macro call, and where my first reading didn't spot that
(rcu_read_auto_t) was not the name of the argument to a mixed-case macro
RCU_READ_LOACK_AUTO_g_auto, rather than g_auto(...) being the start of
the parameter-less macro RCU_READ_LOCK_AUTO definition.
Eric Blake Sept. 11, 2019, 5:56 p.m. UTC | #9 
On 9/11/19 11:56 AM, Daniel P. Berrangé wrote:
> On Wed, Sep 11, 2019 at 05:42:00PM +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

spurious double space

>> g_auto infrastrcture (and thus whatever the compilers hooks are) to

infrastructure
compiler's

>> release it on all exits of the block.
>>
>> Note this macro has a variable declaration in, and hence is not in
>> a while loop.
>>

>> +#define RCU_READ_LOCK_AUTO g_auto(rcu_read_auto_t) \
>> +    _rcu_read_auto = 'x'; \
>> +    rcu_read_lock();
>> +
> 
> Functionally this works, but my gut feeling would be to follow
> the design of GMutexLocker as-is:
> 
>   https://developer.gnome.org/glib/stable/glib-Threads.html#g-mutex-locker-new
> 
> so you get a use pattern of
> 
>   g_autoptr(rcu_read_locker) locker = rcu_read_locker_new();

Another pattern to consider: nbdkit uses:

#define ACQUIRE_LOCK_FOR_CURRENT_SCOPE(mutex) \
  CLEANUP_UNLOCK pthread_mutex_t *_lock = mutex; \
  do { \
    int _r = pthread_mutex_lock (_lock); \
    assert (!_r); \
  } while (0)

with later code calling:

  ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lock);

> 
> This makes it explicit that the code is creating a variable here, which
> in turns means it is clear to force unlock early with
> 
>   g_clear_pointer(&locker, rcu_read_locker_free)

Yes, this aspect of glib is nicer than the corresponding nbdkit usage
pattern.
Dr. David Alan Gilbert Sept. 11, 2019, 6:27 p.m. UTC | #10 
* Eric Blake (eblake@redhat.com) wrote:
> On 9/11/19 12:40 PM, Eric Blake wrote:
> 
> >> +
> >> +#define RCU_READ_LOCK_AUTO g_auto(rcu_read_auto_t) \
> >> +    _rcu_read_auto = 'x'; \
> > 
> > I'm a bit lost at where _rcu_read_auto is declared.  (I could understand
> > if an earlier macro had created that typedef via concatenating _ with
> > rcu_read_auto_t, but making the preprocessor drop _t is not possible. Is
> > this a typo, and if so, why did the compiler not complain?)
> 
> Okay, I read it wrong.  This rendering would be easier for me to
> understand (you are declaring a dummy variable right here):
> 
> #define RCU_READ_LOCK_AUTO \
>     g_auto(rcu_read_auto_t) _rcu_read_auto = 'x'; \
> ...
> 
> In other words, I'm not used to expecting a split between type and
> variable name across two lines, especially when the type is itself a
> macro call, and where my first reading didn't spot that
> (rcu_read_auto_t) was not the name of the argument to a mixed-case macro
> RCU_READ_LOACK_AUTO_g_auto, rather than g_auto(...) being the start of
> the parameter-less macro RCU_READ_LOCK_AUTO definition.

Yep, that's simplified after the rework Dan suggested.

Dave

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



--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Dr. David Alan Gilbert Sept. 11, 2019, 6:49 p.m. UTC | #11 
* Eric Blake (eblake@redhat.com) wrote:
> On 9/11/19 11:56 AM, Daniel P. Berrangé wrote:
> > On Wed, Sep 11, 2019 at 05:42:00PM +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
> 
> spurious double space
> 
> >> g_auto infrastrcture (and thus whatever the compilers hooks are) to
> 
> infrastructure
> compiler's

Thanks.

Dave

> >> release it on all exits of the block.
> >>
> >> Note this macro has a variable declaration in, and hence is not in
> >> a while loop.
> >>
> 
> >> +#define RCU_READ_LOCK_AUTO g_auto(rcu_read_auto_t) \
> >> +    _rcu_read_auto = 'x'; \
> >> +    rcu_read_lock();
> >> +
> > 
> > Functionally this works, but my gut feeling would be to follow
> > the design of GMutexLocker as-is:
> > 
> >   https://developer.gnome.org/glib/stable/glib-Threads.html#g-mutex-locker-new
> > 
> > so you get a use pattern of
> > 
> >   g_autoptr(rcu_read_locker) locker = rcu_read_locker_new();
> 
> Another pattern to consider: nbdkit uses:
> 
> #define ACQUIRE_LOCK_FOR_CURRENT_SCOPE(mutex) \
>   CLEANUP_UNLOCK pthread_mutex_t *_lock = mutex; \
>   do { \
>     int _r = pthread_mutex_lock (_lock); \
>     assert (!_r); \
>   } while (0)
> 
> with later code calling:
> 
>   ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lock);
> 
> > 
> > This makes it explicit that the code is creating a variable here, which
> > in turns means it is clear to force unlock early with
> > 
> >   g_clear_pointer(&locker, rcu_read_locker_free)
> 
> Yes, this aspect of glib is nicer than the corresponding nbdkit usage
> pattern.
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3226
> Virtualization:  qemu.org | libvirt.org
> 



--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Dr. David Alan Gilbert Sept. 11, 2019, 6:52 p.m. UTC | #12 
* Eric Blake (eblake@redhat.com) wrote:
> On 9/11/19 11:42 AM, 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 infrastrcture (and thus whatever the compilers hooks are) to
> > release it on all exits of the block.
> > 
> > Note this macro has a variable declaration in, and hence is not in
> > a while loop.
> > 
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > ---
> >  include/qemu/rcu.h | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/include/qemu/rcu.h b/include/qemu/rcu.h
> > index 22876d1428..6a25b27d28 100644
> > --- a/include/qemu/rcu.h
> > +++ b/include/qemu/rcu.h
> > @@ -154,6 +154,18 @@ extern void call_rcu1(struct rcu_head *head, RCUCBFunc *func);
> >        }),                                                                \
> >        (RCUCBFunc *)g_free);
> >  
> > +typedef char rcu_read_auto_t;
> 
> Declaring new types ending in _t collides with the namespace reserved by
> POSIX.  While I don't think it will bite us, it's still worth
> considering if a different name is better.

Thanks, I've renamed it to 'RCUReadAuto' which is closer to what we
normally use for typedef's (albeit normally of structs)

Dave

> > +static inline void rcu_read_auto_unlock(rcu_read_auto_t *r)
> > +{
> > +  rcu_read_unlock();
> > +}
> > +
> > +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(rcu_read_auto_t, rcu_read_auto_unlock)
> > +
> > +#define RCU_READ_LOCK_AUTO g_auto(rcu_read_auto_t) \
> > +    _rcu_read_auto = 'x'; \
> 
> I'm a bit lost at where _rcu_read_auto is declared.  (I could understand
> if an earlier macro had created that typedef via concatenating _ with
> rcu_read_auto_t, but making the preprocessor drop _t is not possible. Is
> this a typo, and if so, why did the compiler not complain?)
> 
> > +    rcu_read_lock();
> > +
> >  #ifdef __cplusplus
> >  }
> >  #endif
> > 
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3226
> Virtualization:  qemu.org | libvirt.org
> 



--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff mbox series

Patch

diff --git a/include/qemu/rcu.h b/include/qemu/rcu.h
index 22876d1428..6a25b27d28 100644
--- a/include/qemu/rcu.h
+++ b/include/qemu/rcu.h
@@ -154,6 +154,18 @@  extern void call_rcu1(struct rcu_head *head, RCUCBFunc *func);
       }),                                                                \
       (RCUCBFunc *)g_free);
 
+typedef char rcu_read_auto_t;
+static inline void rcu_read_auto_unlock(rcu_read_auto_t *r)
+{
+  rcu_read_unlock();
+}
+
+G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(rcu_read_auto_t, rcu_read_auto_unlock)
+
+#define RCU_READ_LOCK_AUTO g_auto(rcu_read_auto_t) \
+    _rcu_read_auto = 'x'; \
+    rcu_read_lock();
+
 #ifdef __cplusplus
 }
 #endif