[Xen-devel] [PATCH] adjust a few RCU domain locking calls

["Jan Beulich" <JBeulich@xxxxxxxx>]

x86's do_physdev_op() had a case where the locking was entirely
superfluous. Its physdev_map_pirq() further had a case where the lock
was being obtained too early, needlessly complicating early exit paths.

Grant table code had two open coded instances of
rcu_lock_target_domain_by_id(), and a third code section could be
consolidated by using the newly introduced helper function.

The memory hypercall code had two more instances of open coding
rcu_lock_target_domain_by_id(), but note that here this is not just
cleanup, but also fixes an error return path in memory_exchange() to
actually return an error.

Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

--- a/xen/arch/x86/physdev.c
+++ b/xen/arch/x86/physdev.c
@@ -90,14 +90,10 @@ static int physdev_hvm_map_pirq(
 int physdev_map_pirq(domid_t domid, int type, int *index, int *pirq_p,
                      struct msi_info *msi)
 {
-    struct domain *d;
+    struct domain *d = current->domain;
     int pirq, irq, ret = 0;
     void *map_data = NULL;
 
-    ret = rcu_lock_target_domain_by_id(domid, &d);
-    if ( ret )
-        return ret;
-
     if ( domid == DOMID_SELF && is_hvm_domain(d) )
     {
         /*
@@ -105,14 +101,15 @@ int physdev_map_pirq(domid_t domid, int 
          * calls back into itself and deadlocks on hvm_domain.irq_lock.
          */
         if ( !is_hvm_pv_evtchn_domain(d) )
-        {
-            ret = -EINVAL;
-            goto free_domain;
-        }
-        ret = physdev_hvm_map_pirq(d, type, index, pirq_p);
-        goto free_domain;
+            return -EINVAL;
+
+        return physdev_hvm_map_pirq(d, type, index, pirq_p);
     }
 
+    ret = rcu_lock_target_domain_by_id(domid, &d);
+    if ( ret )
+        return ret;
+
     if ( !IS_PRIV_FOR(current->domain, d) )
     {
         ret = -EPERM;
@@ -696,13 +693,12 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_H
     }
     case PHYSDEVOP_get_free_pirq: {
         struct physdev_get_free_pirq out;
-        struct domain *d;
+        struct domain *d = v->domain;
 
         ret = -EFAULT;
         if ( copy_from_guest(&out, arg, 1) != 0 )
             break;
 
-        d = rcu_lock_current_domain();
         spin_lock(&d->event_lock);
 
         ret = get_free_pirq(d, out.type);
@@ -717,7 +713,6 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_H
         }
 
         spin_unlock(&d->event_lock);
-        rcu_unlock_domain(d);
 
         if ( ret >= 0 )
         {
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -24,7 +24,7 @@
  * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
  */
 
-#include <xen/config.h>
+#include <xen/err.h>
 #include <xen/iocap.h>
 #include <xen/lib.h>
 #include <xen/sched.h>
@@ -195,6 +195,30 @@ double_gt_unlock(struct grant_table *lgt
         spin_unlock(&rgt->lock);
 }
 
+static struct domain *gt_lock_target_domain_by_id(domid_t dom)
+{
+    struct domain *d;
+    int rc = GNTST_general_error;
+
+    switch ( rcu_lock_target_domain_by_id(dom, &d) )
+    {
+    case 0:
+        return d;
+
+    case -ESRCH:
+        gdprintk(XENLOG_INFO, "Bad domid %d.\n", dom);
+        rc = GNTST_bad_domain;
+        break;
+
+    case -EPERM:
+        rc = GNTST_permission_denied;
+        break;
+    }
+
+    ASSERT(rc < 0 && -rc <= MAX_ERRNO);
+    return ERR_PTR(rc);
+}
+
 static inline int
 __get_maptrack_handle(
     struct grant_table *t)
@@ -1261,7 +1285,6 @@ gnttab_setup_table(
     struct grant_table *gt;
     int            i;
     unsigned long  gmfn;
-    domid_t        dom;
 
     if ( count != 1 )
         return -EINVAL;
@@ -1281,25 +1304,11 @@ gnttab_setup_table(
         goto out1;
     }
 
-    dom = op.dom;
-    if ( dom == DOMID_SELF )
+    d = gt_lock_target_domain_by_id(op.dom);
+    if ( IS_ERR(d) )
     {
-        d = rcu_lock_current_domain();
-    }
-    else
-    {
-        if ( unlikely((d = rcu_lock_domain_by_id(dom)) == NULL) )
-        {
-            gdprintk(XENLOG_INFO, "Bad domid %d.\n", dom);
-            op.status = GNTST_bad_domain;
-            goto out1;
-        }
-
-        if ( unlikely(!IS_PRIV_FOR(current->domain, d)) )
-        {
-            op.status = GNTST_permission_denied;
-            goto out2;
-        }
+        op.status = PTR_ERR(d);
+        goto out1;
     }
 
     if ( xsm_grant_setup(current->domain, d) )
@@ -1352,7 +1361,6 @@ gnttab_query_size(
 {
     struct gnttab_query_size op;
     struct domain *d;
-    domid_t        dom;
     int rc;
 
     if ( count != 1 )
@@ -1364,25 +1372,11 @@ gnttab_query_size(
         return -EFAULT;
     }
 
-    dom = op.dom;
-    if ( dom == DOMID_SELF )
-    {
-        d = rcu_lock_current_domain();
-    }
-    else
+    d = gt_lock_target_domain_by_id(op.dom);
+    if ( IS_ERR(d) )
     {
-        if ( unlikely((d = rcu_lock_domain_by_id(dom)) == NULL) )
-        {
-            gdprintk(XENLOG_INFO, "Bad domid %d.\n", dom);
-            op.status = GNTST_bad_domain;
-            goto query_out;
-        }
-
-        if ( unlikely(!IS_PRIV_FOR(current->domain, d)) )
-        {
-            op.status = GNTST_permission_denied;
-            goto query_out_unlock;
-        }
+        op.status = PTR_ERR(d);
+        goto query_out;
     }
 
     rc = xsm_grant_query_size(current->domain, d);
@@ -2240,15 +2234,10 @@ gnttab_get_status_frames(XEN_GUEST_HANDL
         return -EFAULT;
     }
 
-    rc = rcu_lock_target_domain_by_id(op.dom, &d);
-    if ( rc < 0 )
+    d = gt_lock_target_domain_by_id(op.dom);
+    if ( IS_ERR(d) )
     {
-        if ( rc == -ESRCH )
-            op.status = GNTST_bad_domain;
-        else if ( rc == -EPERM )
-            op.status = GNTST_permission_denied;
-        else
-            op.status = GNTST_general_error;
+        op.status = PTR_ERR(d);
         goto out1;
     }
     rc = xsm_grant_setup(current->domain, d);
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -329,22 +329,9 @@ static long memory_exchange(XEN_GUEST_HA
         out_chunk_order = exch.in.extent_order - exch.out.extent_order;
     }
 
-    if ( likely(exch.in.domid == DOMID_SELF) )
-    {
-        d = rcu_lock_current_domain();
-    }
-    else
-    {
-        if ( (d = rcu_lock_domain_by_id(exch.in.domid)) == NULL )
-            goto fail_early;
-
-        if ( !IS_PRIV_FOR(current->domain, d) )
-        {
-            rcu_unlock_domain(d);
-            rc = -EPERM;
-            goto fail_early;
-        }
-    }
+    rc = rcu_lock_target_domain_by_id(exch.in.domid, &d);
+    if ( rc )
+        goto fail_early;
 
     memflags |= MEMF_bits(domain_clamp_alloc_bitsize(
         d,
@@ -583,20 +570,8 @@ long do_memory_op(unsigned long cmd, XEN
              && (reservation.mem_flags & XENMEMF_populate_on_demand) )
             args.memflags |= MEMF_populate_on_demand;
 
-        if ( likely(reservation.domid == DOMID_SELF) )
-        {
-            d = rcu_lock_current_domain();
-        }
-        else
-        {
-            if ( (d = rcu_lock_domain_by_id(reservation.domid)) == NULL )
-                return start_extent;
-            if ( !IS_PRIV_FOR(current->domain, d) )
-            {
-                rcu_unlock_domain(d);
-                return start_extent;
-            }
-        }
+        if ( unlikely(rcu_lock_target_domain_by_id(reservation.domid, &d)) )
+            return start_extent;
         args.domain = d;
 
         rc = xsm_memory_adjust_reservation(current->domain, d);

Attachment: adjust-rcu-lock-domain.patch
Description: Text document

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel