From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH v2 1/7] xen: vNUMA support for guests. Date: Thu, 14 Nov 2013 12:18:32 +0100 Message-ID: <1384427912.29902.114.camel@Abyss> References: <1384399569-23969-1-git-send-email-ufimtseva@gmail.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============9141094908564229969==" Return-path: In-Reply-To: <1384399569-23969-1-git-send-email-ufimtseva@gmail.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Elena Ufimtseva Cc: keir@xen.org, stefano.stabellini@eu.citrix.com, george.dunlap@eu.citrix.com, msw@linux.com, lccycc123@gmail.com, xen-devel@lists.xen.org, JBeulich@suse.com List-Id: xen-devel@lists.xenproject.org --===============9141094908564229969== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="=-DeUSreyaODp1WJvL+DiS" --=-DeUSreyaODp1WJvL+DiS Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On mer, 2013-11-13 at 22:26 -0500, Elena Ufimtseva wrote: > Defines interface, structures and hypercalls for guests that wish > to retreive vNUMA topology from Xen. > Well, not only "for guests that wish to retrieve" the vNUMA topology, also for toolstacks that wish to configure it. > Two subop hypercalls introduced by patch: > XEN_DOMCTL_setvnumainfo to define vNUMA domain topology per domain > and XENMEM_get_vnuma_info to retreive that topology by guest. >=20 > Signed-off-by: Elena Ufimtseva > --- > diff --git a/xen/common/domain.c b/xen/common/domain.c > @@ -539,6 +539,7 @@ int domain_kill(struct domain *d) > tmem_destroy(d->tmem); > domain_set_outstanding_pages(d, 0); > d->tmem =3D NULL; > + domain_vnuma_destroy(&d->vnuma); > /* fallthrough */ > case DOMDYING_dying: > rc =3D domain_relinquish_resources(d); > @@ -1297,6 +1298,15 @@ int continue_hypercall_on_cpu( > return 0; > } > =20 > +void domain_vnuma_destroy(struct domain_vnuma_info *v) > +{ I think vnuma_destroy() is ok, as it is tmem_destroy(), evtchn_destory(), etc. > + v->nr_vnodes =3D 0; > + xfree(v->vmemrange); > + xfree(v->vcpu_to_vnode); > + xfree(v->vdistance); > + xfree(v->vnode_numamap); > +} > + > diff --git a/xen/common/domctl.c b/xen/common/domctl.c > @@ -871,6 +872,87 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) = u_domctl) > } > break; > =20 > + case XEN_DOMCTL_setvnumainfo: > + { > + unsigned int i =3D 0, dist_size; > + uint nr_vnodes; > + ret =3D -EFAULT; > + > + /* Already set? */ > + if ( d->vnuma.nr_vnodes > 0 ) > + return 0; > + > + nr_vnodes =3D op->u.vnuma.nr_vnodes; > + =20 > + if ( nr_vnodes =3D=3D 0 ) > + return ret; > + if ( nr_vnodes * nr_vnodes > UINT_MAX ) > + return ret; > + Mmm... I think this three 'return's ought all to be 'break's, or you'll never get to execute the common exit path from do_domctl(). > + /* > + * If null, vnode_numamap will set default to > + * point to allocation mechanism to dont use > + * per physical node allocation or this is for > + * cases when there is no physical NUMA. > + */ > + if ( guest_handle_is_null(op->u.vnuma.vdistance) || > + guest_handle_is_null(op->u.vnuma.vmemrange) || > + guest_handle_is_null(op->u.vnuma.vcpu_to_vnode) ) > + goto err_dom; > + =20 Sorry, I'm not sure I fully understand the comment: you're saying that it is ok for vnuma_nodemap to be NULL, right? > + dist_size =3D nr_vnodes * nr_vnodes; > + =20 > + d->vnuma.vdistance =3D xmalloc_array(unsigned int, dist_size); > + d->vnuma.vmemrange =3D xmalloc_array(vmemrange_t, nr_vnodes); > + d->vnuma.vcpu_to_vnode =3D xmalloc_array(unsigned int, d->max_vc= pus); > + d->vnuma.vnode_numamap =3D xmalloc_array(unsigned int, nr_vnodes= ); > + > + if ( d->vnuma.vdistance =3D=3D NULL || > + d->vnuma.vmemrange =3D=3D NULL || > + d->vnuma.vcpu_to_vnode =3D=3D NULL || > + d->vnuma.vnode_numamap =3D=3D NULL ) > + { > + ret =3D -ENOMEM; > + goto err_dom; > + } > Well, in general, things like just 'err' or 'out' are fine as labels. However, in this case, since we're inside quite a big switch{}, a bit more of context could be helpful. What about killing the '_dom' part (which does not really say much) and putting a meaningful prefix? Also, it's not like the code you're jumping at is executed only on error, so even the 'err_' part looks incorrect. Personally, I'd go for something like 'setvnumainfo_out' (see XEN_DOMCTL_getdomaininfo for reference). > + if ( unlikely(copy_from_guest(d->vnuma.vdistance, > + op->u.vnuma.vdistance, > + dist_size)) ) > + goto err_dom; > + if ( unlikely(copy_from_guest(d->vnuma.vmemrange, > + op->u.vnuma.vmemrange, > + nr_vnodes)) ) > + goto err_dom; > + if ( unlikely(copy_from_guest(d->vnuma.vcpu_to_vnode, > + op->u.vnuma.vcpu_to_vnode, > + d->max_vcpus)) ) > + goto err_dom; > + if ( !guest_handle_is_null(op->u.vnuma.vnode_numamap) ) > + { > + if ( unlikely(copy_from_guest(d->vnuma.vnode_numamap, > + op->u.vnuma.vnode_numamap, > + nr_vnodes)) ) > + goto err_dom; > + } > + else > + for ( i =3D 0; i < nr_vnodes; i++ ) > + d->vnuma.vnode_numamap[i] =3D NUMA_NO_NODE; > + =20 > + /* Everything is good, lets set the number of vnodes */ > + d->vnuma.nr_vnodes =3D nr_vnodes; Put a blank line here. > + ret =3D 0; > +err_dom: > + if ( ret !=3D 0 ) > + { > + d->vnuma.nr_vnodes =3D 0; > + xfree(d->vnuma.vdistance); > + xfree(d->vnuma.vmemrange); > + xfree(d->vnuma.vcpu_to_vnode); > + xfree(d->vnuma.vnode_numamap); > + } > + } > + break; > + > diff --git a/xen/common/memory.c b/xen/common/memory.c > @@ -733,6 +734,41 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDL= E_PARAM(void) arg) > =20 > break; > =20 > + case XENMEM_get_vnuma_info: > + { > + vnuma_topology_info_t mtopology; > + struct domain *d; > + > + rc =3D -EFAULT; > + if ( copy_from_guest(&mtopology, arg, 1) ) > + return -EFAULT; > + if ( (d =3D rcu_lock_domain_by_any_id(mtopology.domid)) =3D=3D N= ULL ) > + return -ESRCH; > + =20 > + if ( (d->vnuma.nr_vnodes =3D=3D 0) || (d->vnuma.nr_vnodes > d->m= ax_vcpus) ) > + return EOPNOTSUPP; > I think you need to rcu_unlock_xxx() here. Also, -EONOTSUPP (note the '-') ? > + =20 > + if ( __copy_to_guest(mtopology.vmemrange, > + d->vnuma.vmemrange, > + d->vnuma.nr_vnodes) !=3D 0 ) > + goto vnumaout; > + if ( __copy_to_guest(mtopology.vdistance, > + d->vnuma.vdistance, > + d->vnuma.nr_vnodes * d->vnuma.nr_vnodes)= !=3D 0 ) > + goto vnumaout; > + if ( __copy_to_guest(mtopology.vcpu_to_vnode, > + d->vnuma.vcpu_to_vnode, > + d->max_vcpus) !=3D 0 ) > + goto vnumaout; > + =20 > + if ( __copy_to_guest(mtopology.nr_vnodes, &d->vnuma.nr_vnodes, 1= ) !=3D 0 ) > + goto vnumaout; > + rc =3D 0; > +vnumaout: > vnumainfo_out ? > + rcu_unlock_domain(d); > + break; > + } > + > default: > rc =3D arch_memory_op(op, arg); > break; > diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h > +/*=20 > + * XEN_DOMCTL_setvnumainfo: sets the vNUMA topology > + * parameters a guest may request. > + */ > +struct xen_domctl_vnuma { > + uint32_t nr_vnodes; > + uint32_t __pad; > + XEN_GUEST_HANDLE_64(uint) vdistance; > + XEN_GUEST_HANDLE_64(uint) vcpu_to_vnode; > + /* domain memory mapping map to physical NUMA nodes */ > + XEN_GUEST_HANDLE_64(uint) vnode_numamap; > + /*=20 > + * memory rages that vNUMA node can represent ^ranges > + * If more than one, its a linked list. > + */ > + XEN_GUEST_HANDLE_64(vmemrange_t) vmemrange; > +}; > diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h > index a057069..bc61bab 100644 > --- a/xen/include/xen/domain.h > +++ b/xen/include/xen/domain.h > @@ -89,4 +89,14 @@ extern unsigned int xen_processor_pmbits; > =20 > extern bool_t opt_dom0_vcpus_pin; > =20 > +struct domain_vnuma_info { > + uint nr_vnodes; > + uint *vdistance; > + uint *vcpu_to_vnode; > + uint *vnode_numamap; > + struct vmemrange *vmemrange; > +}; > + I think you can kill the 'domain_' prefix. It's pretty clear this is a per-domain thing, from the fact that it lives inside struct domain. > +void domain_vnuma_destroy(struct domain_vnuma_info *v); > + Why do you need to declare this function here? Isn't this used only in domain.c ? Regards, Dario --=20 <> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) --=-DeUSreyaODp1WJvL+DiS Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.15 (GNU/Linux) iEYEABECAAYFAlKEsYgACgkQk4XaBE3IOsTICQCgmAAvV+Cbk4Png0z2JVvyM9k5 gp8AoI6BUr69YraIZJw0wHL6nQNwm6Rw =xoyP -----END PGP SIGNATURE----- --=-DeUSreyaODp1WJvL+DiS-- --===============9141094908564229969== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============9141094908564229969==--