Juergen Gross
2010-Nov-26 07:10 UTC
[Xen-devel] [PATCH 0 of 4] support of NUMA topology in xl
This patch series adds support of NUMA topology in xl. Patch 1 adds a libxl function to retireve topology information from hypervisor Patch 2 adds support of -n option to xl info Patch 3 adds possibility to specify complete nodes instead of cpus for cpupools Patch 4 adds a new xl command cpupool-numa-split to create one cpupool per numa node 9 files changed, 430 insertions(+), 19 deletions(-) tools/libxl/libxl.c | 58 ++++++ tools/libxl/libxl.h | 8 tools/libxl/libxl.idl | 7 tools/libxl/libxl_utils.c | 24 ++ tools/libxl/libxl_utils.h | 2 tools/libxl/xl.h | 1 tools/libxl/xl_cmdimpl.c | 314 +++++++++++++++++++++++++++++++++++-- tools/libxl/xl_cmdtable.c | 11 - tools/python/xen/lowlevel/xl/xl.c | 24 ++ _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Juergen Gross
2010-Nov-26 07:10 UTC
[Xen-devel] [PATCH 1 of 4] Support getting topology info in libxl
Added new function libxl_get_topologyinfo() to obtain this information from hypervisor. Signed-off-by: juergen.gross@ts.fujitsu.com 6 files changed, 123 insertions(+) tools/libxl/libxl.c | 58 +++++++++++++++++++++++++++++++++++++ tools/libxl/libxl.h | 8 +++++ tools/libxl/libxl.idl | 7 ++++ tools/libxl/libxl_utils.c | 24 +++++++++++++++ tools/libxl/libxl_utils.h | 2 + tools/python/xen/lowlevel/xl/xl.c | 24 +++++++++++++++ _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Juergen Gross
2010-Nov-26 07:10 UTC
[Xen-devel] [PATCH 2 of 4] support topolgy info in xl info
Adds option -n/--numa to xl info command to print topology information. No numa information up to now, as I''ve no machine which will give this info via xm info (could be a bug in xm, however). Signed-off-by: juergen.gross@ts.fujitsu.com 2 files changed, 50 insertions(+), 11 deletions(-) tools/libxl/xl_cmdimpl.c | 59 +++++++++++++++++++++++++++++++++++++-------- tools/libxl/xl_cmdtable.c | 2 - _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Juergen Gross
2010-Nov-26 07:10 UTC
[Xen-devel] [PATCH 3 of 4] Extend cpupools to support numa
The user interfaces for cpupools are extended to support numa machines: - xl cpupool-create supports now specifying a node list instead of a cpu list. The new cpupool will be created with all free cpus of the specified numa nodes. - xl cpupool-cpu-remove and xl cpupool-cpu-add can take a node number instead of a cpu number. Using ''node:1'' for the cpu parameter will, depending on the operation, either remove all cpus of node 1 in the specified cpupool, or add all free cpus of node 1 to the cpupool. Signed-off-by: juergen.gross@ts.fujitsu.com 2 files changed, 132 insertions(+), 8 deletions(-) tools/libxl/xl_cmdimpl.c | 136 +++++++++++++++++++++++++++++++++++++++++++-- tools/libxl/xl_cmdtable.c | 4 - _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Juergen Gross
2010-Nov-26 07:10 UTC
[Xen-devel] [PATCH 4 of 4] Support new xl command cpupool-numa-split
New xl command cpupool-numa-split which will create one cpupool for each numa node of the machine. Can be called only if no other cpupools than Pool-0 are defined. After creation the cpupools can be managed as usual. Signed-off-by: juergen.gross@ts.fujitsu.com 3 files changed, 125 insertions(+) tools/libxl/xl.h | 1 tools/libxl/xl_cmdimpl.c | 119 +++++++++++++++++++++++++++++++++++++++++++++ tools/libxl/xl_cmdtable.c | 5 + _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Jackson
2010-Dec-07 17:21 UTC
Re: [Xen-devel] [PATCH 1 of 4] Support getting topology info in libxl
Juergen Gross writes ("[Xen-devel] [PATCH 1 of 4] Support getting topology
info in libxl"):> Added new function libxl_get_topologyinfo() to obtain this information from
> hypervisor.
This looks plausible to me. I''d like an ack from Ian Campbell on the
IDL changes. Ian ?
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Ian Jackson
2010-Dec-07 17:23 UTC
Re: [Xen-devel] [PATCH 2 of 4] support topolgy info in xl info
Juergen Gross writes ("[Xen-devel] [PATCH 2 of 4] support topolgy info in
xl info"):> Adds option -n/--numa to xl info command to print topology information.
> No numa information up to now, as I''ve no machine which will give
this info
> via xm info (could be a bug in xm, however).
Just a quick question: the reason for requring the user to specify an
extra option to get this info is that:
* it may be very voluminous
or
* compatibility
?
It seems to me that printing additional information in xl info should
be doable in a way that won''t break old parsers. This isn''t
helped of
course by the fact that we don''t have a definition of how that parsing
is supposed to be done.
What do you think ?
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Ian Jackson
2010-Dec-07 17:25 UTC
Re: [Xen-devel] [PATCH 3 of 4] Extend cpupools to support numa
Juergen Gross writes ("[Xen-devel] [PATCH 3 of 4] Extend cpupools to
support numa"):> The user interfaces for cpupools are extended to support numa machines:
> - xl cpupool-create supports now specifying a node list instead of a cpu
list.
> The new cpupool will be created with all free cpus of the specified numa
> nodes.
> - xl cpupool-cpu-remove and xl cpupool-cpu-add can take a node number
instead
> of a cpu number. Using ''node:1'' for the cpu parameter
will, depending on
> the operation, either remove all cpus of node 1 in the specified cpupool,
> or add all free cpus of node 1 to the cpupool.
>
> Signed-off-by: juergen.gross@ts.fujitsu.com
This looks plausible to me. I''ll apply it when the previous two go in.
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Ian Jackson
2010-Dec-07 17:26 UTC
Re: [Xen-devel] [PATCH 4 of 4] Support new xl command cpupool-numa-split
Juergen Gross writes ("[Xen-devel] [PATCH 4 of 4] Support new xl command
cpupool-numa-split"):> New xl command cpupool-numa-split which will create one cpupool for each
> numa node of the machine. Can be called only if no other cpupools than
Pool-0
> are defined. After creation the cpupools can be managed as usual.
Interesting. Is this a usual use case ? I don''t object to the
feature.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
George Dunlap
2010-Dec-07 17:31 UTC
Re: [Xen-devel] [PATCH 4 of 4] Support new xl command cpupool-numa-split
On Tue, Dec 7, 2010 at 5:26 PM, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:> Juergen Gross writes ("[Xen-devel] [PATCH 4 of 4] Support new xl command cpupool-numa-split"): >> New xl command cpupool-numa-split which will create one cpupool for each >> numa node of the machine. Can be called only if no other cpupools than Pool-0 >> are defined. After creation the cpupools can be managed as usual. > > Interesting. Is this a usual use case ? I don''t object to the feature.it''s the default configuration you get if you don''t manually muck about with cpupools. Sounds like a pretty useful shortcut, rather than having to parse node info manually. (Haven''t looked at the code yet.) -George _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Jackson
2010-Dec-07 17:34 UTC
Re: [Xen-devel] [PATCH 4 of 4] Support new xl command cpupool-numa-split
George Dunlap writes ("Re: [Xen-devel] [PATCH 4 of 4] Support new xl
command cpupool-numa-split"):> it''s the default configuration you get if you don''t
manually muck
> about with cpupools. Sounds like a pretty useful shortcut, rather
> than having to parse node info manually. (Haven''t looked at the
code
> yet.)
Ah that makes perfect sense, thanks.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Juergen Gross
2010-Dec-08 05:47 UTC
Re: [Xen-devel] [PATCH 2 of 4] support topolgy info in xl info
On 12/07/10 18:23, Ian Jackson wrote:> Juergen Gross writes ("[Xen-devel] [PATCH 2 of 4] support topolgy info in xl info"): >> Adds option -n/--numa to xl info command to print topology information. >> No numa information up to now, as I''ve no machine which will give this info >> via xm info (could be a bug in xm, however). > > Just a quick question: the reason for requring the user to specify an > extra option to get this info is that: > * it may be very voluminous > or > * compatibility > ?compatibility with xm Juergen -- Juergen Gross Principal Developer Operating Systems TSP ES&S SWE OS6 Telephone: +49 (0) 89 3222 2967 Fujitsu Technology Solutions e-mail: juergen.gross@ts.fujitsu.com Domagkstr. 28 Internet: ts.fujitsu.com D-80807 Muenchen Company details: ts.fujitsu.com/imprint.html _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2010-Dec-08 10:51 UTC
Re: [Xen-devel] [PATCH 1 of 4] Support getting topology info in libxl
On Fri, 2010-11-26 at 07:10 +0000, Juergen Gross wrote:> diff -r 79b71c77907b -r 37fdfe90e0c2 tools/libxl/libxl.h > --- a/tools/libxl/libxl.h Wed Nov 24 10:20:03 2010 +0000 > +++ b/tools/libxl/libxl.h Thu Nov 25 09:29:43 2010 +0100 > @@ -148,6 +148,13 @@ > uint8_t *map; > } libxl_cpumap; > void libxl_cpumap_destroy(libxl_cpumap *map); > + > +typedef struct { > + uint32_t entries; > + uint32_t *array; > +} libxl_cpuarray; > +#define LIBXL_CPUARRAY_INVENTRY ~0This looked at first glance like you had misspelled INVENTORY. Perhaps LIBXL_CPUARRAY_INVALID_ENTRY?> +void libxl_cpuarray_destroy(libxl_cpuarray *array); > > typedef enum { > XENFV = 1, > @@ -464,6 +471,7 @@ > int libxl_button_press(libxl_ctx *ctx, uint32_t domid, libxl_button button); > > int libxl_get_physinfo(libxl_ctx *ctx, libxl_physinfo *physinfo); > +libxl_topologyinfo *libxl_get_topologyinfo(libxl_ctx *ctx);The idiom in libxl is for such functions to take a pointer to the structure to fill in and return a status code, e.g. int libxl_get_topologyinfo(libxl_ctx *ctx, libxl_topologyinfo *info) e..g libxl_get_physinfo() This is useful since the caller doesn''t always need to manage the dynamic allocation of the info structure e.g, it can use a stack variable for temporary stuff or nest inside another structure etc. Both the error path of libxl_get_topologyinfo and output_topologyinfo() from patch 2/4 leak the info structure which is an error which goes away with this change. The IDL stuff and the python bits look fine to me. Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2010-Dec-08 10:58 UTC
Re: [Xen-devel] [PATCH 3 of 4] Extend cpupools to support numa
See previous comments about the interface to libxl_get_topoplogy and leaking the returned pointer value. On Fri, 2010-11-26 at 07:10 +0000, Juergen Gross wrote:> + if (libxl_get_freecpus(&ctx, &freemap)) { > + fprintf(stderr, "libxl_get_freecpus failed\n"); > + return -ERROR_FAIL; > + } > + > + topology = libxl_get_topologyinfo(&ctx); > + if (topology == NULL) { > + fprintf(stderr, "libxl_get_topologyinfo failed\n"); > + return -ERROR_FAIL; > + } > + > + n = 0; > + for (cpu = 0; cpu < topology->nodemap.entries; cpu++) { > + if (libxl_cpumap_test(&freemap, cpu) && > + (topology->nodemap.array[cpu] == node) && > + !libxl_cpupool_cpuadd(&ctx, poolid, cpu)) { > + n++; > + } > + }This sequence of actions look like they would make a useful addition to the libxl interface as a helper function. Not sure what it would be called, perhaps: libxl_cpupool_cpuadd_node(&ctx, poolid, node) ?> + for (p = 0; p < n_pools; p++) { > + if (poolinfo[p].poolid == poolid) { > + for (cpu = 0; cpu < topology->nodemap.entries; cpu++) { > + if ((topology->nodemap.array[cpu] == node) && > + libxl_cpumap_test(&poolinfo[p].cpumap, cpu) && > + !libxl_cpupool_cpuremove(&ctx, poolid, cpu)) { > + n++; > + } > + } > + } > + }also a helper function? libxl_cpupool_cpuremove_node(&ctx, poolid, node) Isn''t there an existing function to find a poolinfo from a poolid? (if not then should there be?) Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2010-Dec-08 11:16 UTC
Re: [Xen-devel] [PATCH 4 of 4] Support new xl command cpupool-numa-split
On Fri, 2010-11-26 at 07:10 +0000, Juergen Gross wrote:> + > + if (libxl_cpumap_alloc(&ctx, &cpumap)) { > + fprintf(stderr, "Failed to allocate cpumap\n"); > + return -ERROR_FAIL; > + } > + > + poolinfo = libxl_list_cpupool(&ctx, &n_pools); > + if (!poolinfo) { > + fprintf(stderr, "error getting cpupool info\n"); > + return -ERROR_NOMEM; > + } > + poolid = poolinfo[0].poolid; > + schedid = poolinfo[0].sched_id; > + libxl_for_each_cpu(c, poolinfo[0].cpumap) > + if (libxl_cpumap_test(&poolinfo[0].cpumap, c)) > + libxl_cpumap_set(&cpumap, c);Can the libxl_cpumap_alloc and this loop be deferred until after the check and bail for n_pools > 1? There seems to be no way to find out the number of pools without also getting all the info about them, which is a shame.> + /* Reset Pool-0 to 1st node */ > + node = topology->nodemap.array[0]; > + libxl_for_each_cpu(c, cpumap) { > + if (!libxl_cpumap_test(&cpumap, c) && (c < topology->nodemap.entries) && > + (topology->nodemap.array[c] == node)) { > + ret = -libxl_cpupool_cpuadd(&ctx, poolid, c); > + if (ret) { > + fprintf(stderr, "error on adding cpu to Pool-0\n"); > + goto out; > + } > + libxl_cpumap_reset(&freemap, c);(nt really related to this series but I wish this was called libxl_cpumap_clear, I had to go check it wasn''t resetting the whole map or something...)> + } > + } > + libxl_for_each_cpu(c, cpumap) { > + if (libxl_cpumap_test(&cpumap, c) && (c < topology->nodemap.entries) && > + (topology->nodemap.array[c] != node)) { > + ret = -libxl_cpupool_cpuremove(&ctx, poolid, c); > + if (ret) { > + fprintf(stderr, "error on removing cpu from Pool-0\n"); > + goto out; > + } > + libxl_cpumap_set(&freemap, c); > + } > + }Can this loop be merged with the preceding loop, with the body being the else case of the if?> + for (;;) { > + node = -1; > + libxl_for_each_cpu(c, freemap) { > + if (libxl_cpumap_test(&freemap, c) && (node == -1)) { > + node = topology->nodemap.array[c]; > + } > + libxl_cpumap_reset(&cpumap, c); > + if ((node >= 0) && libxl_cpumap_test(&freemap, c) && > + (c < topology->nodemap.entries) && (topology->nodemap.array[c] == node)) { > + libxl_cpumap_reset(&freemap, c); > + libxl_cpumap_set(&cpumap, c); > + } > + } > + if (node == -1) > + break; > + > + snprintf(name, 15, "Pool-node%d", node);Do we want to rename Pool-0 at some point too or do we rely on that name elsewhere? Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Juergen Gross
2010-Dec-08 12:01 UTC
Re: [Xen-devel] [PATCH 1 of 4] Support getting topology info in libxl
On 12/08/10 11:51, Ian Campbell wrote:> On Fri, 2010-11-26 at 07:10 +0000, Juergen Gross wrote: > >> diff -r 79b71c77907b -r 37fdfe90e0c2 tools/libxl/libxl.h >> --- a/tools/libxl/libxl.h Wed Nov 24 10:20:03 2010 +0000 >> +++ b/tools/libxl/libxl.h Thu Nov 25 09:29:43 2010 +0100 >> @@ -148,6 +148,13 @@ >> uint8_t *map; >> } libxl_cpumap; >> void libxl_cpumap_destroy(libxl_cpumap *map); >> + >> +typedef struct { >> + uint32_t entries; >> + uint32_t *array; >> +} libxl_cpuarray; >> +#define LIBXL_CPUARRAY_INVENTRY ~0 > > This looked at first glance like you had misspelled INVENTORY. Perhaps > LIBXL_CPUARRAY_INVALID_ENTRY?Okay, I don''t mind.> >> +void libxl_cpuarray_destroy(libxl_cpuarray *array); >> >> typedef enum { >> XENFV = 1, >> @@ -464,6 +471,7 @@ >> int libxl_button_press(libxl_ctx *ctx, uint32_t domid, libxl_button button); >> >> int libxl_get_physinfo(libxl_ctx *ctx, libxl_physinfo *physinfo); >> +libxl_topologyinfo *libxl_get_topologyinfo(libxl_ctx *ctx); > > The idiom in libxl is for such functions to take a pointer to the > structure to fill in and return a status code, e.g. > int libxl_get_topologyinfo(libxl_ctx *ctx, libxl_topologyinfo *info) > e..g libxl_get_physinfo() > > This is useful since the caller doesn''t always need to manage the > dynamic allocation of the info structure e.g, it can use a stack > variable for temporary stuff or nest inside another structure etc.I''ll change it. Juergen -- Juergen Gross Principal Developer Operating Systems TSP ES&S SWE OS6 Telephone: +49 (0) 89 3222 2967 Fujitsu Technology Solutions e-mail: juergen.gross@ts.fujitsu.com Domagkstr. 28 Internet: ts.fujitsu.com D-80807 Muenchen Company details: ts.fujitsu.com/imprint.html _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Juergen Gross
2010-Dec-08 12:07 UTC
Re: [Xen-devel] [PATCH 3 of 4] Extend cpupools to support numa
On 12/08/10 11:58, Ian Campbell wrote:> See previous comments about the interface to libxl_get_topoplogy and > leaking the returned pointer value. > > On Fri, 2010-11-26 at 07:10 +0000, Juergen Gross wrote: >> + if (libxl_get_freecpus(&ctx,&freemap)) { >> + fprintf(stderr, "libxl_get_freecpus failed\n"); >> + return -ERROR_FAIL; >> + } >> + >> + topology = libxl_get_topologyinfo(&ctx); >> + if (topology == NULL) { >> + fprintf(stderr, "libxl_get_topologyinfo failed\n"); >> + return -ERROR_FAIL; >> + } >> + >> + n = 0; >> + for (cpu = 0; cpu< topology->nodemap.entries; cpu++) { >> + if (libxl_cpumap_test(&freemap, cpu)&& >> + (topology->nodemap.array[cpu] == node)&& >> + !libxl_cpupool_cpuadd(&ctx, poolid, cpu)) { >> + n++; >> + } >> + } > > This sequence of actions look like they would make a useful addition to > the libxl interface as a helper function. > > Not sure what it would be called, perhaps: > libxl_cpupool_cpuadd_node(&ctx, poolid, node) > ?Okay.> >> + for (p = 0; p< n_pools; p++) { >> + if (poolinfo[p].poolid == poolid) { >> + for (cpu = 0; cpu< topology->nodemap.entries; cpu++) { >> + if ((topology->nodemap.array[cpu] == node)&& >> + libxl_cpumap_test(&poolinfo[p].cpumap, cpu)&& >> + !libxl_cpupool_cpuremove(&ctx, poolid, cpu)) { >> + n++; >> + } >> + } >> + } >> + } > > also a helper function? > > libxl_cpupool_cpuremove_node(&ctx, poolid, node)Okay.> > Isn''t there an existing function to find a poolinfo from a poolid? (if > not then should there be?)Currently it is only possible to get infos for ALL cpupools. As the number of cpupools is expected to be not very large, this should be no problem. Perhaps it would make sense to create a helper function to locate the correct poolinfo member based on the poolid. Juergen -- Juergen Gross Principal Developer Operating Systems TSP ES&S SWE OS6 Telephone: +49 (0) 89 3222 2967 Fujitsu Technology Solutions e-mail: juergen.gross@ts.fujitsu.com Domagkstr. 28 Internet: ts.fujitsu.com D-80807 Muenchen Company details: ts.fujitsu.com/imprint.html _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Juergen Gross
2010-Dec-08 12:20 UTC
Re: [Xen-devel] [PATCH 4 of 4] Support new xl command cpupool-numa-split
On 12/08/10 12:16, Ian Campbell wrote:> On Fri, 2010-11-26 at 07:10 +0000, Juergen Gross wrote: >> + >> + if (libxl_cpumap_alloc(&ctx,&cpumap)) { >> + fprintf(stderr, "Failed to allocate cpumap\n"); >> + return -ERROR_FAIL; >> + } >> + >> + poolinfo = libxl_list_cpupool(&ctx,&n_pools); >> + if (!poolinfo) { >> + fprintf(stderr, "error getting cpupool info\n"); >> + return -ERROR_NOMEM; >> + } >> + poolid = poolinfo[0].poolid; >> + schedid = poolinfo[0].sched_id; >> + libxl_for_each_cpu(c, poolinfo[0].cpumap) >> + if (libxl_cpumap_test(&poolinfo[0].cpumap, c)) >> + libxl_cpumap_set(&cpumap, c); > > Can the libxl_cpumap_alloc and this loop be deferred until after the > check and bail for n_pools> 1?Sure.> > There seems to be no way to find out the number of pools without also > getting all the info about them, which is a shame.Taking a quick look I couldn''t spot any way how to find out the number of domains without also getting all the info about them, too...> >> + /* Reset Pool-0 to 1st node */ >> + node = topology->nodemap.array[0]; >> + libxl_for_each_cpu(c, cpumap) { >> + if (!libxl_cpumap_test(&cpumap, c)&& (c< topology->nodemap.entries)&& >> + (topology->nodemap.array[c] == node)) { >> + ret = -libxl_cpupool_cpuadd(&ctx, poolid, c); >> + if (ret) { >> + fprintf(stderr, "error on adding cpu to Pool-0\n"); >> + goto out; >> + } >> + libxl_cpumap_reset(&freemap, c); > > (nt really related to this series but I wish this was called > libxl_cpumap_clear, I had to go check it wasn''t resetting the whole map > or something...)Hmm, do you really think so? It would make me to check whether it is clearing the whole map :-) I think the second parameter is a strong hint :-)> >> + } >> + } >> + libxl_for_each_cpu(c, cpumap) { >> + if (libxl_cpumap_test(&cpumap, c)&& (c< topology->nodemap.entries)&& >> + (topology->nodemap.array[c] != node)) { >> + ret = -libxl_cpupool_cpuremove(&ctx, poolid, c); >> + if (ret) { >> + fprintf(stderr, "error on removing cpu from Pool-0\n"); >> + goto out; >> + } >> + libxl_cpumap_set(&freemap, c); >> + } >> + } > > Can this loop be merged with the preceding loop, with the body being the > else case of the if?No. I have to add new cpus first to avoid a cpupool without cpus in between.> >> + for (;;) { >> + node = -1; >> + libxl_for_each_cpu(c, freemap) { >> + if (libxl_cpumap_test(&freemap, c)&& (node == -1)) { >> + node = topology->nodemap.array[c]; >> + } >> + libxl_cpumap_reset(&cpumap, c); >> + if ((node>= 0)&& libxl_cpumap_test(&freemap, c)&& >> + (c< topology->nodemap.entries)&& (topology->nodemap.array[c] == node)) { >> + libxl_cpumap_reset(&freemap, c); >> + libxl_cpumap_set(&cpumap, c); >> + } >> + } >> + if (node == -1) >> + break; >> + >> + snprintf(name, 15, "Pool-node%d", node); > > Do we want to rename Pool-0 at some point too or do we rely on that name > elsewhere?Good question. There is a hard coded "Pool-0" reference in libxl, but this could easily be changed. I''m not sure about implications in xm/xend. I''ll check this. Juergen -- Juergen Gross Principal Developer Operating Systems TSP ES&S SWE OS6 Telephone: +49 (0) 89 3222 2967 Fujitsu Technology Solutions e-mail: juergen.gross@ts.fujitsu.com Domagkstr. 28 Internet: ts.fujitsu.com D-80807 Muenchen Company details: ts.fujitsu.com/imprint.html _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2010-Dec-08 13:12 UTC
Re: [Xen-devel] [PATCH 4 of 4] Support new xl command cpupool-numa-split
On Wed, 2010-12-08 at 12:20 +0000, Juergen Gross wrote:> On 12/08/10 12:16, Ian Campbell wrote: > > > > There seems to be no way to find out the number of pools without also > > getting all the info about them, which is a shame. > > Taking a quick look I couldn''t spot any way how to find out the number > of domains without also getting all the info about them, too...Yeah. It''s not important, just an observation.> > > > >> + /* Reset Pool-0 to 1st node */ > >> + node = topology->nodemap.array[0]; > >> + libxl_for_each_cpu(c, cpumap) { > >> + if (!libxl_cpumap_test(&cpumap, c)&& (c< topology->nodemap.entries)&& > >> + (topology->nodemap.array[c] == node)) { > >> + ret = -libxl_cpupool_cpuadd(&ctx, poolid, c); > >> + if (ret) { > >> + fprintf(stderr, "error on adding cpu to Pool-0\n"); > >> + goto out; > >> + } > >> + libxl_cpumap_reset(&freemap, c); > > > > (nt really related to this series but I wish this was called > > libxl_cpumap_clear, I had to go check it wasn''t resetting the whole map > > or something...) > > Hmm, do you really think so? > It would make me to check whether it is clearing the whole map :-);-) I think I''m just used to the Linux clear_bit type naming scheme.> I think the second parameter is a strong hint :-)True.> > Can this loop be merged with the preceding loop, with the body being the > > else case of the if? > > No. I have to add new cpus first to avoid a cpupool without cpus in between.ok. I was thinking that because this function only gets here if there is a single pool that all CPUs must be in that pool -- but that''s not actually true is it? Even if that were the common case there''s nothing to enforce that.> > Do we want to rename Pool-0 at some point too or do we rely on that name > > elsewhere? > > Good question. There is a hard coded "Pool-0" reference in libxl, but this > could easily be changed. > I''m not sure about implications in xm/xend. I''ll check this.I don''t think there is a particularly strong requirement to allow xend and xl to coexist. I''d recommend just leaving xend doing what it does today and fix xl/libxl only. Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Juergen Gross
2010-Dec-08 13:17 UTC
Re: [Xen-devel] [PATCH 4 of 4] Support new xl command cpupool-numa-split
On 12/08/10 14:12, Ian Campbell wrote:> On Wed, 2010-12-08 at 12:20 +0000, Juergen Gross wrote: >> On 12/08/10 12:16, Ian Campbell wrote: >>> Can this loop be merged with the preceding loop, with the body being the >>> else case of the if? >> >> No. I have to add new cpus first to avoid a cpupool without cpus in between. > > ok. > > I was thinking that because this function only gets here if there is a > single pool that all CPUs must be in that pool -- but that''s not > actually true is it? Even if that were the common case there''s nothing > to enforce that.Perhaps I should add a comment to avoid a problem later...> >>> Do we want to rename Pool-0 at some point too or do we rely on that name >>> elsewhere? >> >> Good question. There is a hard coded "Pool-0" reference in libxl, but this >> could easily be changed. >> I''m not sure about implications in xm/xend. I''ll check this. > > I don''t think there is a particularly strong requirement to allow xend > and xl to coexist. I''d recommend just leaving xend doing what it does > today and fix xl/libxl only.Okay, I''ll modify xl to rename Pool-0 to Pool-node[n]. Juergen -- Juergen Gross Principal Developer Operating Systems TSP ES&S SWE OS6 Telephone: +49 (0) 89 3222 2967 Fujitsu Technology Solutions e-mail: juergen.gross@ts.fujitsu.com Domagkstr. 28 Internet: ts.fujitsu.com D-80807 Muenchen Company details: ts.fujitsu.com/imprint.html _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2010-Dec-08 13:38 UTC
Re: [Xen-devel] [PATCH 4 of 4] Support new xl command cpupool-numa-split
On Wed, 2010-12-08 at 13:17 +0000, Juergen Gross wrote:> On 12/08/10 14:12, Ian Campbell wrote: > > On Wed, 2010-12-08 at 12:20 +0000, Juergen Gross wrote: > >> On 12/08/10 12:16, Ian Campbell wrote: > >>> Can this loop be merged with the preceding loop, with the body being the > >>> else case of the if? > >> > >> No. I have to add new cpus first to avoid a cpupool without cpus in between. > > > > ok. > > > > I was thinking that because this function only gets here if there is a > > single pool that all CPUs must be in that pool -- but that''s not > > actually true is it? Even if that were the common case there''s nothing > > to enforce that. > > Perhaps I should add a comment to avoid a problem later...That would certainly help. The alternative would be to bail out if all cpus are not associated with Pool-0, not just when there are > 1 pools. That would be consistent with the function only acting on the default configuration. Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Juergen Gross
2010-Dec-08 13:41 UTC
Re: [Xen-devel] [PATCH 4 of 4] Support new xl command cpupool-numa-split
On 12/08/10 14:38, Ian Campbell wrote:> On Wed, 2010-12-08 at 13:17 +0000, Juergen Gross wrote: >> On 12/08/10 14:12, Ian Campbell wrote: >>> On Wed, 2010-12-08 at 12:20 +0000, Juergen Gross wrote: >>>> On 12/08/10 12:16, Ian Campbell wrote: >>>>> Can this loop be merged with the preceding loop, with the body being the >>>>> else case of the if? >>>> >>>> No. I have to add new cpus first to avoid a cpupool without cpus in between. >>> >>> ok. >>> >>> I was thinking that because this function only gets here if there is a >>> single pool that all CPUs must be in that pool -- but that''s not >>> actually true is it? Even if that were the common case there''s nothing >>> to enforce that. >> >> Perhaps I should add a comment to avoid a problem later... > > That would certainly help. > > The alternative would be to bail out if all cpus are not associated with > Pool-0, not just when there are> 1 pools. That would be consistent with > the function only acting on the default configuration.I suspect NUMA systems are subject to cpu hot plug... I''d prefer the way I''ve done it before. It isn''t too complex and more flexible. Juergen -- Juergen Gross Principal Developer Operating Systems TSP ES&S SWE OS6 Telephone: +49 (0) 89 3222 2967 Fujitsu Technology Solutions e-mail: juergen.gross@ts.fujitsu.com Domagkstr. 28 Internet: ts.fujitsu.com D-80807 Muenchen Company details: ts.fujitsu.com/imprint.html _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2010-Dec-08 14:12 UTC
Re: [Xen-devel] [PATCH 4 of 4] Support new xl command cpupool-numa-split
On Wed, 2010-12-08 at 13:41 +0000, Juergen Gross wrote:> On 12/08/10 14:38, Ian Campbell wrote: > > On Wed, 2010-12-08 at 13:17 +0000, Juergen Gross wrote: > >> On 12/08/10 14:12, Ian Campbell wrote: > >>> On Wed, 2010-12-08 at 12:20 +0000, Juergen Gross wrote: > >>>> On 12/08/10 12:16, Ian Campbell wrote: > >>>>> Can this loop be merged with the preceding loop, with the body being the > >>>>> else case of the if? > >>>> > >>>> No. I have to add new cpus first to avoid a cpupool without cpus in between. > >>> > >>> ok. > >>> > >>> I was thinking that because this function only gets here if there is a > >>> single pool that all CPUs must be in that pool -- but that''s not > >>> actually true is it? Even if that were the common case there''s nothing > >>> to enforce that. > >> > >> Perhaps I should add a comment to avoid a problem later... > > > > That would certainly help. > > > > The alternative would be to bail out if all cpus are not associated with > > Pool-0, not just when there are> 1 pools. That would be consistent with > > the function only acting on the default configuration. > > I suspect NUMA systems are subject to cpu hot plug...And hotplugged CPUs don''t automatically go into Pool-0?> I''d prefer the way I''ve done it before. It isn''t too complex and more > flexible.Given the above constraint that seems reasonable. Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Juergen Gross
2010-Dec-09 09:45 UTC
Re: [Xen-devel] [PATCH 4 of 4] Support new xl command cpupool-numa-split
On 12/08/10 15:12, Ian Campbell wrote:> On Wed, 2010-12-08 at 13:41 +0000, Juergen Gross wrote: >> On 12/08/10 14:38, Ian Campbell wrote: >>> On Wed, 2010-12-08 at 13:17 +0000, Juergen Gross wrote: >>>> On 12/08/10 14:12, Ian Campbell wrote: >>>>> On Wed, 2010-12-08 at 12:20 +0000, Juergen Gross wrote: >>>>>> On 12/08/10 12:16, Ian Campbell wrote: >>>>>>> Can this loop be merged with the preceding loop, with the body being the >>>>>>> else case of the if? >>>>>> >>>>>> No. I have to add new cpus first to avoid a cpupool without cpus in between. >>>>> >>>>> ok. >>>>> >>>>> I was thinking that because this function only gets here if there is a >>>>> single pool that all CPUs must be in that pool -- but that''s not >>>>> actually true is it? Even if that were the common case there''s nothing >>>>> to enforce that. >>>> >>>> Perhaps I should add a comment to avoid a problem later... >>> >>> That would certainly help. >>> >>> The alternative would be to bail out if all cpus are not associated with >>> Pool-0, not just when there are> 1 pools. That would be consistent with >>> the function only acting on the default configuration. >> >> I suspect NUMA systems are subject to cpu hot plug... > > And hotplugged CPUs don''t automatically go into Pool-0?I just checked it: they ARE added to Pool-0. This rises another problem: it would be nice to adjust the NUMA splitting after a hot-plug of a cpu... An "update" option to xl cpupool-numa-split would be appropriate, I think. It should move cpus from Pool 0 to the correct node pools and create new node pools if necessary. I''ll prepare another patch later... Juergen -- Juergen Gross Principal Developer Operating Systems TSP ES&S SWE OS6 Telephone: +49 (0) 89 3222 2967 Fujitsu Technology Solutions e-mail: juergen.gross@ts.fujitsu.com Domagkstr. 28 Internet: ts.fujitsu.com D-80807 Muenchen Company details: ts.fujitsu.com/imprint.html _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Gianni Tedesco
2010-Dec-09 12:09 UTC
Re: [Xen-devel] [PATCH 1 of 4] Support getting topology info in libxl
On Fri, 2010-11-26 at 07:10 +0000, Juergen Gross wrote:> diff -r 79b71c77907b -r 37fdfe90e0c2 tools/python/xen/lowlevel/xl/xl.c > --- a/tools/python/xen/lowlevel/xl/xl.c Wed Nov 24 10:20:03 2010 +0000 > +++ b/tools/python/xen/lowlevel/xl/xl.c Thu Nov 25 09:29:43 2010 +0100 > @@ -224,6 +224,11 @@ > return 0; > } > > +int attrib__libxl_cpuarray_set(PyObject *v, libxl_cpuarray *pptr) > +{ > + return -1; > +} > + > int attrib__libxl_domain_build_state_ptr_set(PyObject *v, libxl_domain_build_state **pptr) > { > return -1; > @@ -284,6 +289,25 @@ > } > } > return cpulist; > +} > + > +PyObject *attrib__libxl_cpuarray_get(libxl_cpuarray *pptr) > +{ > + PyObject *list = NULL; > + int i; > + > + list = PyList_New(0); > + for (i = 0; i < pptr->entries; i++) { > + if (pptr->array[i] == LIBXL_CPUARRAY_INVENTRY) { > + PyList_Append(list, Py_None);Needs a Py_INCREF(Py_None); - this is a confusing rule about python lists. There are actually a slew of these Py_None refcounting bugs in there that I have a patch for and will send out at some point. Thanks for the patch Gianni _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel