In the course of preparing v4 of "evtchn: (not so) recent XSAs follow-on" (which is a contextual prereq to some of the patches here) I've noticed some slight inefficiencies. I've then also looked for similar patterns elsewhere. 1: common: don't (kind of) open-code rcu_lock_domain_by_any_id() 2: evtchn: don't pointlessly use get_domain() 3: argo: don't pointlessly use get_domain_by_id() 4: x86: don't pointlessly use get_domain_by_id() 5: x86/mem-sharing: don't pointlessly use get_domain_by_id() Jan
Even more so when using rcu_lock_domain_by_id() in place of the more efficient rcu_lock_current_domain(). Signed-off-by: Jan Beulich <jbeulich@suse.com> --- Besides get_pg_owner(), gnttab_copy_lock_domain() has similar strange(?) behavior: They accept DOMID_SELF, but not the resolved ID of the caller. --- v4: New. --- a/xen/common/event_channel.c +++ b/xen/common/event_channel.c @@ -353,10 +353,7 @@ static int evtchn_bind_interdomain(evtch evtchn_port_t rport = bind->remote_port; domid_t rdom = bind->remote_dom; - if ( rdom == DOMID_SELF ) - rdom = current->domain->domain_id; - - if ( (rd = rcu_lock_domain_by_id(rdom)) == NULL ) + if ( (rd = rcu_lock_domain_by_any_id(rdom)) == NULL ) return -ESRCH; write_lock(&ld->event_lock); --- a/xen/common/page_alloc.c +++ b/xen/common/page_alloc.c @@ -2568,12 +2568,6 @@ struct domain *get_pg_owner(domid_t domi { struct domain *pg_owner = NULL, *curr = current->domain; - if ( likely(domid == DOMID_SELF) ) - { - pg_owner = rcu_lock_current_domain(); - goto out; - } - if ( unlikely(domid == curr->domain_id) ) { gdprintk(XENLOG_WARNING, "Cannot specify itself as foreign domain\n"); @@ -2591,7 +2585,7 @@ struct domain *get_pg_owner(domid_t domi break; default: - if ( (pg_owner = rcu_lock_domain_by_id(domid)) == NULL ) + if ( (pg_owner = rcu_lock_domain_by_any_id(domid)) == NULL ) gdprintk(XENLOG_WARNING, "Unknown domain d%d\n", domid); break; }
On 05/01/2021 13:24, Jan Beulich wrote: > Even more so when using rcu_lock_domain_by_id() in place of the more > efficient rcu_lock_current_domain(). > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > Besides get_pg_owner(), gnttab_copy_lock_domain() has similar strange(?) > behavior: They accept DOMID_SELF, but not the resolved ID of the caller. I queried that behaviour years and years ago. According to Keir, it is part of the attempt to force guests to not know their own domid. Xen itself does (or did) its very hardest to prevent a domU knowing its own domid, while at the same time the "domid" key in xenstore is mandatory for setting up PV rings. I'd personally think was a short sighted move. ~Andrew
On 05.01.2021 14:38, Andrew Cooper wrote: > On 05/01/2021 13:24, Jan Beulich wrote: >> Even more so when using rcu_lock_domain_by_id() in place of the more >> efficient rcu_lock_current_domain(). >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> Thanks. >> --- >> Besides get_pg_owner(), gnttab_copy_lock_domain() has similar strange(?) >> behavior: They accept DOMID_SELF, but not the resolved ID of the caller. > > I queried that behaviour years and years ago. > > According to Keir, it is part of the attempt to force guests to not know > their own domid. Xen itself does (or did) its very hardest to prevent a > domU knowing its own domid, while at the same time the "domid" key in > xenstore is mandatory for setting up PV rings. > > I'd personally think was a short sighted move. Let me make another patch then, unless you can see reasons we should stick to the current behavior. Figuring out its own domid is possible for a domain, after all, and even shouldn't be overly difficult. Jan
On 05/01/2021 14:17, Jan Beulich wrote: >>> --- >>> Besides get_pg_owner(), gnttab_copy_lock_domain() has similar strange(?) >>> behavior: They accept DOMID_SELF, but not the resolved ID of the caller. >> I queried that behaviour years and years ago. >> >> According to Keir, it is part of the attempt to force guests to not know >> their own domid. Xen itself does (or did) its very hardest to prevent a >> domU knowing its own domid, while at the same time the "domid" key in >> xenstore is mandatory for setting up PV rings. >> >> I'd personally think was a short sighted move. > Let me make another patch then, unless you can see reasons we should > stick to the current behavior. Figuring out its own domid is possible > for a domain, after all, and even shouldn't be overly difficult. I don't see any reason to keep these semantics, other than the general argument against relaxing things vs compatibility with older hypervisors. I honestly can't make up my mind, so whichever you prefer. ~Andrew
For short-lived references rcu_lock_domain() is the better (slightly cheaper) alternative. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/common/event_channel.c +++ b/xen/common/event_channel.c @@ -642,7 +642,7 @@ int evtchn_close(struct domain *d1, int */ write_unlock(&chn1->lock); write_unlock(&chn2->lock); - put_domain(d2); + rcu_unlock_domain(d2); chn2 = NULL; } @@ -677,7 +677,7 @@ int evtchn_close(struct domain *d1, int */ write_unlock(&chn1->lock); write_unlock(&chn2->lock); - put_domain(d2); + rcu_unlock_domain(d2); chn2 = NULL; } @@ -725,9 +725,8 @@ int evtchn_close(struct domain *d1, int if ( !chn2 ) { - /* If we unlock chn1 then we could lose d2. Must get a reference. */ - if ( unlikely(!get_domain(d2)) ) - BUG(); + /* If we unlock chn1 then we could lose d2. */ + rcu_lock_domain(d2); chn2 = _evtchn_from_port(d2, chn1->u.interdomain.remote_port); BUG_ON(!chn2); @@ -778,7 +777,7 @@ int evtchn_close(struct domain *d1, int */ write_unlock(&chn1->lock); write_unlock(&chn2->lock); - put_domain(d2); + rcu_unlock_domain(d2); } write_unlock(&d1->event_lock);
For short-lived references rcu_lock_domain_by_id() is the better (slightly cheaper) alternative. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- Is it really intentional for fill_ring_data() to return success (0) in case the domain can't be found or has argo disabled? 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? --- 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; }
On Tue, Jan 5, 2021 at 5:26 AM Jan Beulich <jbeulich@suse.com> 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@suse.com> Acked-by: Christopher Clark <christopher.w.clark@gmail.com> > --- > 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; > } >
For short-lived references rcu_lock_domain_by_id() is the better (slightly cheaper) alternative. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/arch/x86/cpu/mcheck/mcaction.c +++ b/xen/arch/x86/cpu/mcheck/mcaction.c @@ -87,7 +87,7 @@ mc_memerr_dhandler(struct mca_binfo *bin BUG_ON( bank->mc_domid == DOMID_COW ); if ( bank->mc_domid != DOMID_XEN ) { - d = get_domain_by_id(bank->mc_domid); + d = rcu_lock_domain_by_id(bank->mc_domid); ASSERT(d); gfn = get_gpfn_from_mfn((bank->mc_addr) >> PAGE_SHIFT); @@ -132,6 +132,8 @@ mc_memerr_dhandler(struct mca_binfo *bin goto vmce_failed; } + rcu_unlock_domain(d); + /* * Impacted domain go on with domain's recovery job * if the domain has its own MCA handler. @@ -139,12 +141,11 @@ mc_memerr_dhandler(struct mca_binfo *bin * its own recovery job. */ *result = MCER_RECOVERED; - put_domain(d); return; vmce_failed: - put_domain(d); domain_crash(d); + rcu_unlock_domain(d); } } } --- a/xen/arch/x86/cpu/mcheck/mce.c +++ b/xen/arch/x86/cpu/mcheck/mce.c @@ -1497,7 +1497,6 @@ long do_mca(XEN_GUEST_HANDLE_PARAM(xen_m if ( mc_msrinject->mcinj_flags & MC_MSRINJ_F_GPADDR ) { - domid_t domid; struct domain *d; struct mcinfo_msr *msr; unsigned int i; @@ -1505,17 +1504,17 @@ long do_mca(XEN_GUEST_HANDLE_PARAM(xen_m unsigned long gfn, mfn; p2m_type_t t; - domid = (mc_msrinject->mcinj_domid == DOMID_SELF) ? - current->domain->domain_id : mc_msrinject->mcinj_domid; - if ( domid >= DOMID_FIRST_RESERVED ) - return x86_mcerr("do_mca inject: incompatible flag " - "MC_MSRINJ_F_GPADDR with domain %d", - -EINVAL, domid); - - d = get_domain_by_id(domid); + d = rcu_lock_domain_by_any_id(mc_msrinject->mcinj_domid); if ( d == NULL ) + { + if ( mc_msrinject->mcinj_domid >= DOMID_FIRST_RESERVED ) + return x86_mcerr("do_mca inject: incompatible flag " + "MC_MSRINJ_F_GPADDR with domain %d", + -EINVAL, domid); + return x86_mcerr("do_mca inject: bad domain id %d", -EINVAL, domid); + } for ( i = 0, msr = &mc_msrinject->mcinj_msr[0]; i < mc_msrinject->mcinj_count; @@ -1528,7 +1527,7 @@ long do_mca(XEN_GUEST_HANDLE_PARAM(xen_m if ( mfn == mfn_x(INVALID_MFN) ) { put_gfn(d, gfn); - put_domain(d); + rcu_unlock_domain(d); return x86_mcerr("do_mca inject: bad gfn %#lx of domain %d", -EINVAL, gfn, domid); } @@ -1538,7 +1537,7 @@ long do_mca(XEN_GUEST_HANDLE_PARAM(xen_m put_gfn(d, gfn); } - put_domain(d); + rcu_unlock_domain(d); } if ( !x86_mc_msrinject_verify(mc_msrinject) ) --- a/xen/arch/x86/debug.c +++ b/xen/arch/x86/debug.c @@ -164,13 +164,13 @@ unsigned int dbg_rw_mem(void * __user ad unsigned int len, domid_t domid, bool toaddr, uint64_t pgd3) { - struct domain *d = get_domain_by_id(domid); + struct domain *d = rcu_lock_domain_by_id(domid); if ( d ) { if ( !d->is_dying ) len = dbg_rw_guest_mem(d, addr, buf, len, toaddr, pgd3); - put_domain(d); + rcu_unlock_domain(d); } return len; --- a/xen/arch/x86/irq.c +++ b/xen/arch/x86/irq.c @@ -323,7 +323,7 @@ void destroy_irq(unsigned int irq) if ( desc->arch.creator_domid != DOMID_INVALID ) { - struct domain *d = get_domain_by_id(desc->arch.creator_domid); + struct domain *d = rcu_lock_domain_by_id(desc->arch.creator_domid); if ( d ) { @@ -334,7 +334,7 @@ void destroy_irq(unsigned int irq) "Could not revoke %pd access to IRQ%u (error %d)\n", d, irq, err); - put_domain(d); + rcu_unlock_domain(d); } desc->arch.creator_domid = DOMID_INVALID;
For short-lived references rcu_lock_domain_by_id() is the better (slightly cheaper) alternative. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/arch/x86/mm/mem_sharing.c +++ b/xen/arch/x86/mm/mem_sharing.c @@ -537,7 +537,7 @@ static int audit(void) p2m_type_t t; mfn_t o_mfn; - d = get_domain_by_id(g->domain); + d = rcu_lock_domain_by_id(g->domain); if ( d == NULL ) { gdprintk(XENLOG_ERR, @@ -562,7 +562,7 @@ static int audit(void) d, g->gfn, mfn_x(mfn), p2m_ram_shared, t); errors++; } - put_domain(d); + rcu_unlock_domain(d); nr_gfns++; } /* The type count has an extra ref because we have locked the page */ @@ -1043,10 +1043,10 @@ static int share_pages(struct domain *sd rmap_del(gfn, cpage, 0); rmap_add(gfn, spage); put_count++; - d = get_domain_by_id(gfn->domain); + d = rcu_lock_domain_by_id(gfn->domain); BUG_ON(!d); BUG_ON(set_shared_p2m_entry(d, gfn->gfn, smfn)); - put_domain(d); + rcu_unlock_domain(d); } ASSERT(list_empty(&cpage->sharing->gfns)); BUG_ON(!put_count);
On Tue, Jan 5, 2021 at 8:28 AM Jan Beulich <jbeulich@suse.com> wrote: > > For short-lived references rcu_lock_domain_by_id() is the better > (slightly cheaper) alternative. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Acked-by: Tamas K Lengyel <tamas@tklengyel.com>
© 2016 - 2024 Red Hat, Inc.