Re: [PATCH 3/5] argo: don't pointlessly use get_domain_by_id()

[Christopher Clark <christopher.w.clark@xxxxxxxxx>]

On Tue, Jan 5, 2021 at 5:26 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> For short-lived references rcu_lock_domain_by_id() is the better
> (slightly cheaper) alternative.

This should scale better than the existing use of get_domain_by_id.

I actually found it pretty hard to study the code for the effects of
switching over to use the RCU domain reference logic, and I would have
been happier with reviewing if I'd been able to exercise it with some
focused testing; XTF needs support for tests with multiple test VMs to
enable Argo tests of communication and interactions with hypervisor
state.

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

Acked-by: Christopher Clark <christopher.w.clark@xxxxxxxxx>

> ---
> Is it really intentional for fill_ring_data() to return success (0) in
> case the domain can't be found or has argo disabled?

Good question; I think this logic can and should be improved. I will
work on a patch.

> Even if so, the
> uninitialized ent.max_message_size will be leaked to the caller then
> (and similarly when find_ring_info_by_match() returns NULL). Perhaps
> the field should only be copied back when ent.flags is non-zero?

Yes.

thanks,

Christopher

>
> --- a/xen/common/argo.c
> +++ b/xen/common/argo.c
> @@ -445,13 +445,13 @@ signal_domain(struct domain *d)
>  static void
>  signal_domid(domid_t domain_id)
>  {
> -    struct domain *d = get_domain_by_id(domain_id);
> +    struct domain *d = rcu_lock_domain_by_id(domain_id);
>
>      if ( !d )
>          return;
>
>      signal_domain(d);
> -    put_domain(d);
> +    rcu_unlock_domain(d);
>  }
>
>  static void
> @@ -983,7 +983,7 @@ ringbuf_insert(const struct domain *d, s
>  static void
>  wildcard_pending_list_remove(domid_t domain_id, struct pending_ent *ent)
>  {
> -    struct domain *d = get_domain_by_id(domain_id);
> +    struct domain *d = rcu_lock_domain_by_id(domain_id);
>
>      if ( !d )
>          return;
> @@ -996,13 +996,13 @@ wildcard_pending_list_remove(domid_t dom
>          list_del(&ent->wildcard_node);
>          spin_unlock(&d->argo->wildcard_L2_lock);
>      }
> -    put_domain(d);
> +    rcu_unlock_domain(d);
>  }
>
>  static void
>  wildcard_pending_list_insert(domid_t domain_id, struct pending_ent *ent)
>  {
> -    struct domain *d = get_domain_by_id(domain_id);
> +    struct domain *d = rcu_lock_domain_by_id(domain_id);
>
>      if ( !d )
>          return;
> @@ -1015,7 +1015,7 @@ wildcard_pending_list_insert(domid_t dom
>          list_add(&ent->wildcard_node, &d->argo->wildcard_pend_list);
>          spin_unlock(&d->argo->wildcard_L2_lock);
>      }
> -    put_domain(d);
> +    rcu_unlock_domain(d);
>  }
>
>  static void
> @@ -1283,7 +1283,7 @@ partner_rings_remove(struct domain *src_
>                                                        struct argo_send_info,
>                                                        node)) )
>          {
> -            struct domain *dst_d = get_domain_by_id(send_info->id.domain_id);
> +            struct domain *dst_d = 
> rcu_lock_domain_by_id(send_info->id.domain_id);
>
>              if ( dst_d && dst_d->argo )
>              {
> @@ -1302,7 +1302,7 @@ partner_rings_remove(struct domain *src_
>                  ASSERT_UNREACHABLE();
>
>              if ( dst_d )
> -                put_domain(dst_d);
> +                rcu_unlock_domain(dst_d);
>
>              list_del(&send_info->node);
>              xfree(send_info);
> @@ -1330,7 +1330,7 @@ fill_ring_data(const struct domain *curr
>
>      ent.flags = 0;
>
> -    dst_d = get_domain_by_id(ent.ring.domain_id);
> +    dst_d = rcu_lock_domain_by_id(ent.ring.domain_id);
>      if ( !dst_d || !dst_d->argo )
>          goto out;
>
> @@ -1340,10 +1340,7 @@ fill_ring_data(const struct domain *curr
>       */
>      ret = xsm_argo_send(currd, dst_d);
>      if ( ret )
> -    {
> -        put_domain(dst_d);
> -        return ret;
> -    }
> +        goto out;
>
>      read_lock(&dst_d->argo->rings_L2_rwlock);
>
> @@ -1405,7 +1402,7 @@ fill_ring_data(const struct domain *curr
>
>   out:
>      if ( dst_d )
> -        put_domain(dst_d);
> +        rcu_unlock_domain(dst_d);
>
>      if ( !ret && (__copy_field_to_guest(data_ent_hnd, &ent, flags) ||
>                    __copy_field_to_guest(data_ent_hnd, &ent, 
> max_message_size)) )
> @@ -1569,7 +1566,7 @@ unregister_ring(struct domain *currd,
>      if ( ring_id.partner_id == XEN_ARGO_DOMID_ANY )
>          goto out;
>
> -    dst_d = get_domain_by_id(ring_id.partner_id);
> +    dst_d = rcu_lock_domain_by_id(ring_id.partner_id);
>      if ( !dst_d || !dst_d->argo )
>      {
>          ASSERT_UNREACHABLE();
> @@ -1592,7 +1589,7 @@ unregister_ring(struct domain *currd,
>      read_unlock(&L1_global_argo_rwlock);
>
>      if ( dst_d )
> -        put_domain(dst_d);
> +        rcu_unlock_domain(dst_d);
>
>      xfree(send_info);
>
> @@ -1663,7 +1660,7 @@ register_ring(struct domain *currd,
>      }
>      else
>      {
> -        dst_d = get_domain_by_id(reg.partner_id);
> +        dst_d = rcu_lock_domain_by_id(reg.partner_id);
>          if ( !dst_d )
>          {
>              argo_dprintk("!dst_d, ESRCH\n");
> @@ -1845,7 +1842,7 @@ register_ring(struct domain *currd,
>
>   out:
>      if ( dst_d )
> -        put_domain(dst_d);
> +        rcu_unlock_domain(dst_d);
>
>      if ( ret )
>          xfree(send_info);
> @@ -1988,7 +1985,7 @@ sendv(struct domain *src_d, xen_argo_add
>      src_id.domain_id = src_d->domain_id;
>      src_id.partner_id = dst_addr->domain_id;
>
> -    dst_d = get_domain_by_id(dst_addr->domain_id);
> +    dst_d = rcu_lock_domain_by_id(dst_addr->domain_id);
>      if ( !dst_d )
>          return -ESRCH;
>
> @@ -1998,7 +1995,7 @@ sendv(struct domain *src_d, xen_argo_add
>          gprintk(XENLOG_ERR, "argo: XSM REJECTED %i -> %i\n",
>                  src_d->domain_id, dst_d->domain_id);
>
> -        put_domain(dst_d);
> +        rcu_unlock_domain(dst_d);
>
>          return ret;
>      }
> @@ -2068,7 +2065,7 @@ sendv(struct domain *src_d, xen_argo_add
>          signal_domain(dst_d);
>
>      if ( dst_d )
> -        put_domain(dst_d);
> +        rcu_unlock_domain(dst_d);
>
>      return ( ret < 0 ) ? ret : len;
>  }
>