From mboxrd@z Thu Jan  1 00:00:00 1970
From: Andre Przywara <andre.przywara@amd.com>
Subject: Re: [vNUMA v2][PATCH 1/8] Config options
Date: Tue, 3 Aug 2010 14:40:09 +0200
Message-ID: <4C580E29.1090302@amd.com>
References: <1BEA8649F0C00540AB2811D7922ECB6C9338B4CB@orsmsx507.amr.corp.intel.com>	<AANLkTinWRm56hJPBX1eyBhQe1eRaWN-XGu2LQW6NEeLz@mail.gmail.com>
	<AANLkTimHTLjhWCo-=VvcfETRXyX3Sa9W=B4u=hc-LjrV@mail.gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="windows-1252"; format=flowed
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xensource.com>
In-Reply-To: <AANLkTimHTLjhWCo-=VvcfETRXyX3Sa9W=B4u=hc-LjrV@mail.gmail.com>
List-Unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xensource.com>
List-Help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-Subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
Sender: xen-devel-bounces@lists.xensource.com
Errors-To: xen-devel-bounces@lists.xensource.com
To: Dulloor <dulloor@gmail.com>
Cc: "xen-devel@lists.xensource.com" <xen-devel@lists.xensource.com>
List-Id: xen-devel@lists.xenproject.org

Dulloor wrote:
> Implement the following config options :
> 
> strategy = "str", where str is confine/stripe/split/auto
> vnodes = <num-nodes>
> stripesz = <size-in-pages>
As Ian already said, I'd also prefer NUMA related names instead of
the generic "strategy". Also "vnodes" may be a bit misleading, what about
"guestnodes"?

> +typedef struct xc_domain_numa_config
> +{
> +    uint32_t strategy;      /* By default, DONTCARE (for now) */
> +    uint32_t nr_nodes;      /* For SPLIT/STRIPE */
> +    uint32_t stripe_size;   /* For STRIPE only */
Are 32 bit here sufficient? Although for the stripe size probably 4GB
are more than needed, I'd prefer to use 64bit (or long) for each
memory-related variable.
> +} xc_domain_numa_config_t;
> +

> +++ b/tools/libxl/xl_cmdimpl.c
> +static uint32_t numa_str_to_val(const char *str)
> +{
> +    if (!strcasecmp(str, "AUTO"))
> +        return XC_DOM_NUMA_AUTO;
> +    if (!strcasecmp(str, "CONFINE"))
> +        return XC_DOM_NUMA_CONFINE;
> +    if (!strcasecmp(str, "SPLIT"))
> +        return XC_DOM_NUMA_SPLIT;
> +    if (!strcasecmp(str, "STRIPE"))
> +        return XC_DOM_NUMA_STRIPE;
> +
> +    return XC_DOM_NUMA_NONE;
Shouldn't the function return something like "unknown" here?
This would allow to detect typos in the config file.

> @@ -650,6 +686,14 @@ static void parse_config_data(const char
>      if (!xlu_cfg_get_long (config, "videoram", &l))
>          b_info->video_memkb = l * 1024;
>  
> +    if (!xlu_cfg_get_string (config, "strategy", &buf)) {
> +        b_info->numa_config.strategy = numa_str_to_val(buf);
Here one chould check the returned value for "unknown" to detect
illegal strategy types.
> +        if (!xlu_cfg_get_long (config, "vnodes", &l))
> +            b_info->numa_config.nr_nodes = l;
> +        if (!xlu_cfg_get_long (config, "stripesz", &l))
> +            b_info->numa_config.stripe_size = l;
> +    }
> +
>      if (!xlu_cfg_get_string (config, "kernel", &buf))
>          b_info->kernel.path = strdup(buf);
>  

Regards,
Andre.

-- 
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
Tel: +49 351 448-3567-12