Re: [PATCH v3 03/19] libxc: allocate memory with vNUMA information for PV guest

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 13/01/15 12:11, Wei Liu wrote:
> From libxc's point of view, it only needs to know vnode to pnode mapping
> and size of each vnode to allocate memory accordingly. Add these fields
> to xc_dom structure.
>
> The caller might not pass in vNUMA information. In that case, a dummy
> layout is generated for the convenience of libxc's allocation code. The
> upper layer (libxl etc) still sees the domain has no vNUMA
> configuration.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Dario Faggioli <dario.faggioli@citrix.com>
> Cc: Elena Ufimtseva <ufimtseva@gmail.com>
> ---
> Changes in v3:
> 1. Rewrite commit log.
> 2. Shorten some error messages.
> ---
>  tools/libxc/include/xc_dom.h |    5 +++
>  tools/libxc/xc_dom_x86.c     |   79 ++++++++++++++++++++++++++++++++++++------
>  tools/libxc/xc_private.h     |    2 ++
>  3 files changed, 75 insertions(+), 11 deletions(-)
>
> diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
> index 07d7224..c459e77 100644
> --- a/tools/libxc/include/xc_dom.h
> +++ b/tools/libxc/include/xc_dom.h
> @@ -167,6 +167,11 @@ struct xc_dom_image {
>      struct xc_dom_loader *kernel_loader;
>      void *private_loader;
>  
> +    /* vNUMA information */
> +    unsigned int *vnode_to_pnode; /* vnode to pnode mapping array */
> +    uint64_t *vnode_size;         /* vnode size array */

Please make it very clear in the comment here that "size" is in MB (at
least I presume so, given the shifts by 20).  There are currently no
specified units.

> +    unsigned int nr_vnodes;       /* number of elements of above arrays */
> +
>      /* kernel loader */
>      struct xc_dom_arch *arch_hooks;
>      /* allocate up to virt_alloc_end */
> diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
> index bf06fe4..06a7e54 100644
> --- a/tools/libxc/xc_dom_x86.c
> +++ b/tools/libxc/xc_dom_x86.c
> @@ -759,7 +759,8 @@ 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, allocsz, mfn, total, pfn_base;
> +    int i, j;
>  
>      rc = x86_compat(dom->xch, dom->guest_domid, dom->guest_type);
>      if ( rc )
> @@ -811,18 +812,74 @@ 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;
> -        
> +
> +        /* Setup dummy vNUMA information if it's not provided. Not
> +         * that this is a valid state if libxl doesn't provide any
> +         * vNUMA information.
> +         *
> +         * In this case we setup some dummy value for the convenience
> +         * of the allocation code. Note that from the user's PoV the
> +         * guest still has no vNUMA configuration.
> +         */
> +        if ( dom->nr_vnodes == 0 )
> +        {
> +            dom->nr_vnodes = 1;
> +            dom->vnode_to_pnode = xc_dom_malloc(dom,
> +                                                sizeof(*dom->vnode_to_pnode));
> +            dom->vnode_to_pnode[0] = XC_VNUMA_NO_NODE;
> +            dom->vnode_size = xc_dom_malloc(dom, sizeof(*dom->vnode_size));
> +            dom->vnode_size[0] = (dom->total_pages << PAGE_SHIFT) >> 20;
> +        }
> +
> +        total = 0;
> +        for ( i = 0; i < dom->nr_vnodes; i++ )
> +            total += ((dom->vnode_size[i] << 20) >> PAGE_SHIFT);

Can I suggest a "mb_to_pages()" helper rather than opencoding this in
several locations.

> +        if ( total != dom->total_pages )
> +        {
> +            xc_dom_panic(dom->xch, XC_INTERNAL_ERROR,
> +                         "%s: vNUMA page count mismatch (0x%"PRIpfn" != 0x%"PRIpfn")\n",
> +                         __FUNCTION__, total, dom->total_pages);

__func__ please.  It is part of C99 unlike __FUNCTION__ which is a gnuism.

andrewcoop:xen.git$ git grep  __FUNCTION__ | wc -l
230
andrewcoop:xen.git$ git grep  __func__ | wc -l
194

Looks like the codebase is very mixed, but best to err on the side of
the standard.

> +            return -EINVAL;
> +        }
> +
>          /* allocate guest memory */
> -        for ( i = rc = allocsz = 0;
> -              (i < dom->total_pages) && !rc;
> -              i += allocsz )
> +        pfn_base = 0;
> +        for ( i = 0; i < dom->nr_vnodes; i++ )
>          {
> -            allocsz = dom->total_pages - i;
> -            if ( allocsz > 1024*1024 )
> -                allocsz = 1024*1024;
> -            rc = xc_domain_populate_physmap_exact(
> -                dom->xch, dom->guest_domid, allocsz,
> -                0, 0, &dom->p2m_host[i]);
> +            unsigned int memflags;
> +            uint64_t pages;
> +
> +            memflags = 0;
> +            if ( dom->vnode_to_pnode[i] != XC_VNUMA_NO_NODE )
> +            {
> +                memflags |= XENMEMF_exact_node(dom->vnode_to_pnode[i]);
> +                memflags |= XENMEMF_exact_node_request;
> +            }
> +
> +            pages = (dom->vnode_size[i] << 20) >> PAGE_SHIFT;
> +
> +            for ( j = 0; j < pages; j += allocsz )
> +            {
> +                allocsz = pages - j;
> +                if ( allocsz > 1024*1024 )
> +                    allocsz = 1024*1024;
> +
> +                rc = xc_domain_populate_physmap_exact(dom->xch,
> +                         dom->guest_domid, allocsz, 0, memflags,
> +                         &dom->p2m_host[pfn_base+j]);
> +
> +                if ( rc )
> +                {
> +                    if ( dom->vnode_to_pnode[i] != XC_VNUMA_NO_NODE )
> +                        xc_dom_panic(dom->xch, XC_INTERNAL_ERROR,
> +                                     "%s: fail to allocate 0x%"PRIx64"/0x%"PRIpfn" pages (v=%d, p=%d)\n",
> +                                     __FUNCTION__, pages, dom->total_pages, i,
> +                                     dom->vnode_to_pnode[i]);

"failed to allocate"

I am not sure the total number of pages is useful here, especially as
you don't know how many pages have successfully been allocated first.

Finally, please also print an error for the non-vnuma case.  Failing to
populate memory is not something which should go without an error message.

~Andrew