[PATCH RFC v2 5/7] libxl/vNUMA: VM config parsing functions

[PATCH RFC v2 5/7] libxl/vNUMA: VM config parsing functions

[Elena Ufimtseva]

vNUMA VM config parsing functions.
Parses VM vNUMA related config, verifies and
sets default values for errorneous parameters.
If sum of all vnodes memory sizes does not match
memory, it will be adjusted accordingly;

Required configuration options:
- nr_vnodes - number of vNUMA nodes;
- vnumamem - vnodes memory ranges list;
optional:
- vdistance - array of distances;
default values: 10 for same node, 20 for the rest;
- vcpu_to_vnode - maps vcpu to vnode;
should be specified for each node, otherwise default
interleaved initialization used;

Examples:
a)
vnodes = 2
vnumamem = "512m, 512m"
b)
memory = 2048
vcpus = 6
vnodes = 2
vnumamem = "1g, 1g"
vnuma_distance = "10 20, 10 20"
vcpu_to_vnode ="1, 0, 0, 0, 1, 1"

Signed-off-by: Elena Ufimtseva <ufimtseva@gmail.com>

---
Changes since v1:
* defined default initializers for config parameters;
* added domain memory adjustment in case vNUMA nodes
sizes do not match it;

TODO:
* have maxmem parsed as list and in conjunction with
nr_nodes use as domain initial memory and vNUMA nodes
sizes;
* use standard xl list parsing functions;
---
 tools/libxl/xl_cmdimpl.c |  205 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 205 insertions(+)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 884f050..1520140 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -540,6 +540,143 @@ vcpp_out:
     return rc;
 }
 
+static void vdistance_default(unsigned int *vdistance, unsigned int nr_vnodes)
+{
+    int i, j;
+    for (i = 0; i < nr_vnodes; i++)
+        for (j = 0; j < nr_vnodes; j++)
+            *(vdistance + j * nr_vnodes + i) = i == j ? 10 : 20;
+}
+
+static void vcputovnode_default(unsigned int *vcpu_to_vnode, unsigned int nr_vnodes, unsigned int max_vcpus)
+{
+    int i;
+    if (vcpu_to_vnode == NULL)
+        return;
+    for(i = 0; i < max_vcpus; i++)
+        vcpu_to_vnode[i] = i % nr_vnodes;
+}
+
+static int vdistance_parse(char *vdistcfg, unsigned int *vdistance, unsigned int nr_vnodes)
+{
+    char *endptr, *toka, *tokb, *saveptra = NULL, *saveptrb = NULL;
+    unsigned int *vdist_tmp = NULL;
+    int rc;
+    int i, j, dist, parsed = 0;	
+    rc = -EINVAL;
+
+    if(vdistance == NULL)
+        return rc;
+    vdist_tmp = (unsigned int *)malloc(nr_vnodes * nr_vnodes * sizeof(*vdistance));
+    if (vdist_tmp == NULL)
+        return rc;
+    i =0; j = 0;
+    for (toka = strtok_r(vdistcfg, ",", &saveptra); toka;
+        toka = strtok_r(NULL, ",", &saveptra)) {
+        if ( i >= nr_vnodes ) 
+            goto vdist_parse_err;
+        for (tokb = strtok_r(toka, " ", &saveptrb); tokb;
+            tokb = strtok_r(NULL, " ", &saveptrb)) {
+            if (j >= nr_vnodes) 
+                goto vdist_parse_err;
+            dist = strtol(tokb, &endptr, 10);
+            if (tokb == endptr)
+                goto vdist_parse_err;
+            *(vdist_tmp + j*nr_vnodes + i) = dist;
+            parsed++;
+            j++;
+        }
+        i++;
+        j = 0;
+    }
+    if (parsed == nr_vnodes * nr_vnodes) {
+        memcpy(vdistance, vdist_tmp, nr_vnodes * nr_vnodes * sizeof(*vdistance));
+        rc = 0;
+    }
+vdist_parse_err:
+    if (vdist_tmp !=NULL ) free(vdist_tmp);
+    return rc;
+}
+
+static int vcputovnode_parse(char *cfg, unsigned int *vmap, unsigned int nr_vnodes, unsigned int nr_vcpus)
+{
+    char *toka, *endptr, *saveptra = NULL;
+    unsigned int *vmap_tmp = NULL, nodemap = 0, smask;
+    
+    int rc = 0;
+    int i;
+    rc = -EINVAL;
+    i = 0;
+    smask = ~(~0 << nr_vnodes);
+    if(vmap == NULL)
+        return rc;
+    vmap_tmp = (unsigned int *)malloc(sizeof(*vmap) * nr_vcpus);
+    memset(vmap_tmp, 0, sizeof(*vmap) * nr_vcpus);
+    for (toka = strtok_r(cfg, " ", &saveptra); toka;
+        toka = strtok_r(NULL, " ", &saveptra)) {
+        if (i >= nr_vcpus) goto vmap_parse_out;
+            vmap_tmp[i] = strtoul(toka, &endptr, 10);
+            nodemap |= (1 << vmap_tmp[i]);
+            if( endptr == toka) 
+                goto vmap_parse_out;
+        i++;
+    }
+    memcpy(vmap, vmap_tmp, sizeof(*vmap) * nr_vcpus);
+    if( ((nodemap & smask) + 1) == (1 << nr_vnodes) )
+        rc = i;
+    else 
+        /* Not all nodes have vcpus, will use default map */
+        rc = -EINVAL;
+vmap_parse_out:
+    if (vmap_tmp != NULL) free(vmap_tmp);
+    return rc;
+}
+
+static int vnumamem_parse(char *vmemsizes, uint64_t *vmemregions, int nr_vnodes)
+{
+    uint64_t memsize;
+    char *endptr, *toka, *saveptr = NULL;
+    int rc;
+    int j;
+    
+    rc = -EINVAL;
+    memsize = j = 0;
+    if(vmemregions == NULL)
+        goto vmem_parse_out;
+    for (toka = strtok_r(vmemsizes, ",", &saveptr); toka;
+        toka = strtok_r(NULL, ",", &saveptr)) {
+        if ( j >= nr_vnodes ) 
+            goto vmem_parse_out;
+        memsize = strtoul(toka, &endptr, 10);
+        if (endptr == toka) 
+            goto vmem_parse_out;
+        switch (*endptr) {
+            case 'G':
+            case 'g':
+                memsize = memsize * 1024 * 1024 * 1024;
+                break;
+            case 'M':
+            case 'm':
+                memsize = memsize * 1024 * 1024;
+                break;
+            case 'K':
+            case 'k':
+                memsize = memsize * 1024 ;
+                break;
+            default:
+                continue;
+                break;
+        }
+        if (memsize > 0) {
+            vmemregions[j] = memsize;
+            j++;
+        }
+    }
+    rc = j;
+vmem_parse_out:   
+    return rc;
+}
+
 static void parse_config_data(const char *config_source,
                               const char *config_data,
                               int config_len,
@@ -871,6 +1008,11 @@ static void parse_config_data(const char *config_source,
     {
         char *cmdline = NULL;
         const char *root = NULL, *extra = "";
+        const char *vnumamemcfg = NULL;
+        int nr_vnuma_regions;
+        long unsigned int vnuma_memparsed = 0;
+        const char *vmapcfg  = NULL;
+        const char *vdistcfg = NULL;
 
         xlu_cfg_replace_string (config, "kernel", &b_info->u.pv.kernel, 0);
 
@@ -889,6 +1031,69 @@ static void parse_config_data(const char *config_source,
             exit(1);
         }
 
+        if (!xlu_cfg_get_long (config, "vnodes", &l, 0)) {
+                b_info->nr_vnodes = l;
+                if(b_info->nr_vnodes != 0 && 
+                  !xlu_cfg_get_string (config, "vnumamem", &vnumamemcfg, 0)) 
+                {
+                    b_info->vnuma_memszs = calloc(b_info->nr_vnodes,
+                                                sizeof(*b_info->vnuma_memszs));
+                    if (b_info->vnuma_memszs == NULL) 
+                        exit(1);
+                    char *buf2 = strdup(vnumamemcfg);
+                    nr_vnuma_regions = vnumamem_parse(buf2, b_info->vnuma_memszs,
+                                                            b_info->nr_vnodes);
+                    if(nr_vnuma_regions != b_info->nr_vnodes ) {
+                        fprintf(stderr, "WARNING: Incorrect vNUMA memory config\n");
+                        if(buf2) free(buf2);
+                        exit(1);
+                    }
+                    for(i = 0; i < b_info->nr_vnodes; i++)
+                        vnuma_memparsed = vnuma_memparsed + (b_info->vnuma_memszs[i] >> 10);
+                    if(vnuma_memparsed != b_info->max_memkb)
+                    /* setting up new memory limit */
+                    {
+                        fprintf(stderr, "WARNING: vNUMA memory is not equal to max_mem, adjusting.\n");
+                        b_info->max_memkb = vnuma_memparsed;
+                        b_info->target_memkb = vnuma_memparsed;
+                    }
+                    if (buf2) free(buf2);
+                    b_info->vdistance = (unsigned int *)calloc(b_info->nr_vnodes * b_info->nr_vnodes, 
+                                                            sizeof(*b_info->vdistance));
+                        if (b_info->vdistance == NULL) 
+                           exit(1);
+
+                    if(!xlu_cfg_get_string(config, "vnuma_distance", &vdistcfg, 0)) {
+                                                buf2 = strdup(vdistcfg);
+                        if(vdistance_parse(buf2, b_info->vdistance, b_info->nr_vnodes) < 0)
+                        {
+                            vdistance_default(b_info->vdistance, b_info->nr_vnodes);
+                        } 
+                        if(buf2) free(buf2);
+                    }
+                    else
+                        vdistance_default(b_info->vdistance, b_info->nr_vnodes);
+                    
+                    b_info->vcpu_to_vnode = (unsigned int *)calloc(b_info->max_vcpus,
+                                                                        sizeof(*b_info->vcpu_to_vnode));
+                    if (b_info->vcpu_to_vnode == NULL) 
+                        exit(1);
+                    if(!xlu_cfg_get_string(config, "vcpu_to_vnode", &vmapcfg, 0))
+                    {
+                        buf2 = strdup(vmapcfg);
+                        if (vcputovnode_parse(buf2, b_info->vcpu_to_vnode,
+                                                b_info->nr_vnodes, b_info->max_vcpus) < 0) {
+                            vcputovnode_default(b_info->vcpu_to_vnode, b_info->nr_vnodes, b_info->max_vcpus);
+                        }
+                        if(buf2) free(buf2);
+                    }
+                    else
+                        vcputovnode_default(b_info->vcpu_to_vnode, b_info->nr_vnodes, b_info->max_vcpus);
+                }
+                else 
+                    b_info->nr_vnodes=0;
+        }
+        
         xlu_cfg_replace_string (config, "bootloader", &b_info->u.pv.bootloader, 0);
         switch (xlu_cfg_get_list_as_string_list(config, "bootloader_args",
                                       &b_info->u.pv.bootloader_args, 1))
-- 
1.7.10.4

Re: [PATCH RFC v2 5/7] libxl/vNUMA: VM config parsing functions

[George Dunlap]

On Fri, Sep 13, 2013 at 9:50 AM, Elena Ufimtseva <ufimtseva@gmail.com> wrote:
> vNUMA VM config parsing functions.
> Parses VM vNUMA related config, verifies and
> sets default values for errorneous parameters.
> If sum of all vnodes memory sizes does not match
> memory, it will be adjusted accordingly;

Minor thing: this should be "xl/vnuma", since everything is in xl.

> Required configuration options:
> - nr_vnodes - number of vNUMA nodes;
> - vnumamem - vnodes memory ranges list;
> optional:
> - vdistance - array of distances;
> default values: 10 for same node, 20 for the rest;
> - vcpu_to_vnode - maps vcpu to vnode;
> should be specified for each node, otherwise default
> interleaved initialization used;
>
> Examples:
> a)
> vnodes = 2
> vnumamem = "512m, 512m"
> b)
> memory = 2048
> vcpus = 6
> vnodes = 2
> vnumamem = "1g, 1g"
> vnuma_distance = "10 20, 10 20"
> vcpu_to_vnode ="1, 0, 0, 0, 1, 1"

If you make these lists python lists (i.e., [1, 0, 0, 0, 1, 1]), then
not only will the file format be more consistent, but you should be
able to adapt xlu_cfg_get_list() to do most of the parsing for you.
(You may need to copy xlu_cfg_get_list_as_string_list() as
xlu_cfg_get_list_as_int_list() or something.)

I think probably that when specifying memory sizes, we should follow
suit with the "memory" and "maxmem" parameters, and just always use
megabytes.  That simplifies the parsing too.

Not sure about vnuma_distance -- I suppose to be completely consistent
we'd have to do nested lists, like so: [[10, 20], [20, 10]]  But that
may make parsing a lot more complicated.  Since we're passing a big
array to the domain builder anyway, would it be bad just to make it
one long array, like so?  [10, 20, 20, 10]  Or maybe we can just not
have the parameter for this series and use the default distance array
you have here; then we can add node distance parsing later.

IanC / IanJ -- thoughts?

I'll save a detailed review until we have a better idea what the
interface will be.  The big thing would be adding blank lines to break
the code into "paragraphs".

 -George

Re: [PATCH RFC v2 5/7] libxl/vNUMA: VM config parsing functions

[George Dunlap]

On Fri, Sep 13, 2013 at 9:50 AM, Elena Ufimtseva <ufimtseva@gmail.com> wrote:
> vNUMA VM config parsing functions.
> Parses VM vNUMA related config, verifies and
> sets default values for errorneous parameters.
> If sum of all vnodes memory sizes does not match
> memory, it will be adjusted accordingly;

Also:

* I think if there's a mismatch in the config file we should error
out, not allow one bit of the config to override another part.
* It's probably worth making a default for vnumamem as well, as there
is for vcpu_to_vnode.

 -George

Re: [PATCH RFC v2 5/7] libxl/vNUMA: VM config parsing functions

[Elena Ufimtseva]

On Thu, Sep 19, 2013 at 10:29 AM, George Dunlap
<George.Dunlap@eu.citrix.com> wrote:
> On Fri, Sep 13, 2013 at 9:50 AM, Elena Ufimtseva <ufimtseva@gmail.com> wrote:
>> vNUMA VM config parsing functions.
>> Parses VM vNUMA related config, verifies and
>> sets default values for errorneous parameters.
>> If sum of all vnodes memory sizes does not match
>> memory, it will be adjusted accordingly;
>
> Also:
>
> * I think if there's a mismatch in the config file we should error
> out, not allow one bit of the config to override another part.
> * It's probably worth making a default for vnumamem as well, as there
> is for vcpu_to_vnode.
>
>  -George

The other simplified way is to specify only two values. One for the
distance between same node and another between not the same nodes.
like this:

vdistance = [10, 20]

and this will expand to

10 20 20 20
20 10 20 20
20 20 10 20
20 20 20 10

for 4 nodes.
There is a loss in flexibility of course and other parsing can remain
the same or as George proposed, using lists.

Elena






-- 
Elena

Re: [PATCH RFC v2 5/7] libxl/vNUMA: VM config parsing functions

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1253 bytes --]

On mer, 2013-10-09 at 14:45 -0400, Elena Ufimtseva wrote:
> The other simplified way is to specify only two values. One for the
> distance between same node and another between not the same nodes.
> like this:
> 
> vdistance = [10, 20]
> 
Mmm... I like it...

> and this will expand to
> 
> 10 20 20 20
> 20 10 20 20
> 20 20 10 20
> 20 20 20 10
> 
> for 4 nodes.
>
Nice.

> There is a loss in flexibility of course and other parsing can remain
> the same or as George proposed, using lists.
> 
Well, can't we support both? I mean, even if you configure 2 nodes you'd
need 4 values, right? So, there are no cases where you only specify 2
values.

Well, let's make it like this: if you provide 2 values, it acts as you
say above; otherwise you have to specify all of them. If you should have
specified 4 (or 16) values, and you specify, say, 3 (or, say, 10) all
the unspecified ones will have the same default value (e.g., 10). How do
you like this?

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 #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH RFC v2 5/7] libxl/vNUMA: VM config parsing functions

[Elena Ufimtseva]

On Thu, Oct 10, 2013 at 4:38 AM, Dario Faggioli
<dario.faggioli@citrix.com> wrote:
> On mer, 2013-10-09 at 14:45 -0400, Elena Ufimtseva wrote:
>> The other simplified way is to specify only two values. One for the
>> distance between same node and another between not the same nodes.
>> like this:
>>
>> vdistance = [10, 20]
>>
> Mmm... I like it...
>
>> and this will expand to
>>
>> 10 20 20 20
>> 20 10 20 20
>> 20 20 10 20
>> 20 20 20 10
>>
>> for 4 nodes.
>>
> Nice.
>
>> There is a loss in flexibility of course and other parsing can remain
>> the same or as George proposed, using lists.
>>
> Well, can't we support both? I mean, even if you configure 2 nodes you'd
> need 4 values, right? So, there are no cases where you only specify 2
> values.
>
> Well, let's make it like this: if you provide 2 values, it acts as you
> say above; otherwise you have to specify all of them. If you should have
> specified 4 (or 16) values, and you specify, say, 3 (or, say, 10) all
> the unspecified ones will have the same default value (e.g., 10). How do
> you like this?

Yes, two have both ways is better.

I would even add the third way - if the number of values is power of
number of nodes,
take them as it is and expand by rows (as it is right now).

so we have distances:

1) [10, 20] - only two values => same node 10, othes - 20;

2) did you mean, that if vnodes = 4, vdistance = [10, 10, 10]; the
rest of it should be 10?
Then it means all distances are the same?

3) if vdistance number of elements = vnodes * vnodes, take as it is.

Elena







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



-- 
Elena

Re: [PATCH RFC v2 5/7] libxl/vNUMA: VM config parsing functions

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 2003 bytes --]

On gio, 2013-10-10 at 12:25 -0400, Elena Ufimtseva wrote:
> On Thu, Oct 10, 2013 at 4:38 AM, Dario Faggioli
> > Well, let's make it like this: if you provide 2 values, it acts as you
> > say above; otherwise you have to specify all of them. If you should have
> > specified 4 (or 16) values, and you specify, say, 3 (or, say, 10) all
> > the unspecified ones will have the same default value (e.g., 10). How do
> > you like this?
> 
> Yes, two have both ways is better.
> 
Ok, then.

> I would even add the third way - if the number of values is power of
> number of nodes,
> take them as it is and expand by rows (as it is right now).
> 
Sounds fine, although, I'm not sure I'm getting 100% of what you're
saying. What do you mean with "the number of values is power of number
of nodes"? Do you mind giving an example?

> so we have distances:
> 
> 1) [10, 20] - only two values => same node 10, othes - 20;
> 
Yep.

> 2) did you mean, that if vnodes = 4, vdistance = [10, 10, 10]; the
> rest of it should be 10?
> Then it means all distances are the same?
> 
As said on IRC, I mean that we should have a default value for the
distances, in case the user does not say anything about them.

If it says something, but not all of it, we can try doing something wise
with what he gives us (which is exactly what we're doing above if we
have only 2 values).

However, if it's not immediate to translate what he says into something
that we need, we either exit with error or use as much info as we can,
and fill the rest with a default value (and print a warning).

Personally, I'm fine with both.

> 3) if vdistance number of elements = vnodes * vnodes, take as it is.
> 
Sure.

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 #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH RFC v2 5/7] libxl/vNUMA: VM config parsing functions

[Elena Ufimtseva]

On Thu, Oct 10, 2013 at 5:32 PM, Dario Faggioli
<dario.faggioli@citrix.com> wrote:
> On gio, 2013-10-10 at 12:25 -0400, Elena Ufimtseva wrote:
>> On Thu, Oct 10, 2013 at 4:38 AM, Dario Faggioli
>> > Well, let's make it like this: if you provide 2 values, it acts as you
>> > say above; otherwise you have to specify all of them. If you should have
>> > specified 4 (or 16) values, and you specify, say, 3 (or, say, 10) all
>> > the unspecified ones will have the same default value (e.g., 10). How do
>> > you like this?
>>
>> Yes, two have both ways is better.
>>
> Ok, then.
>
>> I would even add the third way - if the number of values is power of
>> number of nodes,
>> take them as it is and expand by rows (as it is right now).
>>
> Sounds fine, although, I'm not sure I'm getting 100% of what you're
> saying. What do you mean with "the number of values is power of number
> of nodes"? Do you mind giving an example?

Apologies, used wrong words :)
I meant to say, third way is the one as before - user specifies all
values and we use them
as is.

>
>> so we have distances:
>>
>> 1) [10, 20] - only two values => same node 10, othes - 20;
>>
> Yep.
>
>> 2) did you mean, that if vnodes = 4, vdistance = [10, 10, 10]; the
>> rest of it should be 10?
>> Then it means all distances are the same?
>>
> As said on IRC, I mean that we should have a default value for the
> distances, in case the user does not say anything about them.
>
> If it says something, but not all of it, we can try doing something wise
> with what he gives us (which is exactly what we're doing above if we
> have only 2 values).
>
> However, if it's not immediate to translate what he says into something
> that we need, we either exit with error or use as much info as we can,
> and fill the rest with a default value (and print a warning).
>

Ok, let me see if I can come up with something quick :)

> Personally, I'm fine with both.
>
>> 3) if vdistance number of elements = vnodes * vnodes, take as it is.
>>
> Sure.
>
> 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)
>



-- 
Elena

Re: [PATCH RFC v2 5/7] libxl/vNUMA: VM config parsing functions

[Ian Jackson]

Dario Faggioli writes ("Re: [Xen-devel] [PATCH RFC v2 5/7] libxl/vNUMA: VM config parsing functions"):
> On gio, 2013-10-10 at 12:25 -0400, Elena Ufimtseva wrote:
> > 3) if vdistance number of elements = vnodes * vnodes, take as it is.
> 
> Sure.

I have some qualms about this:

Aren't there some constraints that need to be imposed ?  For example,
distance[X,Y]==distance[Y,X] ?  What about the triangle inequality
(distance[A,B] + distance[B,C] >= distance[A,C]) ?

Do we really want/need to allow the specification of any arbitrary
distance matrix (subject perhaps to such constraints) ?

If we do need this I think the nested lists are probably a better
syntax for specifying the whole array.  That would also more easily
allow the user to specify only half of the symmetric matrix, and make
it syntactically clearer what's intended.

Ian.

Re: [PATCH RFC v2 5/7] libxl/vNUMA: VM config parsing functions

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 2793 bytes --]


On ven, 2013-10-11 at 12:18 +0100, Ian Jackson wrote:
> Dario Faggioli writes ("Re: [Xen-devel] [PATCH RFC v2 5/7] libxl/vNUMA: VM config parsing functions"):
> > On gio, 2013-10-10 at 12:25 -0400, Elena Ufimtseva wrote:
> > > 3) if vdistance number of elements = vnodes * vnodes, take as it is.
> > 
> > Sure.
> 
> I have some qualms about this:
> 
> Aren't there some constraints that need to be imposed ?  For example,
> distance[X,Y]==distance[Y,X] ?  What about the triangle inequality
> (distance[A,B] + distance[B,C] >= distance[A,C]) ?
> 
I think Linux does some sanity checking of the distance table (and I
think it disables NUMA if finding out something weird, Elena?). However,
this (what Linux expects/checks for) shouldn't really be the only
criterion, since although Linux is the only current implementation,
there is no reason why this can't be implemented by other OSes.

My point being that we can for sure identify some obviously pathological
cases (after having reached an agreement on what that would mean), and
either exit or print a warning if they show up, but I don't think it's
libxl's job to fully ensure consistency.

> Do we really want/need to allow the specification of any arbitrary
> distance matrix (subject perhaps to such constraints) ?
> 
Personally, yes, I think we should try to stick with what the user asks
us to do. Again, we perhaps can have some checking in place (this is not
an hot path) and print warnings.

We can also consider a symmetric matrix as a sane default and hence
allow the user to specify only half of the distances.

> If we do need this I think the nested lists are probably a better
> syntax for specifying the whole array.  
>
Agreed. I actually wanted to say the same. Would something like this be
ok?

distances = [ [10, 20], [20, 10] ]

Or this:

distances = [ [10, 20, 30, 40],
              [20, 10, 20, 30],
              [30, 20, 10, 20],
              [40, 30, 20, 10] ]

Which, considering the above, could also be specified as this:

distances = [ [10, 20, 30, 40],
                  [10, 20, 30],
                      [10, 20],
                          [10] ]

(I.e., "distances = [ [10, 20, 30, 40], [10, 20, 30], [10, 20], [10] ])


Elena, what do you think?

> That would also more easily
> allow the user to specify only half of the symmetric matrix, and make
> it syntactically clearer what's intended.
> 
Wow, I seem we're on the same pitch here. :-D

Thanks and 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 #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH RFC v2 5/7] libxl/vNUMA: VM config parsing functions

[Ian Jackson]

Dario Faggioli writes ("Re: [Xen-devel] [PATCH RFC v2 5/7] libxl/vNUMA: VM config parsing functions"):
> On ven, 2013-10-11 at 12:18 +0100, Ian Jackson wrote:
> > Aren't there some constraints that need to be imposed ?  For example,
> > distance[X,Y]==distance[Y,X] ?  What about the triangle inequality
> > (distance[A,B] + distance[B,C] >= distance[A,C]) ?
> > 
> I think Linux does some sanity checking of the distance table (and I
> think it disables NUMA if finding out something weird, Elena?). However,
> this (what Linux expects/checks for) shouldn't really be the only
> criterion, since although Linux is the only current implementation,
> there is no reason why this can't be implemented by other OSes.

I wasn't really asking about Linux.  I was talking about NUMA systems
in general.  The thing which prompted me to ask is simply that I want
to be sure that the set of checks we perform has been deliberately
chosen, to reflect the actual potential nature of NUMA systems.

You have chosen to call this parameter "distance".  In mathematics, a
distance would normally have both the symmetry and triangle
inequality, as well as a number of other properties:
    https://en.wikipedia.org/wiki/Metric_space#Definition

What subset of those properties are true of NUMA distances ?  (I
assume that these NUMA distances are estimates to be used by
heuristics, rather than actual measurements of a specific underlying
property.)

> > If we do need this I think the nested lists are probably a better
> > syntax for specifying the whole array.  
> 
> Agreed. I actually wanted to say the same. Would something like this be
> ok?
> 
> distances = [ [10, 20], [20, 10] ]

Right.  I don't think there is anything preventing us implementing
this in the parser, although the existing code probably doesn't
support it.  I can help with the parser.

...
> distances = [ [10, 20, 30, 40],
>                   [10, 20, 30],
>                       [10, 20],
>                           [10] ]
> 
> (I.e., "distances = [ [10, 20, 30, 40], [10, 20, 30], [10, 20], [10] ])

I'd suggest that we should expect the user to specify the lower left
half, rather than the upper right.  That avoids the counterintuitive
offset of the indices in subsequent rows.

(Also I observe that your example violates distance[A,B] == 0.)

Ian.

Re: [PATCH RFC v2 5/7] libxl/vNUMA: VM config parsing functions

[Elena Ufimtseva]

On Mon, Oct 14, 2013 at 9:15 AM, Dario Faggioli
<dario.faggioli@citrix.com> wrote:
>
> On ven, 2013-10-11 at 12:18 +0100, Ian Jackson wrote:
>> Dario Faggioli writes ("Re: [Xen-devel] [PATCH RFC v2 5/7] libxl/vNUMA: VM config parsing functions"):
>> > On gio, 2013-10-10 at 12:25 -0400, Elena Ufimtseva wrote:
>> > > 3) if vdistance number of elements = vnodes * vnodes, take as it is.
>> >
>> > Sure.
>>
>> I have some qualms about this:
>>
>> Aren't there some constraints that need to be imposed ?  For example,
>> distance[X,Y]==distance[Y,X] ?  What about the triangle inequality
>> (distance[A,B] + distance[B,C] >= distance[A,C]) ?
>>
> I think Linux does some sanity checking of the distance table (and I
> think it disables NUMA if finding out something weird, Elena?). However,
> this (what Linux expects/checks for) shouldn't really be the only
> criterion, since although Linux is the only current implementation,
> there is no reason why this can't be implemented by other OSes.
>

Yes, Linux does its sanity checking like the same node distance is
always 10, the
check for symmetrical distance table.
As I understand, all NUMA parameters are specified by vendor and OS can
run its own checks or error fixes.

> My point being that we can for sure identify some obviously pathological
> cases (after having reached an agreement on what that would mean), and
> either exit or print a warning if they show up, but I don't think it's
> libxl's job to fully ensure consistency.
>
>> Do we really want/need to allow the specification of any arbitrary
>> distance matrix (subject perhaps to such constraints) ?
>>
> Personally, yes, I think we should try to stick with what the user asks
> us to do. Again, we perhaps can have some checking in place (this is not
> an hot path) and print warnings.
>
> We can also consider a symmetric matrix as a sane default and hence
> allow the user to specify only half of the distances.
>
>> If we do need this I think the nested lists are probably a better
>> syntax for specifying the whole array.
>>
> Agreed. I actually wanted to say the same. Would something like this be
> ok?
>
> distances = [ [10, 20], [20, 10] ]
>
> Or this:
>
> distances = [ [10, 20, 30, 40],
>               [20, 10, 20, 30],
>               [30, 20, 10, 20],
>               [40, 30, 20, 10] ]
>
> Which, considering the above, could also be specified as this:
>
> distances = [ [10, 20, 30, 40],
>                   [10, 20, 30],
>                       [10, 20],
>                           [10] ]
>
> (I.e., "distances = [ [10, 20, 30, 40], [10, 20, 30], [10, 20], [10] ])
>
>
> Elena, what do you think?

yes, I like it.

>
>> That would also more easily
>> allow the user to specify only half of the symmetric matrix, and make
>> it syntactically clearer what's intended.
>>
> Wow, I seem we're on the same pitch here. :-D
>
> Thanks and 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)



-- 
Elena

Re: [PATCH RFC v2 5/7] libxl/vNUMA: VM config parsing functions

[Elena Ufimtseva]

On Mon, Oct 14, 2013 at 9:26 AM, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> Dario Faggioli writes ("Re: [Xen-devel] [PATCH RFC v2 5/7] libxl/vNUMA: VM config parsing functions"):
>> On ven, 2013-10-11 at 12:18 +0100, Ian Jackson wrote:
>> > Aren't there some constraints that need to be imposed ?  For example,
>> > distance[X,Y]==distance[Y,X] ?  What about the triangle inequality
>> > (distance[A,B] + distance[B,C] >= distance[A,C]) ?
>> >
>> I think Linux does some sanity checking of the distance table (and I
>> think it disables NUMA if finding out something weird, Elena?). However,
>> this (what Linux expects/checks for) shouldn't really be the only
>> criterion, since although Linux is the only current implementation,
>> there is no reason why this can't be implemented by other OSes.
>
> I wasn't really asking about Linux.  I was talking about NUMA systems
> in general.  The thing which prompted me to ask is simply that I want
> to be sure that the set of checks we perform has been deliberately
> chosen, to reflect the actual potential nature of NUMA systems.
>
> You have chosen to call this parameter "distance".  In mathematics, a
> distance would normally have both the symmetry and triangle
> inequality, as well as a number of other properties:
>     https://en.wikipedia.org/wiki/Metric_space#Definition
>
> What subset of those properties are true of NUMA distances ?  (I
> assume that these NUMA distances are estimates to be used by
> heuristics, rather than actual measurements of a specific underlying
> property.)

Ian, I think the distance as you say its not a geometrical distance.
In that case, I am not even sure if that triangle inequality should hold.
I may be wrong.
And NUMA distance is something that vendor defines.

>
>> > If we do need this I think the nested lists are probably a better
>> > syntax for specifying the whole array.
>>
>> Agreed. I actually wanted to say the same. Would something like this be
>> ok?
>>
>> distances = [ [10, 20], [20, 10] ]
>
> Right.  I don't think there is anything preventing us implementing
> this in the parser, although the existing code probably doesn't
> support it.  I can help with the parser.

Sure, that would be great! Id like to work on this too, but the timing may not
permit it untill end of October.

>
> ...
>> distances = [ [10, 20, 30, 40],
>>                   [10, 20, 30],
>>                       [10, 20],
>>                           [10] ]
>>
>> (I.e., "distances = [ [10, 20, 30, 40], [10, 20, 30], [10, 20], [10] ])
>
> I'd suggest that we should expect the user to specify the lower left
> half, rather than the upper right.  That avoids the counterintuitive
> offset of the indices in subsequent rows.
>
> (Also I observe that your example violates distance[A,B] == 0.)
>
> Ian.



-- 
Elena