Re: [Xen-devel] [PATCH v10 7/9] libxc: allocate domain memory for vnuma enabled

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

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.
> 
> Signed-off-by: Elena Ufimtseva <ufimtseva@xxxxxxxxx>
> ---

> @@ -385,6 +395,9 @@ static inline xen_pfn_t xc_dom_p2m_guest(struct 
> xc_dom_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)
>  
(~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 domid)
>  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;
>  
>      rc = 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 = 0; pfn < dom->total_pages; pfn++ )
>              dom->p2m_host[pfn] = 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 == 0 ) {
> +            xc_dom_printf(dom->xch,
> +                         "%s: Cannot construct vNUMA topology with 0 
> vnodes\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] = 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 = 0, node_pfn_base, i;
> +
> +    rc = allocsz = node_pfn_base = n = 0;
> +
> +    for ( n = 0; n < dom->vnodes; n++ )
> +    {
> +        memflags = 0;
> +        if ( dom->vnode_to_pnode[n] != LIBXC_VNUMA_NO_NODE )
> +        {
> +            memflags |= XENMEMF_exact_node(dom->vnode_to_pnode[n]);
> +            memflags |= XENMEMF_exact_node_request;
> +        }
> +        /* memeszs are in megabytes, calc pages from it for this node. */
> +        vnode_pages = (dom->numa_memszs[n] << 20) >> PAGE_SHIFT_X86;
> +        for ( i = 0; i < vnode_pages; i += allocsz )
> +        {
> +            allocsz = vnode_pages - i;
> +            if ( allocsz > 1024*1024 )
> +                allocsz = 1024*1024;
> +
> +            rc = 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_pnode[n], dom->total_pages);
> +                return rc;
> +            }
> +        }
> +        node_pfn_base += i;
>
This looks fine, but I also think we better sanity check things against
total_pages, as the old code was doing.

Regards,
Dario

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

Attachment: signature.asc
Description: This is a digitally signed message part

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel