* [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
* [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
* [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 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 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 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 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 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 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 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
* 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 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: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: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 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
* 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).