[PATCH] libxl: use xc_topologyinfo to figure out how many CPUs we actually have

[PATCH] libxl: use xc_topologyinfo to figure out how many CPUs we actually have

[Darrio Faggioli]

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

Re: [PATCH] libxl: use xc_topologyinfo to figure out how many CPUs we actually have

[Dario Faggioli]

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

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)



[-- 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] libxl: use xc_topologyinfo to figure out how many CPUs we actually have

[Ian Jackson]

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.

Re: [PATCH] libxl: use xc_topologyinfo to figure out how many CPUs we actually have

[Dario Faggioli]

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

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)




[-- 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] libxl: use xc_topologyinfo to figure out how many CPUs we actually have

[Ian Campbell]

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

Re: [PATCH] libxl: use xc_topologyinfo to figure out how many CPUs we actually have

[Dario Faggioli]

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

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)



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