Elena Ufimtseva
2013-Sep-13 08:50 UTC
[PATCH RFC v2 5/7] libxl/vNUMA: VM config parsing functions
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
George Dunlap
2013-Sep-19 14:09 UTC
Re: [PATCH RFC v2 5/7] libxl/vNUMA: VM config parsing functions
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
George Dunlap
2013-Sep-19 14:29 UTC
Re: [PATCH RFC v2 5/7] libxl/vNUMA: VM config parsing functions
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
Elena Ufimtseva
2013-Oct-09 18:45 UTC
Re: [PATCH RFC v2 5/7] libxl/vNUMA: VM config parsing functions
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. > > -GeorgeThe 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
Dario Faggioli
2013-Oct-10 08:38 UTC
Re: [PATCH RFC v2 5/7] libxl/vNUMA: VM config parsing functions
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) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Elena Ufimtseva
2013-Oct-10 16:25 UTC
Re: [PATCH RFC v2 5/7] libxl/vNUMA: VM config parsing functions
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
Dario Faggioli
2013-Oct-10 21:32 UTC
Re: [PATCH RFC v2 5/7] libxl/vNUMA: VM config parsing functions
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) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Elena Ufimtseva
2013-Oct-10 22:32 UTC
Re: [PATCH RFC v2 5/7] libxl/vNUMA: VM config parsing functions
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
Ian Jackson
2013-Oct-11 11:18 UTC
Re: [PATCH RFC v2 5/7] libxl/vNUMA: VM config parsing functions
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.
Dario Faggioli
2013-Oct-14 13:15 UTC
Re: [PATCH RFC v2 5/7] libxl/vNUMA: VM config parsing functions
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) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Ian Jackson
2013-Oct-14 13:26 UTC
Re: [PATCH RFC v2 5/7] libxl/vNUMA: VM config parsing functions
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.
Elena Ufimtseva
2013-Oct-14 16:45 UTC
Re: [PATCH RFC v2 5/7] libxl/vNUMA: VM config parsing functions
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
Elena Ufimtseva
2013-Oct-14 16:52 UTC
Re: [PATCH RFC v2 5/7] libxl/vNUMA: VM config parsing functions
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