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

[Keir Fraser <keir.xen@xxxxxxxxx>]

On 07/09/2012 13:56, "Jan Beulich" <JBeulich@xxxxxxxx> wrote:

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

Acked-by: Keir Fraser <keir@xxxxxxx>

> --- 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);
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel



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