Darrio Faggioli
2012-May-09 11:03 UTC
[PATCH] libxl: use xc_topologyinfo to figure out how many CPUs we actually have
Within libxl_get_cpu_topology(), considering all the CPUs the hypervisor
supports to be valid topology entries might lead to misleading and incorrect
behaviours, e.g., the output of `xl info -n'' below on a 16 cores
machine:
...
cpu_topology :
cpu: core socket node
0: 0 1 0
1: 0 1 0
2: 1 1 0
3: 1 1 0
4: 9 1 0
5: 9 1 0
6: 10 1 0
7: 10 1 0
8: 0 0 1
9: 0 0 1
10: 1 0 1
11: 1 0 1
12: 9 0 1
13: 9 0 1
14: 10 0 1
15: 10 0 1
16: 0 0 0
17: 0 0 0
18: 0 0 0
19: 0 0 0
20: 0 0 0
...
...
62: 0 0 0
63: 0 0 0
However, xc_topologyinfo() tells us (in max_cpu_index) how many entries
arrays it returns corresponds to actually valid CPUs, so let''s use that
information.
Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -2903,7 +2903,8 @@ libxl_cputopology *libxl_get_cpu_topolog
}
for (i = 0; i < max_cpus; i++) {
-#define V(map, i) (map[i] == INVALID_TOPOLOGY_ID) ? \
+#define V(map, i) (i > tinfo.max_cpu_index || \
+ map[i] == INVALID_TOPOLOGY_ID) ? \
LIBXL_CPUTOPOLOGY_INVALID_ENTRY : map[i]
ret[i].core = V(coremap, i);
ret[i].socket = V(socketmap, i);
Dario Faggioli
2012-May-09 11:11 UTC
Re: [PATCH] libxl: use xc_topologyinfo to figure out how many CPUs we actually have
On Wed, 2012-05-09 at 13:03 +0200, Darrio Faggioli wrote:> Within libxl_get_cpu_topology(), considering all the CPUs the hypervisor > supports to be valid topology entries might lead to misleading and incorrect > behaviours, e.g., the output of `xl info -n'' below on a 16 cores machine: > ... > <snip> > > However, xc_topologyinfo() tells us (in max_cpu_index) how many entries > arrays it returns corresponds to actually valid CPUs, so let''s use that > information. > > Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com> >And, as it is an actual bugfix and very very small in size, not to forget that I''d need that for the NUMA placement series, I''m proposing this for 4.2... Does it make sense? Thanks and Regards, Dario -- <<This happens because I choose it to happen!>> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://retis.sssup.it/people/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 Campbell
2012-May-09 12:04 UTC
Re: [PATCH] libxl: use xc_topologyinfo to figure out how many CPUs we actually have
On Wed, 2012-05-09 at 12:03 +0100, Darrio Faggioli wrote:> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c > --- a/tools/libxl/libxl.c > +++ b/tools/libxl/libxl.c > @@ -2903,7 +2903,8 @@ libxl_cputopology *libxl_get_cpu_topolog > } > > for (i = 0; i < max_cpus; i++) { > -#define V(map, i) (map[i] == INVALID_TOPOLOGY_ID) ? \ > +#define V(map, i) (i > tinfo.max_cpu_index || \ > + map[i] == INVALID_TOPOLOGY_ID) ? \This ensures that cpus entries above max_cpu_index are INVALID_TOPOLOGY_ID but do you also want to size the return array using tinfo.max_cpu_index too? And also return that in *nr instead of? (I don''t know the answer, either of max-possible- and max-online-cpus is a reasonable size for this array)> LIBXL_CPUTOPOLOGY_INVALID_ENTRY : map[i] > ret[i].core = V(coremap, i); > ret[i].socket = V(socketmap, i);
Dario Faggioli
2012-May-09 12:45 UTC
Re: [PATCH] libxl: use xc_topologyinfo to figure out how many CPUs we actually have
On Wed, 2012-05-09 at 13:04 +0100, Ian Campbell wrote:> On Wed, 2012-05-09 at 12:03 +0100, Darrio Faggioli wrote: > > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c > > --- a/tools/libxl/libxl.c > > +++ b/tools/libxl/libxl.c > > @@ -2903,7 +2903,8 @@ libxl_cputopology *libxl_get_cpu_topolog > > } > > > > for (i = 0; i < max_cpus; i++) { > > -#define V(map, i) (map[i] == INVALID_TOPOLOGY_ID) ? \ > > +#define V(map, i) (i > tinfo.max_cpu_index || \ > > + map[i] == INVALID_TOPOLOGY_ID) ? \ > > This ensures that cpus entries above max_cpu_index are > INVALID_TOPOLOGY_ID >Yep, thus giving consumers of this call the chance to figure out what the valid entries are. It looked the most natural way of doing this, given output_topology (for instance) already check for LIBXL_CPUTOPOLOGY_INVALID_ENTRY entries and skip them. Also, the get-cpu-topology command in xenpm, which always worked properly on my testbox, does exactly that.> but do you also want to size the return array using > tinfo.max_cpu_index too? And also return that in *nr instead of? > > (I don''t know the answer, either of max-possible- and max-online-cpus is > a reasonable size for this array) >Well, I really don''t know either. This is the minimum impact working version. For sure, I''d like very much to return that value via *nr, as it will give me something more sensible to work with... You know what, I''m sending a patch doing exactly that, as I think what you''re suggesting is actually better. Thanks, Dario -- <<This happens because I choose it to happen!>> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://retis.sssup.it/people/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
2012-May-09 15:23 UTC
Re: [PATCH] libxl: use xc_topologyinfo to figure out how many CPUs we actually have
Dario Faggioli writes ("Re: [PATCH] libxl: use xc_topologyinfo to figure
out how many CPUs we actually have"):> And, as it is an actual bugfix and very very small in size, not to
> forget that I''d need that for the NUMA placement series,
I''m proposing
> this for 4.2... Does it make sense?
It seems to to me that this is indeed 4.2 material.
Ian.
Dario Faggioli
2012-May-10 09:26 UTC
Re: [PATCH] libxl: use xc_topologyinfo to figure out how many CPUs we actually have
On Wed, 2012-05-09 at 16:23 +0100, Ian Jackson wrote:> Dario Faggioli writes ("Re: [PATCH] libxl: use xc_topologyinfo to figure out how many CPUs we actually have"): > > And, as it is an actual bugfix and very very small in size, not to > > forget that I''d need that for the NUMA placement series, I''m proposing > > this for 4.2... Does it make sense? > > It seems to to me that this is indeed 4.2 material. >Cool. :-) Aside from that, any comments (better if on the v2 here http://permalink.gmane.org/gmane.comp.emulators.xen.devel/130218) ? Regards, Dario -- <<This happens because I choose it to happen!>> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://retis.sssup.it/people/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