Re: [xen-devel][vNUMA v2][PATCH 1/8] Config options

[Dulloor <dulloor@xxxxxxxxx>]

On Tue, Aug 3, 2010 at 5:40 AM, Andre Przywara <andre.przywara@xxxxxxx> wrote:
> 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"?
That's right. How about "numa_strategy", "numa_vnodes", and "numa_stripesz" ?

>
>> +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.
Here, stripe_size is in 4K pages, so effectively we have 44 bits,
which should suffice.
But, for consistency, I will change that to 64-bit.

>>
>> +} 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.
I  was thinking that if someone misconfigures (or for typos), we fall back
to the current case. But, it makes sense not to do that. Will change this.

>
>> @@ -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.
OK.

>>
>> +        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
>
>

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel