From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH v10 7/9] libxc: allocate domain memory for vnuma enabled Date: Fri, 12 Sep 2014 13:06:32 +0200 Message-ID: <1410519992.3531.35.camel@Solace.lan> References: <1409718258-3276-1-git-send-email-ufimtseva@gmail.com> <1409718258-3276-5-git-send-email-ufimtseva@gmail.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============4941801034352499437==" Return-path: In-Reply-To: <1409718258-3276-5-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, Ian.Campbell@citrix.com, stefano.stabellini@eu.citrix.com, george.dunlap@eu.citrix.com, msw@linux.com, lccycc123@gmail.com, ian.jackson@eu.citrix.com, xen-devel@lists.xen.org, JBeulich@suse.com, Wei Liu List-Id: xen-devel@lists.xenproject.org --===============4941801034352499437== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="=-YsaKsZWNmycvYswI2W7y" --=-YsaKsZWNmycvYswI2W7y Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On mer, 2014-09-03 at 00:24 -0400, Elena Ufimtseva wrote: > vNUMA-aware domain memory allocation based on provided > vnode to pnode map. If this map is not defined, use > default allocation. Default allocation will not specify > any physical node when allocating memory. > Domain creation will fail if at least one node was not defined. >=20 > Signed-off-by: Elena Ufimtseva > --- > @@ -385,6 +395,9 @@ static inline xen_pfn_t xc_dom_p2m_guest(struct xc_do= m_image *dom, > int arch_setup_meminit(struct xc_dom_image *dom); > int arch_setup_bootearly(struct xc_dom_image *dom); > int arch_setup_bootlate(struct xc_dom_image *dom); > +int arch_boot_alloc(struct xc_dom_image *dom); > + > +#define LIBXC_VNUMA_NO_NODE ~((unsigned int)0) > =20 (~0U) should be fine > --- a/tools/libxc/xc_dom_x86.c > +++ b/tools/libxc/xc_dom_x86.c > @@ -759,7 +759,7 @@ static int x86_shadow(xc_interface *xch, domid_t domi= d) > int arch_setup_meminit(struct xc_dom_image *dom) > { > int rc; > - xen_pfn_t pfn, allocsz, i, j, mfn; > + xen_pfn_t pfn, i, j, mfn; > =20 > rc =3D x86_compat(dom->xch, dom->guest_domid, dom->guest_type); > if ( rc ) > @@ -811,25 +811,77 @@ int arch_setup_meminit(struct xc_dom_image *dom) > /* setup initial p2m */ > for ( pfn =3D 0; pfn < dom->total_pages; pfn++ ) > dom->p2m_host[pfn] =3D pfn; > + > + /* > + * Any PV domain should have at least one vNUMA node. > + * If no config was defined, one default vNUMA node > + * will be set. > + */ > + if ( dom->vnodes =3D=3D 0 ) { > + xc_dom_printf(dom->xch, > + "%s: Cannot construct vNUMA topology with 0 vno= des\n", > + __FUNCTION__); > + return -EINVAL; > And here, at least in theory, the opposite of what we've been saying for libxl holds, so you should be setting errno, and returning -1. However, that is not consistently applied throughout all of libxc, so have a look at what the convention is for this file/function, as well as for the functions called by this function, and follow it. > +/* > + * Allocates domain memory taking into account > + * defined vnuma topology and vnode_to_pnode map. > + * Any pv guest will have at least one vnuma node > + * with vnuma_memszs[0] =3D domain memory and the rest > + * topology initialized with default values. > Maybe mention here where/who takes care of that, at the libxc level. > + */ > +int arch_boot_alloc(struct xc_dom_image *dom) > +{ > + int rc; > + unsigned int n, memflags; > + unsigned long long vnode_pages; > + unsigned long long allocsz =3D 0, node_pfn_base, i; > + > + rc =3D allocsz =3D node_pfn_base =3D n =3D 0; > + > + for ( n =3D 0; n < dom->vnodes; n++ ) > + { > + memflags =3D 0; > + if ( dom->vnode_to_pnode[n] !=3D LIBXC_VNUMA_NO_NODE ) > + { > + memflags |=3D XENMEMF_exact_node(dom->vnode_to_pnode[n]); > + memflags |=3D XENMEMF_exact_node_request; > + } > + /* memeszs are in megabytes, calc pages from it for this node. *= / > + vnode_pages =3D (dom->numa_memszs[n] << 20) >> PAGE_SHIFT_X86; > + for ( i =3D 0; i < vnode_pages; i +=3D allocsz ) > + { > + allocsz =3D vnode_pages - i; > + if ( allocsz > 1024*1024 ) > + allocsz =3D 1024*1024; > + > + rc =3D xc_domain_populate_physmap_exact(dom->xch, dom->guest= _domid, > + allocsz, 0, memflags, > + &dom->p2m_host[node_pfn_base= + i]); > + if ( rc ) > + { > + xc_dom_panic(dom->xch, XC_INTERNAL_ERROR, > + "%s: Failed allocation of %Lu pages for vnode %d= on pnode %d out of %lu\n", > + __FUNCTION__, vnode_pages, n, dom->vnode_to_pnod= e[n], dom->total_pages); > + return rc; > + } > + } > + node_pfn_base +=3D i; > This looks fine, but I also think we better sanity check things against total_pages, as the old code was doing. Regards, Dario --=20 <> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) --=-YsaKsZWNmycvYswI2W7y 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 v2 iEYEABECAAYFAlQS07gACgkQk4XaBE3IOsSsLQCdG52PqytOG/pivAMbyYrSrCDN ZvwAn3BF0T2TwvCAWWVi0929SW1/GkDA =iihC -----END PGP SIGNATURE----- --=-YsaKsZWNmycvYswI2W7y-- --===============4941801034352499437== 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 --===============4941801034352499437==--