[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 3/5] argo: don't pointlessly use get_domain_by_id()
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; > } >
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |