From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ian Campbell <Ian.Campbell@citrix.com>
Subject: Re: [PATCH v4 15/21] libxc: allocate memory with vNUMA
 information for HVM guest
Date: Wed, 28 Jan 2015 16:36:56 +0000
Message-ID: <1422463016.5187.58.camel@citrix.com>
References: <1422011632-22018-1-git-send-email-wei.liu2@citrix.com>
	<1422011632-22018-16-git-send-email-wei.liu2@citrix.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <1422011632-22018-16-git-send-email-wei.liu2@citrix.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Wei Liu <wei.liu2@citrix.com>
Cc: ufimtseva@gmail.com, andrew.cooper3@citrix.com, dario.faggioli@citrix.com, ian.jackson@eu.citrix.com, xen-devel@lists.xen.org, JBeulich@suse.com
List-Id: xen-devel@lists.xenproject.org

On Fri, 2015-01-23 at 11:13 +0000, Wei Liu wrote:
> The algorithm is more or less the same as the one used for PV guest.

Any reason the code can't be shared then? :-D
>  
> +    if ( nr_pages > target_pages )
> +        memflags |= XENMEMF_populate_on_demand;

OOI how does vNUMA and PoD interact? Do you prefer to fill a node as
much as possible (leaving other nodes entirely PoD) or do you prefer to
balance the PoD pages between nodes?

I think the former?

I suspect this depends a lot on the guest behaviour, so there probably
isn't a right answer. Are there corner cases where this might go wrong
though?

> +
> +    if ( args->nr_vnuma_info == 0 )
> +    {
> +        /* Build dummy vnode information */
> +        dummy_vnuma_info.vnode = 0;
> +        dummy_vnuma_info.pnode = XC_VNUMA_NO_NODE;
> +        dummy_vnuma_info.pages = args->mem_size >> PAGE_SHIFT;
> +        args->nr_vnuma_info = 1;
> +        args->vnuma_info = &dummy_vnuma_info;

You could have done this in the PV case too, I nearly suggested it then
but realised I already had, so I figured there was a reason not to?

> +    }
> +    else
> +    {
> +        if ( nr_pages > target_pages )
> +        {
> +            PERROR("Cannot enable vNUMA and PoD at the same time");

And there's the answer to my question above ;-)

> +            goto error_out;
>  
> +    for ( i = 0; i < args->nr_vnuma_info; i++ )
>      {

Reindenting in a precursor patch made this diff a lot easier to read,
thanks.

>              if ( count > max_pages )
> @@ -388,19 +440,20 @@ static int setup_guest(xc_interface *xch,
>                  unsigned long nr_extents = count >> SUPERPAGE_1GB_SHIFT;
>                  xen_pfn_t sp_extents[nr_extents];
>  
> -                for ( i = 0; i < nr_extents; i++ )
> -                    sp_extents[i] =
> -                        page_array[cur_pages+(i<<SUPERPAGE_1GB_SHIFT)];
> +                for ( j = 0; j < nr_extents; j++ )
> +                    sp_extents[j] =
> +                        page_array[cur_pages+(j<<SUPERPAGE_1GB_SHIFT)];

You might condider s/i/j/ for a precursor patch too. Or given the scope
of the outermost loop is quite large a more specific name than i (like
vnid) might be more appropriate.

Ian.