* [Qemu-devel] [PATCH 0/3] Automatic RCU read unlock @ 2019-09-11 16:41 Dr. David Alan Gilbert (git) 2019-09-11 16:42 ` [Qemu-devel] [PATCH 1/3] rcu: Add automatically released rcu_read_lock variant Dr. David Alan Gilbert (git) ` (4 more replies) 0 siblings, 5 replies; 21+ messages in thread From: Dr. David Alan Gilbert (git) @ 2019-09-11 16:41 UTC (permalink / raw) To: qemu-devel, pbonzini, berrange, quintela From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> This patch uses glib's g_auto mechanism to automatically free rcu_read_lock's at the end of the block. Given that humans have a habit of forgetting an error path somewhere it's best to leave it to the compiler. In particular: https://bugzilla.redhat.com/show_bug.cgi?id=1746787 suggests we've forgotten an unlock case somewhere in the rdma migration code. Dave Dr. David Alan Gilbert (3): rcu: Add automatically released rcu_read_lock variant migration: Use automatic rcu_read unlock in ram.c migration: Use automatic rcu_read unlock in rdma.c include/qemu/rcu.h | 12 ++++++++++ migration/ram.c | 25 ++++++++------------ migration/rdma.c | 57 +++++++++------------------------------------- 3 files changed, 32 insertions(+), 62 deletions(-) -- 2.21.0 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [Qemu-devel] [PATCH 1/3] rcu: Add automatically released rcu_read_lock variant 2019-09-11 16:41 [Qemu-devel] [PATCH 0/3] Automatic RCU read unlock Dr. David Alan Gilbert (git) @ 2019-09-11 16:42 ` Dr. David Alan Gilbert (git) 2019-09-11 16:56 ` Daniel P. Berrangé 2019-09-11 17:40 ` Eric Blake 2019-09-11 16:42 ` [Qemu-devel] [PATCH 2/3] migration: Use automatic rcu_read unlock in ram.c Dr. David Alan Gilbert (git) ` (3 subsequent siblings) 4 siblings, 2 replies; 21+ messages in thread From: Dr. David Alan Gilbert (git) @ 2019-09-11 16:42 UTC (permalink / raw) To: qemu-devel, pbonzini, berrange, quintela 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(); + #ifdef __cplusplus } #endif -- 2.21.0 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] rcu: Add automatically released rcu_read_lock variant 2019-09-11 16:42 ` [Qemu-devel] [PATCH 1/3] rcu: Add automatically released rcu_read_lock variant Dr. David Alan Gilbert (git) @ 2019-09-11 16:56 ` Daniel P. Berrangé 2019-09-11 17:04 ` Dr. David Alan Gilbert 2019-09-11 17:56 ` Eric Blake 2019-09-11 17:40 ` Eric Blake 1 sibling, 2 replies; 21+ messages in thread From: Daniel P. Berrangé @ 2019-09-11 16:56 UTC (permalink / raw) To: Dr. David Alan Gilbert (git); +Cc: pbonzini, qemu-devel, quintela 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 -- |: 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 :| ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] rcu: Add automatically released rcu_read_lock variant 2019-09-11 16:56 ` Daniel P. Berrangé @ 2019-09-11 17:04 ` Dr. David Alan Gilbert 2019-09-11 17:09 ` Daniel P. Berrangé 2019-09-11 17:56 ` Eric Blake 1 sibling, 1 reply; 21+ messages in thread From: Dr. David Alan Gilbert @ 2019-09-11 17:04 UTC (permalink / raw) To: Daniel P. Berrangé; +Cc: pbonzini, qemu-devel, quintela * 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] rcu: Add automatically released rcu_read_lock variant 2019-09-11 17:04 ` Dr. David Alan Gilbert @ 2019-09-11 17:09 ` Daniel P. Berrangé 2019-09-11 17:10 ` Dr. David Alan Gilbert 0 siblings, 1 reply; 21+ messages in thread From: Daniel P. Berrangé @ 2019-09-11 17:09 UTC (permalink / raw) To: Dr. David Alan Gilbert; +Cc: pbonzini, qemu-devel, quintela 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 -- |: 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 :| ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] rcu: Add automatically released rcu_read_lock variant 2019-09-11 17:09 ` Daniel P. Berrangé @ 2019-09-11 17:10 ` Dr. David Alan Gilbert 2019-09-11 17:16 ` Daniel P. Berrangé 0 siblings, 1 reply; 21+ messages in thread From: Dr. David Alan Gilbert @ 2019-09-11 17:10 UTC (permalink / raw) To: Daniel P. Berrangé; +Cc: pbonzini, qemu-devel, quintela * 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] rcu: Add automatically released rcu_read_lock variant 2019-09-11 17:10 ` Dr. David Alan Gilbert @ 2019-09-11 17:16 ` Daniel P. Berrangé 2019-09-11 17:18 ` Dr. David Alan Gilbert 0 siblings, 1 reply; 21+ messages in thread From: Daniel P. Berrangé @ 2019-09-11 17:16 UTC (permalink / raw) To: Dr. David Alan Gilbert; +Cc: pbonzini, qemu-devel, quintela 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 -- |: 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 :| ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] rcu: Add automatically released rcu_read_lock variant 2019-09-11 17:16 ` Daniel P. Berrangé @ 2019-09-11 17:18 ` Dr. David Alan Gilbert 0 siblings, 0 replies; 21+ messages in thread From: Dr. David Alan Gilbert @ 2019-09-11 17:18 UTC (permalink / raw) To: Daniel P. Berrangé; +Cc: pbonzini, qemu-devel, quintela * 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] rcu: Add automatically released rcu_read_lock variant 2019-09-11 16:56 ` Daniel P. Berrangé 2019-09-11 17:04 ` Dr. David Alan Gilbert @ 2019-09-11 17:56 ` Eric Blake 2019-09-11 18:49 ` Dr. David Alan Gilbert 1 sibling, 1 reply; 21+ messages in thread From: Eric Blake @ 2019-09-11 17:56 UTC (permalink / raw) To: Daniel P. Berrangé, Dr. David Alan Gilbert (git) Cc: pbonzini, qemu-devel, quintela [-- Attachment #1.1: Type: text/plain, Size: 1671 bytes --] 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. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] rcu: Add automatically released rcu_read_lock variant 2019-09-11 17:56 ` Eric Blake @ 2019-09-11 18:49 ` Dr. David Alan Gilbert 0 siblings, 0 replies; 21+ messages in thread From: Dr. David Alan Gilbert @ 2019-09-11 18:49 UTC (permalink / raw) To: Eric Blake; +Cc: pbonzini, Daniel P. Berrangé, qemu-devel, quintela * 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] rcu: Add automatically released rcu_read_lock variant 2019-09-11 16:42 ` [Qemu-devel] [PATCH 1/3] rcu: Add automatically released rcu_read_lock variant Dr. David Alan Gilbert (git) 2019-09-11 16:56 ` Daniel P. Berrangé @ 2019-09-11 17:40 ` Eric Blake 2019-09-11 17:49 ` Eric Blake 2019-09-11 18:52 ` Dr. David Alan Gilbert 1 sibling, 2 replies; 21+ messages in thread From: Eric Blake @ 2019-09-11 17:40 UTC (permalink / raw) To: Dr. David Alan Gilbert (git), qemu-devel, pbonzini, berrange, quintela [-- Attachment #1.1: Type: text/plain, Size: 1884 bytes --] 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, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] rcu: Add automatically released rcu_read_lock variant 2019-09-11 17:40 ` Eric Blake @ 2019-09-11 17:49 ` Eric Blake 2019-09-11 18:27 ` Dr. David Alan Gilbert 2019-09-11 18:52 ` Dr. David Alan Gilbert 1 sibling, 1 reply; 21+ messages in thread From: Eric Blake @ 2019-09-11 17:49 UTC (permalink / raw) To: Dr. David Alan Gilbert (git), qemu-devel, pbonzini, berrange, quintela [-- Attachment #1.1: Type: text/plain, Size: 1182 bytes --] 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, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] rcu: Add automatically released rcu_read_lock variant 2019-09-11 17:49 ` Eric Blake @ 2019-09-11 18:27 ` Dr. David Alan Gilbert 0 siblings, 0 replies; 21+ messages in thread From: Dr. David Alan Gilbert @ 2019-09-11 18:27 UTC (permalink / raw) To: Eric Blake; +Cc: pbonzini, berrange, qemu-devel, quintela * 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] rcu: Add automatically released rcu_read_lock variant 2019-09-11 17:40 ` Eric Blake 2019-09-11 17:49 ` Eric Blake @ 2019-09-11 18:52 ` Dr. David Alan Gilbert 1 sibling, 0 replies; 21+ messages in thread From: Dr. David Alan Gilbert @ 2019-09-11 18:52 UTC (permalink / raw) To: Eric Blake; +Cc: pbonzini, berrange, qemu-devel, quintela * 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [Qemu-devel] [PATCH 2/3] migration: Use automatic rcu_read unlock in ram.c 2019-09-11 16:41 [Qemu-devel] [PATCH 0/3] Automatic RCU read unlock Dr. David Alan Gilbert (git) 2019-09-11 16:42 ` [Qemu-devel] [PATCH 1/3] rcu: Add automatically released rcu_read_lock variant Dr. David Alan Gilbert (git) @ 2019-09-11 16:42 ` Dr. David Alan Gilbert (git) 2019-09-11 16:59 ` Daniel P. Berrangé 2019-09-11 16:42 ` [Qemu-devel] [PATCH 3/3] migration: Use automatic rcu_read unlock in rdma.c Dr. David Alan Gilbert (git) ` (2 subsequent siblings) 4 siblings, 1 reply; 21+ messages in thread From: Dr. David Alan Gilbert (git) @ 2019-09-11 16:42 UTC (permalink / raw) To: qemu-devel, pbonzini, berrange, quintela From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> Use the automatic read unlocker in migration/ram.c; only for the cases where the unlock is at the end of the function. Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> --- migration/ram.c | 25 +++++++++---------------- 1 file changed, 9 insertions(+), 16 deletions(-) diff --git a/migration/ram.c b/migration/ram.c index b2bd618a89..750d198f37 100644 --- a/migration/ram.c +++ b/migration/ram.c @@ -181,14 +181,14 @@ int foreach_not_ignored_block(RAMBlockIterFunc func, void *opaque) RAMBlock *block; int ret = 0; - rcu_read_lock(); + RCU_READ_LOCK_AUTO + RAMBLOCK_FOREACH_NOT_IGNORED(block) { ret = func(block, opaque); if (ret) { break; } } - rcu_read_unlock(); return ret; } @@ -2398,13 +2398,12 @@ static void migration_page_queue_free(RAMState *rs) /* This queue generally should be empty - but in the case of a failed * migration might have some droppings in. */ - rcu_read_lock(); + RCU_READ_LOCK_AUTO QSIMPLEQ_FOREACH_SAFE(mspr, &rs->src_page_requests, next_req, next_mspr) { memory_region_unref(mspr->rb->mr); QSIMPLEQ_REMOVE_HEAD(&rs->src_page_requests, next_req); g_free(mspr); } - rcu_read_unlock(); } /** @@ -2425,7 +2424,8 @@ int ram_save_queue_pages(const char *rbname, ram_addr_t start, ram_addr_t len) RAMState *rs = ram_state; ram_counters.postcopy_requests++; - rcu_read_lock(); + RCU_READ_LOCK_AUTO + if (!rbname) { /* Reuse last RAMBlock */ ramblock = rs->last_req_rb; @@ -2467,12 +2467,10 @@ int ram_save_queue_pages(const char *rbname, ram_addr_t start, ram_addr_t len) QSIMPLEQ_INSERT_TAIL(&rs->src_page_requests, new_entry, next_req); migration_make_urgent_request(); qemu_mutex_unlock(&rs->src_page_req_mutex); - rcu_read_unlock(); return 0; err: - rcu_read_unlock(); return -1; } @@ -2712,7 +2710,8 @@ static uint64_t ram_bytes_total_common(bool count_ignored) RAMBlock *block; uint64_t total = 0; - rcu_read_lock(); + RCU_READ_LOCK_AUTO + if (count_ignored) { RAMBLOCK_FOREACH_MIGRATABLE(block) { total += block->used_length; @@ -2722,7 +2721,6 @@ static uint64_t ram_bytes_total_common(bool count_ignored) total += block->used_length; } } - rcu_read_unlock(); return total; } @@ -3086,7 +3084,7 @@ int ram_postcopy_send_discard_bitmap(MigrationState *ms) RAMBlock *block; int ret; - rcu_read_lock(); + RCU_READ_LOCK_AUTO /* This should be our last sync, the src is now paused */ migration_bitmap_sync(rs); @@ -3107,13 +3105,11 @@ int ram_postcopy_send_discard_bitmap(MigrationState *ms) * point. */ error_report("migration ram resized during precopy phase"); - rcu_read_unlock(); return -EINVAL; } /* Deal with TPS != HPS and huge pages */ ret = postcopy_chunk_hostpages(ms, block); if (ret) { - rcu_read_unlock(); return ret; } @@ -3128,7 +3124,6 @@ int ram_postcopy_send_discard_bitmap(MigrationState *ms) trace_ram_postcopy_send_discard_bitmap(); ret = postcopy_each_ram_send_discard(ms); - rcu_read_unlock(); return ret; } @@ -3149,7 +3144,7 @@ int ram_discard_range(const char *rbname, uint64_t start, size_t length) trace_ram_discard_range(rbname, start, length); - rcu_read_lock(); + RCU_READ_LOCK_AUTO RAMBlock *rb = qemu_ram_block_by_name(rbname); if (!rb) { @@ -3169,8 +3164,6 @@ int ram_discard_range(const char *rbname, uint64_t start, size_t length) ret = ram_block_discard_range(rb, start, length); err: - rcu_read_unlock(); - return ret; } -- 2.21.0 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 2/3] migration: Use automatic rcu_read unlock in ram.c 2019-09-11 16:42 ` [Qemu-devel] [PATCH 2/3] migration: Use automatic rcu_read unlock in ram.c Dr. David Alan Gilbert (git) @ 2019-09-11 16:59 ` Daniel P. Berrangé 2019-09-11 17:25 ` Dr. David Alan Gilbert 0 siblings, 1 reply; 21+ messages in thread From: Daniel P. Berrangé @ 2019-09-11 16:59 UTC (permalink / raw) To: Dr. David Alan Gilbert (git); +Cc: pbonzini, qemu-devel, quintela On Wed, Sep 11, 2019 at 05:42:01PM +0100, Dr. David Alan Gilbert (git) wrote: > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > Use the automatic read unlocker in migration/ram.c; > only for the cases where the unlock is at the end of the function. > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > --- > migration/ram.c | 25 +++++++++---------------- > 1 file changed, 9 insertions(+), 16 deletions(-) > > diff --git a/migration/ram.c b/migration/ram.c > index b2bd618a89..750d198f37 100644 > --- a/migration/ram.c > +++ b/migration/ram.c > @@ -181,14 +181,14 @@ int foreach_not_ignored_block(RAMBlockIterFunc func, void *opaque) > RAMBlock *block; > int ret = 0; > > - rcu_read_lock(); > + RCU_READ_LOCK_AUTO FWIW, I'm not a fan of macros which are used without a trailing ';' as I believe it can confuses editors' code indentation logic. 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 :| ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 2/3] migration: Use automatic rcu_read unlock in ram.c 2019-09-11 16:59 ` Daniel P. Berrangé @ 2019-09-11 17:25 ` Dr. David Alan Gilbert 0 siblings, 0 replies; 21+ messages in thread From: Dr. David Alan Gilbert @ 2019-09-11 17:25 UTC (permalink / raw) To: Daniel P. Berrangé; +Cc: pbonzini, qemu-devel, quintela * Daniel P. Berrangé (berrange@redhat.com) wrote: > On Wed, Sep 11, 2019 at 05:42:01PM +0100, Dr. David Alan Gilbert (git) wrote: > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > > > Use the automatic read unlocker in migration/ram.c; > > only for the cases where the unlock is at the end of the function. > > > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > --- > > migration/ram.c | 25 +++++++++---------------- > > 1 file changed, 9 insertions(+), 16 deletions(-) > > > > diff --git a/migration/ram.c b/migration/ram.c > > index b2bd618a89..750d198f37 100644 > > --- a/migration/ram.c > > +++ b/migration/ram.c > > @@ -181,14 +181,14 @@ int foreach_not_ignored_block(RAMBlockIterFunc func, void *opaque) > > RAMBlock *block; > > int ret = 0; > > > > - rcu_read_lock(); > > + RCU_READ_LOCK_AUTO > > FWIW, I'm not a fan of macros which are used without a trailing ';' > as I believe it can confuses editors' code indentation logic. Added. > > 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [Qemu-devel] [PATCH 3/3] migration: Use automatic rcu_read unlock in rdma.c 2019-09-11 16:41 [Qemu-devel] [PATCH 0/3] Automatic RCU read unlock Dr. David Alan Gilbert (git) 2019-09-11 16:42 ` [Qemu-devel] [PATCH 1/3] rcu: Add automatically released rcu_read_lock variant Dr. David Alan Gilbert (git) 2019-09-11 16:42 ` [Qemu-devel] [PATCH 2/3] migration: Use automatic rcu_read unlock in ram.c Dr. David Alan Gilbert (git) @ 2019-09-11 16:42 ` Dr. David Alan Gilbert (git) 2019-09-11 16:58 ` [Qemu-devel] [PATCH 0/3] Automatic RCU read unlock Daniel P. Berrangé 2019-09-11 20:30 ` no-reply 4 siblings, 0 replies; 21+ messages in thread From: Dr. David Alan Gilbert (git) @ 2019-09-11 16:42 UTC (permalink / raw) To: qemu-devel, pbonzini, berrange, quintela From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> Use the automatic read unlocker in migration/rdma.c. Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> --- migration/rdma.c | 57 ++++++++++-------------------------------------- 1 file changed, 11 insertions(+), 46 deletions(-) diff --git a/migration/rdma.c b/migration/rdma.c index 78e6b72bac..40f8292ab9 100644 --- a/migration/rdma.c +++ b/migration/rdma.c @@ -88,7 +88,6 @@ static uint32_t known_capabilities = RDMA_CAPABILITY_PIN_ALL; " to abort!"); \ rdma->error_reported = 1; \ } \ - rcu_read_unlock(); \ return rdma->error_state; \ } \ } while (0) @@ -2678,11 +2677,10 @@ static ssize_t qio_channel_rdma_writev(QIOChannel *ioc, size_t i; size_t len = 0; - rcu_read_lock(); + RCU_READ_LOCK_AUTO rdma = atomic_rcu_read(&rioc->rdmaout); if (!rdma) { - rcu_read_unlock(); return -EIO; } @@ -2695,7 +2693,6 @@ static ssize_t qio_channel_rdma_writev(QIOChannel *ioc, ret = qemu_rdma_write_flush(f, rdma); if (ret < 0) { rdma->error_state = ret; - rcu_read_unlock(); return ret; } @@ -2715,7 +2712,6 @@ static ssize_t qio_channel_rdma_writev(QIOChannel *ioc, if (ret < 0) { rdma->error_state = ret; - rcu_read_unlock(); return ret; } @@ -2724,7 +2720,6 @@ static ssize_t qio_channel_rdma_writev(QIOChannel *ioc, } } - rcu_read_unlock(); return done; } @@ -2764,11 +2759,10 @@ static ssize_t qio_channel_rdma_readv(QIOChannel *ioc, ssize_t i; size_t done = 0; - rcu_read_lock(); + RCU_READ_LOCK_AUTO rdma = atomic_rcu_read(&rioc->rdmain); if (!rdma) { - rcu_read_unlock(); return -EIO; } @@ -2805,7 +2799,6 @@ static ssize_t qio_channel_rdma_readv(QIOChannel *ioc, if (ret < 0) { rdma->error_state = ret; - rcu_read_unlock(); return ret; } @@ -2819,14 +2812,12 @@ static ssize_t qio_channel_rdma_readv(QIOChannel *ioc, /* Still didn't get enough, so lets just return */ if (want) { if (done == 0) { - rcu_read_unlock(); return QIO_CHANNEL_ERR_BLOCK; } else { break; } } } - rcu_read_unlock(); return done; } @@ -2882,7 +2873,7 @@ qio_channel_rdma_source_prepare(GSource *source, GIOCondition cond = 0; *timeout = -1; - rcu_read_lock(); + RCU_READ_LOCK_AUTO if (rsource->condition == G_IO_IN) { rdma = atomic_rcu_read(&rsource->rioc->rdmain); } else { @@ -2891,7 +2882,6 @@ qio_channel_rdma_source_prepare(GSource *source, if (!rdma) { error_report("RDMAContext is NULL when prepare Gsource"); - rcu_read_unlock(); return FALSE; } @@ -2900,7 +2890,6 @@ qio_channel_rdma_source_prepare(GSource *source, } cond |= G_IO_OUT; - rcu_read_unlock(); return cond & rsource->condition; } @@ -2911,7 +2900,7 @@ qio_channel_rdma_source_check(GSource *source) RDMAContext *rdma; GIOCondition cond = 0; - rcu_read_lock(); + RCU_READ_LOCK_AUTO if (rsource->condition == G_IO_IN) { rdma = atomic_rcu_read(&rsource->rioc->rdmain); } else { @@ -2920,7 +2909,6 @@ qio_channel_rdma_source_check(GSource *source) if (!rdma) { error_report("RDMAContext is NULL when check Gsource"); - rcu_read_unlock(); return FALSE; } @@ -2929,7 +2917,6 @@ qio_channel_rdma_source_check(GSource *source) } cond |= G_IO_OUT; - rcu_read_unlock(); return cond & rsource->condition; } @@ -2943,7 +2930,7 @@ qio_channel_rdma_source_dispatch(GSource *source, RDMAContext *rdma; GIOCondition cond = 0; - rcu_read_lock(); + RCU_READ_LOCK_AUTO if (rsource->condition == G_IO_IN) { rdma = atomic_rcu_read(&rsource->rioc->rdmain); } else { @@ -2952,7 +2939,6 @@ qio_channel_rdma_source_dispatch(GSource *source, if (!rdma) { error_report("RDMAContext is NULL when dispatch Gsource"); - rcu_read_unlock(); return FALSE; } @@ -2961,7 +2947,6 @@ qio_channel_rdma_source_dispatch(GSource *source, } cond |= G_IO_OUT; - rcu_read_unlock(); return (*func)(QIO_CHANNEL(rsource->rioc), (cond & rsource->condition), user_data); @@ -3058,7 +3043,7 @@ qio_channel_rdma_shutdown(QIOChannel *ioc, QIOChannelRDMA *rioc = QIO_CHANNEL_RDMA(ioc); RDMAContext *rdmain, *rdmaout; - rcu_read_lock(); + RCU_READ_LOCK_AUTO rdmain = atomic_rcu_read(&rioc->rdmain); rdmaout = atomic_rcu_read(&rioc->rdmain); @@ -3085,7 +3070,6 @@ qio_channel_rdma_shutdown(QIOChannel *ioc, break; } - rcu_read_unlock(); return 0; } @@ -3131,18 +3115,16 @@ static size_t qemu_rdma_save_page(QEMUFile *f, void *opaque, RDMAContext *rdma; int ret; - rcu_read_lock(); + RCU_READ_LOCK_AUTO rdma = atomic_rcu_read(&rioc->rdmaout); if (!rdma) { - rcu_read_unlock(); return -EIO; } CHECK_ERROR_STATE(); if (migration_in_postcopy()) { - rcu_read_unlock(); return RAM_SAVE_CONTROL_NOT_SUPP; } @@ -3227,11 +3209,9 @@ static size_t qemu_rdma_save_page(QEMUFile *f, void *opaque, } } - rcu_read_unlock(); return RAM_SAVE_CONTROL_DELAYED; err: rdma->error_state = ret; - rcu_read_unlock(); return ret; } @@ -3451,11 +3431,10 @@ static int qemu_rdma_registration_handle(QEMUFile *f, void *opaque) int count = 0; int i = 0; - rcu_read_lock(); + RCU_READ_LOCK_AUTO rdma = atomic_rcu_read(&rioc->rdmain); if (!rdma) { - rcu_read_unlock(); return -EIO; } @@ -3698,7 +3677,6 @@ out: if (ret < 0) { rdma->error_state = ret; } - rcu_read_unlock(); return ret; } @@ -3716,11 +3694,10 @@ rdma_block_notification_handle(QIOChannelRDMA *rioc, const char *name) int curr; int found = -1; - rcu_read_lock(); + RCU_READ_LOCK_AUTO rdma = atomic_rcu_read(&rioc->rdmain); if (!rdma) { - rcu_read_unlock(); return -EIO; } @@ -3734,7 +3711,6 @@ rdma_block_notification_handle(QIOChannelRDMA *rioc, const char *name) if (found == -1) { error_report("RAMBlock '%s' not found on destination", name); - rcu_read_unlock(); return -ENOENT; } @@ -3742,7 +3718,6 @@ rdma_block_notification_handle(QIOChannelRDMA *rioc, const char *name) trace_rdma_block_notification_handle(name, rdma->next_src_index); rdma->next_src_index++; - rcu_read_unlock(); return 0; } @@ -3767,17 +3742,15 @@ static int qemu_rdma_registration_start(QEMUFile *f, void *opaque, QIOChannelRDMA *rioc = QIO_CHANNEL_RDMA(opaque); RDMAContext *rdma; - rcu_read_lock(); + RCU_READ_LOCK_AUTO rdma = atomic_rcu_read(&rioc->rdmaout); if (!rdma) { - rcu_read_unlock(); return -EIO; } CHECK_ERROR_STATE(); if (migration_in_postcopy()) { - rcu_read_unlock(); return 0; } @@ -3785,7 +3758,6 @@ static int qemu_rdma_registration_start(QEMUFile *f, void *opaque, qemu_put_be64(f, RAM_SAVE_FLAG_HOOK); qemu_fflush(f); - rcu_read_unlock(); return 0; } @@ -3802,17 +3774,15 @@ static int qemu_rdma_registration_stop(QEMUFile *f, void *opaque, RDMAControlHeader head = { .len = 0, .repeat = 1 }; int ret = 0; - rcu_read_lock(); + RCU_READ_LOCK_AUTO rdma = atomic_rcu_read(&rioc->rdmaout); if (!rdma) { - rcu_read_unlock(); return -EIO; } CHECK_ERROR_STATE(); if (migration_in_postcopy()) { - rcu_read_unlock(); return 0; } @@ -3844,7 +3814,6 @@ static int qemu_rdma_registration_stop(QEMUFile *f, void *opaque, qemu_rdma_reg_whole_ram_blocks : NULL); if (ret < 0) { ERROR(errp, "receiving remote info!"); - rcu_read_unlock(); return ret; } @@ -3868,7 +3837,6 @@ static int qemu_rdma_registration_stop(QEMUFile *f, void *opaque, "not identical on both the source and destination.", local->nb_blocks, nb_dest_blocks); rdma->error_state = -EINVAL; - rcu_read_unlock(); return -EINVAL; } @@ -3885,7 +3853,6 @@ static int qemu_rdma_registration_stop(QEMUFile *f, void *opaque, local->block[i].length, rdma->dest_blocks[i].length); rdma->error_state = -EINVAL; - rcu_read_unlock(); return -EINVAL; } local->block[i].remote_host_addr = @@ -3903,11 +3870,9 @@ static int qemu_rdma_registration_stop(QEMUFile *f, void *opaque, goto err; } - rcu_read_unlock(); return 0; err: rdma->error_state = ret; - rcu_read_unlock(); return ret; } -- 2.21.0 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 0/3] Automatic RCU read unlock 2019-09-11 16:41 [Qemu-devel] [PATCH 0/3] Automatic RCU read unlock Dr. David Alan Gilbert (git) ` (2 preceding siblings ...) 2019-09-11 16:42 ` [Qemu-devel] [PATCH 3/3] migration: Use automatic rcu_read unlock in rdma.c Dr. David Alan Gilbert (git) @ 2019-09-11 16:58 ` Daniel P. Berrangé 2019-09-11 17:13 ` Dr. David Alan Gilbert 2019-09-11 20:30 ` no-reply 4 siblings, 1 reply; 21+ messages in thread From: Daniel P. Berrangé @ 2019-09-11 16:58 UTC (permalink / raw) To: Dr. David Alan Gilbert (git); +Cc: pbonzini, qemu-devel, quintela On Wed, Sep 11, 2019 at 05:41:59PM +0100, Dr. David Alan Gilbert (git) wrote: > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > This patch uses glib's g_auto mechanism to automatically free > rcu_read_lock's at the end of the block. Given that humans > have a habit of forgetting an error path somewhere it's > best to leave it to the compiler. > > In particular: > https://bugzilla.redhat.com/show_bug.cgi?id=1746787 > suggests we've forgotten an unlock case somewhere in the > rdma migration code. Probably worth mentioning this in the commit message of the 3rd patch so someone reading history sees that the patch wasn't just a no-op conversion, but instead actively fixing a bug. 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 :| ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 0/3] Automatic RCU read unlock 2019-09-11 16:58 ` [Qemu-devel] [PATCH 0/3] Automatic RCU read unlock Daniel P. Berrangé @ 2019-09-11 17:13 ` Dr. David Alan Gilbert 0 siblings, 0 replies; 21+ messages in thread From: Dr. David Alan Gilbert @ 2019-09-11 17:13 UTC (permalink / raw) To: Daniel P. Berrangé; +Cc: pbonzini, qemu-devel, quintela * Daniel P. Berrangé (berrange@redhat.com) wrote: > On Wed, Sep 11, 2019 at 05:41:59PM +0100, Dr. David Alan Gilbert (git) wrote: > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > > > This patch uses glib's g_auto mechanism to automatically free > > rcu_read_lock's at the end of the block. Given that humans > > have a habit of forgetting an error path somewhere it's > > best to leave it to the compiler. > > > > In particular: > > https://bugzilla.redhat.com/show_bug.cgi?id=1746787 > > suggests we've forgotten an unlock case somewhere in the > > rdma migration code. > > Probably worth mentioning this in the commit message of the 3rd patch > so someone reading history sees that the patch wasn't just a no-op > conversion, but instead actively fixing a bug. Added. 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 0/3] Automatic RCU read unlock 2019-09-11 16:41 [Qemu-devel] [PATCH 0/3] Automatic RCU read unlock Dr. David Alan Gilbert (git) ` (3 preceding siblings ...) 2019-09-11 16:58 ` [Qemu-devel] [PATCH 0/3] Automatic RCU read unlock Daniel P. Berrangé @ 2019-09-11 20:30 ` no-reply 4 siblings, 0 replies; 21+ messages in thread From: no-reply @ 2019-09-11 20:30 UTC (permalink / raw) To: dgilbert; +Cc: pbonzini, berrange, qemu-devel, quintela Patchew URL: https://patchew.org/QEMU/20190911164202.31136-1-dgilbert@redhat.com/ Hi, This series seems to have some coding style problems. See output below for more information: Subject: [Qemu-devel] [PATCH 0/3] Automatic RCU read unlock Message-id: 20190911164202.31136-1-dgilbert@redhat.com Type: series === TEST SCRIPT BEGIN === #!/bin/bash git rev-parse base > /dev/null || exit 0 git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram ./scripts/checkpatch.pl --mailback base.. === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 Switched to a new branch 'test' bf33be9 migration: Use automatic rcu_read unlock in rdma.c c64f2f4 migration: Use automatic rcu_read unlock in ram.c af6a608 rcu: Add automatically released rcu_read_lock variant === OUTPUT BEGIN === 1/3 Checking commit af6a608b908d (rcu: Add automatically released rcu_read_lock variant) ERROR: Macros with multiple statements should be enclosed in a do - while loop #33: FILE: include/qemu/rcu.h:165: +#define RCU_READ_LOCK_AUTO g_auto(rcu_read_auto_t) \ + _rcu_read_auto = 'x'; \ + rcu_read_lock(); total: 1 errors, 0 warnings, 18 lines checked Patch 1/3 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. 2/3 Checking commit c64f2f4c923f (migration: Use automatic rcu_read unlock in ram.c) 3/3 Checking commit bf33be959c2b (migration: Use automatic rcu_read unlock in rdma.c) === OUTPUT END === Test command exited with code: 1 The full log is available at http://patchew.org/logs/20190911164202.31136-1-dgilbert@redhat.com/testing.checkpatch/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-devel@redhat.com ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2019-09-11 20:33 UTC | newest] Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-09-11 16:41 [Qemu-devel] [PATCH 0/3] Automatic RCU read unlock Dr. David Alan Gilbert (git) 2019-09-11 16:42 ` [Qemu-devel] [PATCH 1/3] rcu: Add automatically released rcu_read_lock variant Dr. David Alan Gilbert (git) 2019-09-11 16:56 ` Daniel P. Berrangé 2019-09-11 17:04 ` Dr. David Alan Gilbert 2019-09-11 17:09 ` Daniel P. Berrangé 2019-09-11 17:10 ` Dr. David Alan Gilbert 2019-09-11 17:16 ` Daniel P. Berrangé 2019-09-11 17:18 ` Dr. David Alan Gilbert 2019-09-11 17:56 ` Eric Blake 2019-09-11 18:49 ` Dr. David Alan Gilbert 2019-09-11 17:40 ` Eric Blake 2019-09-11 17:49 ` Eric Blake 2019-09-11 18:27 ` Dr. David Alan Gilbert 2019-09-11 18:52 ` Dr. David Alan Gilbert 2019-09-11 16:42 ` [Qemu-devel] [PATCH 2/3] migration: Use automatic rcu_read unlock in ram.c Dr. David Alan Gilbert (git) 2019-09-11 16:59 ` Daniel P. Berrangé 2019-09-11 17:25 ` Dr. David Alan Gilbert 2019-09-11 16:42 ` [Qemu-devel] [PATCH 3/3] migration: Use automatic rcu_read unlock in rdma.c Dr. David Alan Gilbert (git) 2019-09-11 16:58 ` [Qemu-devel] [PATCH 0/3] Automatic RCU read unlock Daniel P. Berrangé 2019-09-11 17:13 ` Dr. David Alan Gilbert 2019-09-11 20:30 ` no-reply
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).