On lun, 2013-07-08 at 18:45 -0600, Jim Fehlig wrote:
> Dario Faggioli wrote:
>
> > diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> > index e170357..7515995 100644
> > --- a/src/libxl/libxl_conf.c
> > +++ b/src/libxl/libxl_conf.c
> > @@ -161,6 +161,115 @@ libxlBuildCapabilities(virArch hostarch,
> > }
> >
> > static virCapsPtr
> > +libxlMakeNumaCapabilities(libxl_numainfo *numa_info,
> > + int nr_nodes,
> > + libxl_cputopology *cpu_topo,
> > + int nr_cpus,
> > + virCapsPtr caps)
> >
>
> I think this should return an int (0 success, -1 failure).
>
Well, of course I can do that (but see below)
> Noticed the following topology on an old 2 socket, quad core Intel
> machine I'm using to test this patch
>
> <topology>
> <cells num='1'>
> <cell id='0'>
> <memory unit='KiB'>9175040</memory>
> <cpus num='8'>
> <cpu id='0' socket_id='0' core_id='0' siblings='0,4'/>
> <cpu id='1' socket_id='0' core_id='1' siblings='1,5'/>
> <cpu id='2' socket_id='0' core_id='2' siblings='2,6'/>
> <cpu id='3' socket_id='0' core_id='3' siblings='3,7'/>
> <cpu id='4' socket_id='1' core_id='0' siblings='0,4'/>
> <cpu id='5' socket_id='1' core_id='1' siblings='1,5'/>
> <cpu id='6' socket_id='1' core_id='2' siblings='2,6'/>
> <cpu id='7' socket_id='1' core_id='3' siblings='3,7'/>
> </cpus>
> </cell>
> </cells>
> </topology>
>
> I'm not sure how cpus 0 and 4 can be siblings when they are on different
> sockets, but seems xl reports similar info
>
Oh, I see, my bad, same coreID but different socketID means !siblings
_even_ if on the same node. :-P
That makes sense, what I was not thinking to was the possibility of
having different sockets _within_ the same node, which seems like your
case according to both libxl and numactl.
Does that make sense? What machine is that? Do both the socket share the
same memory bus? Again, it looks like so
Anyway, I will fix that.
> cpu_topology :
> cpu: core socket node
> 0: 0 0 0
> 1: 1 0 0
> 2: 2 0 0
> 3: 3 0 0
> 4: 0 1 0
> 5: 1 1 0
> 6: 2 1 0
> 7: 3 1 0
> numa_info :
> node: memsize memfree distances
> 0: 8960 7116 10
>
> BTW, the qemu driver reports the following on the same machine
>
> <topology>
> <cells num='1'>
> <cell id='0'>
> <memory unit='KiB'>8387124</memory>
> <cpus num='8'>
> <cpu id='0' socket_id='0' core_id='0' siblings='0'/>
> <cpu id='1' socket_id='1' core_id='0' siblings='1'/>
> <cpu id='2' socket_id='0' core_id='1' siblings='2'/>
> <cpu id='3' socket_id='1' core_id='1' siblings='3'/>
> <cpu id='4' socket_id='0' core_id='2' siblings='4'/>
> <cpu id='5' socket_id='1' core_id='2' siblings='5'/>
> <cpu id='6' socket_id='0' core_id='3' siblings='6'/>
> <cpu id='7' socket_id='1' core_id='3' siblings='7'/>
> </cpus>
> </cell>
> </cells>
> </topology>
>
> which seems correct since hyperthreading is not supported.
>
Yes, and it is basically the same, apart from the ordering, than what
libxl says (notice that all cpus belongs to node 0!). Again, I will fix
the siblings part in my patch, in order to take the case of "more
sockets per node" into better account.
> > +static virCapsPtr
> > libxlMakeCapabilitiesInternal(virArch hostarch,
> > libxl_physinfo *phy_info,
> > char *capabilities)
> > @@ -772,7 +881,11 @@ libxlMakeCapabilities(libxl_ctx *ctx)
> > {
> > int err;
> > libxl_physinfo phy_info;
> > + libxl_numainfo *numa_info = NULL;
> > + libxl_cputopology *cpu_topo = NULL;
> > const libxl_version_info *ver_info;
> > + int nr_nodes = 0, nr_cpus = 0;
> > + virCapsPtr caps;
> >
> > err = regcomp(&xen_cap_rec, xen_cap_re, REG_EXTENDED);
> > if (err != 0) {
> > @@ -796,9 +909,35 @@ libxlMakeCapabilities(libxl_ctx *ctx)
> > return NULL;
> > }
> >
> > - return libxlMakeCapabilitiesInternal(virArchFromHost(),
> > + /* Let's try to fetch NUMA info, but it is not critical if we fail */
> > + numa_info = libxl_get_numainfo(ctx, &nr_nodes);
> > + if (numa_info == NULL)
> > + VIR_WARN("libxl_get_numainfo failed to retrieve NUMA data");
> > + else {
> > + /* If the above failed, we'd have no NUMa caps anyway! */
> > + cpu_topo = libxl_get_cpu_topology(ctx, &nr_cpus);
> > + if (cpu_topo == NULL) {
> > + VIR_WARN("libxl_get_cpu_topology failed to retrieve topology");
> > + libxl_numainfo_list_free(numa_info, nr_nodes);
> > + }
> > + }
> >
>
> Move these after the call to libxlMakeCapabilitiesInternal, before
> calling libxlMakeNumaCapabilities.
>
Makes sense, will do.
> > +
> > + caps = libxlMakeCapabilitiesInternal(virArchFromHost(),
> > &phy_info,
> > ver_info->capabilities);
> >
>
> Check that caps != NULL ?
>
Ok.
> > +
> > + /* Add topology information to caps, if available */
> > + if (numa_info && cpu_topo)
> > + caps = libxlMakeNumaCapabilities(numa_info,
> > + nr_nodes,
> > + cpu_topo,
> > + nr_cpus,
> > + caps);
> >
>
> Hmm, guess there is not much to check wrt errors at this point, so
> libxlMakeNumaCapabilities could return void. I suppose returning
> success or failure via int is more libvirt style.
>
And here's the return 0 or -1 point. The point is we do not care much
about errors as, if something bad happened during the construction of
NUMA capabilities, we revert all we've done, with the effect of leaving
caps unmodified, wrt the one libxlMakeNumaCapabilities() was given.
That is why errors are reported and handled (as described above) inside
the function itself, and that is therefore why I don't think it would be
that useful to have it propagate such information to the caller in such
an explicit manner as returning 0 or -1.
Moreover, given as you said yourself it could well return void, I
figured it was worthwhile to use the return value for something useful,
not to mention that, yes, 0/-1 will make it more libvirt style, but this
is an internal function that, at least according to me, should look more
like libxlMakeCapabilitiesInternal() than like anything else (and
libxlMakeCapabilitiesInternal() was returning a virCapsPtr before my
patch :-) ).
So, in summary, I agree on the code motion above, but I think the
signature and usage of libxlMakeNumaCapabilities() should stay as it is
now. That being said, I will of course do the change if you insist that
this is what you want. So, your call, I was just trying to give some
more --hopefully useful-- context and rationale bits to reason on...
After all, that is what maintainership is about, isn't it! :-P
Thanks again 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:
signature.asc
Description: This is a digitally signed message part
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list