[Qemu-devel] [PATCH 0/3] Automatic RCU read unlock

[Qemu-devel] [PATCH 0/3] Automatic RCU read unlock

[Dr. David Alan Gilbert (git)]

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

[Qemu-devel] [PATCH 1/3] rcu: Add automatically released rcu_read_lock variant

[Dr. David Alan Gilbert (git)]

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

[Qemu-devel] [PATCH 2/3] migration: Use automatic rcu_read unlock in ram.c

[Dr. David Alan Gilbert (git)]

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

[Qemu-devel] [PATCH 3/3] migration: Use automatic rcu_read unlock in rdma.c

[Dr. David Alan Gilbert (git)]

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

Re: [Qemu-devel] [PATCH 1/3] rcu: Add automatically released rcu_read_lock variant

[Daniel P. Berrangé]

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 :|

Re: [Qemu-devel] [PATCH 0/3] Automatic RCU read unlock

[Daniel P. Berrangé]

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 :|

Re: [Qemu-devel] [PATCH 2/3] migration: Use automatic rcu_read unlock in ram.c

[Daniel P. Berrangé]

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 :|

Re: [Qemu-devel] [PATCH 1/3] rcu: Add automatically released rcu_read_lock variant

[Dr. David Alan Gilbert]

* 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

Re: [Qemu-devel] [PATCH 1/3] rcu: Add automatically released rcu_read_lock variant

[Daniel P. Berrangé]

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 :|

Re: [Qemu-devel] [PATCH 1/3] rcu: Add automatically released rcu_read_lock variant

[Dr. David Alan Gilbert]

* 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

Re: [Qemu-devel] [PATCH 0/3] Automatic RCU read unlock

[Dr. David Alan Gilbert]

* 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

Re: [Qemu-devel] [PATCH 1/3] rcu: Add automatically released rcu_read_lock variant

[Daniel P. Berrangé]

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 :|

Re: [Qemu-devel] [PATCH 1/3] rcu: Add automatically released rcu_read_lock variant

[Dr. David Alan Gilbert]

* 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

Re: [Qemu-devel] [PATCH 2/3] migration: Use automatic rcu_read unlock in ram.c

[Dr. David Alan Gilbert]

* 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

Re: [Qemu-devel] [PATCH 1/3] rcu: Add automatically released rcu_read_lock variant

[Eric Blake]

[-- 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 --]

Re: [Qemu-devel] [PATCH 1/3] rcu: Add automatically released rcu_read_lock variant

[Eric Blake]

[-- 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 --]

Re: [Qemu-devel] [PATCH 1/3] rcu: Add automatically released rcu_read_lock variant

[Eric Blake]

[-- 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 --]

Re: [Qemu-devel] [PATCH 1/3] rcu: Add automatically released rcu_read_lock variant

[Dr. David Alan Gilbert]

* 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

Re: [Qemu-devel] [PATCH 1/3] rcu: Add automatically released rcu_read_lock variant

[Dr. David Alan Gilbert]

* 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

Re: [Qemu-devel] [PATCH 1/3] rcu: Add automatically released rcu_read_lock variant

[Dr. David Alan Gilbert]

* 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

Re: [Qemu-devel] [PATCH 0/3] Automatic RCU read unlock

[no-reply]

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