[PATCH 0/2] rcu_read auto macro use

[PATCH 0/2] rcu_read auto macro use

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Hi,
  A couple more uses of the rcu_read macros; in qsp and
hyperv (neither of which list maintainers, so I guess
best through RCU).

The hyperv case saves a temporary.
The qsp case uses an rcu_read_lock around the lifetime
of a snapshot and carefully comments that; but now
it's automatic.

[Hyperv not tested]

Dave

Dr. David Alan Gilbert (2):
  hyperv: Use auto rcu_read macros
  qsp: Use WITH_RCU_READ_LOCK_GUARD

 hw/hyperv/hyperv.c | 22 +++++++++-------------
 util/qsp.c         | 22 ++++++++++------------
 2 files changed, 19 insertions(+), 25 deletions(-)

-- 
2.23.0

[PATCH 1/2] hyperv: Use auto rcu_read macros

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Use RCU_READ_LOCK_GUARD and WITH_RCU_READ_LOCK_GUARD
to replace the manual rcu_read_(un)lock calls.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 hw/hyperv/hyperv.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/hw/hyperv/hyperv.c b/hw/hyperv/hyperv.c
index 6ebf31c310..da8ce82725 100644
--- a/hw/hyperv/hyperv.c
+++ b/hw/hyperv/hyperv.c
@@ -546,14 +546,14 @@ uint16_t hyperv_hcall_post_message(uint64_t param, bool fast)
     }
 
     ret = HV_STATUS_INVALID_CONNECTION_ID;
-    rcu_read_lock();
-    QLIST_FOREACH_RCU(mh, &msg_handlers, link) {
-        if (mh->conn_id == (msg->connection_id & HV_CONNECTION_ID_MASK)) {
-            ret = mh->handler(msg, mh->data);
-            break;
+    WITH_RCU_READ_LOCK_GUARD() {
+        QLIST_FOREACH_RCU(mh, &msg_handlers, link) {
+            if (mh->conn_id == (msg->connection_id & HV_CONNECTION_ID_MASK)) {
+                ret = mh->handler(msg, mh->data);
+                break;
+            }
         }
     }
-    rcu_read_unlock();
 
 unmap:
     cpu_physical_memory_unmap(msg, len, 0, 0);
@@ -619,7 +619,6 @@ int hyperv_set_event_flag_handler(uint32_t conn_id, EventNotifier *notifier)
 
 uint16_t hyperv_hcall_signal_event(uint64_t param, bool fast)
 {
-    uint16_t ret;
     EventFlagHandler *handler;
 
     if (unlikely(!fast)) {
@@ -645,15 +644,12 @@ uint16_t hyperv_hcall_signal_event(uint64_t param, bool fast)
         return HV_STATUS_INVALID_HYPERCALL_INPUT;
     }
 
-    ret = HV_STATUS_INVALID_CONNECTION_ID;
-    rcu_read_lock();
+    RCU_READ_LOCK_GUARD();
     QLIST_FOREACH_RCU(handler, &event_flag_handlers, link) {
         if (handler->conn_id == param) {
             event_notifier_set(handler->notifier);
-            ret = 0;
-            break;
+            return 0;
         }
     }
-    rcu_read_unlock();
-    return ret;
+    return HV_STATUS_INVALID_CONNECTION_ID;
 }
-- 
2.23.0

[PATCH 2/2] qsp: Use WITH_RCU_READ_LOCK_GUARD

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

The automatic rcu read lock maintenance works quite
nicely in this case where it previously relied on a comment to
delimit the lifetime and now has a block.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 util/qsp.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/util/qsp.c b/util/qsp.c
index 62265417fd..7d5147f1b2 100644
--- a/util/qsp.c
+++ b/util/qsp.c
@@ -598,7 +598,6 @@ static void qsp_ht_delete(void *p, uint32_t h, void *htp)
 
 static void qsp_mktree(GTree *tree, bool callsite_coalesce)
 {
-    QSPSnapshot *snap;
     struct qht ht, coalesce_ht;
     struct qht *htp;
 
@@ -610,20 +609,19 @@ static void qsp_mktree(GTree *tree, bool callsite_coalesce)
      * We must remain in an RCU read-side critical section until we're done
      * with the snapshot.
      */
-    rcu_read_lock();
-    snap = atomic_rcu_read(&qsp_snapshot);
+    WITH_RCU_READ_LOCK_GUARD() {
+        QSPSnapshot *snap = atomic_rcu_read(&qsp_snapshot);
 
-    /* Aggregate all results from the global hash table into a local one */
-    qht_init(&ht, qsp_entry_no_thread_cmp, QSP_INITIAL_SIZE,
-             QHT_MODE_AUTO_RESIZE | QHT_MODE_RAW_MUTEXES);
-    qht_iter(&qsp_ht, qsp_aggregate, &ht);
+        /* Aggregate all results from the global hash table into a local one */
+        qht_init(&ht, qsp_entry_no_thread_cmp, QSP_INITIAL_SIZE,
+                 QHT_MODE_AUTO_RESIZE | QHT_MODE_RAW_MUTEXES);
+        qht_iter(&qsp_ht, qsp_aggregate, &ht);
 
-    /* compute the difference wrt the snapshot, if any */
-    if (snap) {
-        qsp_diff(&snap->ht, &ht);
+        /* compute the difference wrt the snapshot, if any */
+        if (snap) {
+            qsp_diff(&snap->ht, &ht);
+        }
     }
-    /* done with the snapshot; RCU can reclaim it */
-    rcu_read_unlock();
 
     htp = &ht;
     if (callsite_coalesce) {
-- 
2.23.0

Re: [PATCH 1/2] hyperv: Use auto rcu_read macros

[Roman Kagan]

On Fri, Dec 13, 2019 at 01:19:30PM +0000, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> Use RCU_READ_LOCK_GUARD and WITH_RCU_READ_LOCK_GUARD
> to replace the manual rcu_read_(un)lock calls.
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  hw/hyperv/hyperv.c | 22 +++++++++-------------
>  1 file changed, 9 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/hyperv/hyperv.c b/hw/hyperv/hyperv.c
> index 6ebf31c310..da8ce82725 100644
> --- a/hw/hyperv/hyperv.c
> +++ b/hw/hyperv/hyperv.c
> @@ -546,14 +546,14 @@ uint16_t hyperv_hcall_post_message(uint64_t param, bool fast)
>      }
>  
>      ret = HV_STATUS_INVALID_CONNECTION_ID;
> -    rcu_read_lock();
> -    QLIST_FOREACH_RCU(mh, &msg_handlers, link) {
> -        if (mh->conn_id == (msg->connection_id & HV_CONNECTION_ID_MASK)) {
> -            ret = mh->handler(msg, mh->data);
> -            break;
> +    WITH_RCU_READ_LOCK_GUARD() {
> +        QLIST_FOREACH_RCU(mh, &msg_handlers, link) {
> +            if (mh->conn_id == (msg->connection_id & HV_CONNECTION_ID_MASK)) {
> +                ret = mh->handler(msg, mh->data);
> +                break;
> +            }
>          }
>      }
> -    rcu_read_unlock();
>  
>  unmap:
>      cpu_physical_memory_unmap(msg, len, 0, 0);
> @@ -619,7 +619,6 @@ int hyperv_set_event_flag_handler(uint32_t conn_id, EventNotifier *notifier)
>  
>  uint16_t hyperv_hcall_signal_event(uint64_t param, bool fast)
>  {
> -    uint16_t ret;
>      EventFlagHandler *handler;
>  
>      if (unlikely(!fast)) {
> @@ -645,15 +644,12 @@ uint16_t hyperv_hcall_signal_event(uint64_t param, bool fast)
>          return HV_STATUS_INVALID_HYPERCALL_INPUT;
>      }
>  
> -    ret = HV_STATUS_INVALID_CONNECTION_ID;
> -    rcu_read_lock();
> +    RCU_READ_LOCK_GUARD();
>      QLIST_FOREACH_RCU(handler, &event_flag_handlers, link) {
>          if (handler->conn_id == param) {
>              event_notifier_set(handler->notifier);
> -            ret = 0;
> -            break;
> +            return 0;
>          }
>      }
> -    rcu_read_unlock();
> -    return ret;
> +    return HV_STATUS_INVALID_CONNECTION_ID;
>  }

I have a slight preference towards using WITH_RCU_READ_LOCK_GUARD
instead of sticking RCU_READ_LOCK_GUARD in the middle of the function
and implicitly relying on there being none but trivial statements past
the rcu-protected section.

Nothing that I would insist on, though, so

Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>

Re: [PATCH 0/2] rcu_read auto macro use

[Paolo Bonzini]

On 13/12/19 14:19, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> Hi,
>   A couple more uses of the rcu_read macros; in qsp and
> hyperv (neither of which list maintainers, so I guess
> best through RCU).
> 
> The hyperv case saves a temporary.
> The qsp case uses an rcu_read_lock around the lifetime
> of a snapshot and carefully comments that; but now
> it's automatic.
> 
> [Hyperv not tested]

Queued, thanks.

Paolo

> Dave
> 
> Dr. David Alan Gilbert (2):
>   hyperv: Use auto rcu_read macros
>   qsp: Use WITH_RCU_READ_LOCK_GUARD
> 
>  hw/hyperv/hyperv.c | 22 +++++++++-------------
>  util/qsp.c         | 22 ++++++++++------------
>  2 files changed, 19 insertions(+), 25 deletions(-)
>