Dario Faggioli wrote: > 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 > Yep, makes sense. And yes, both sockets share the same memory bus. The machine is an Intel DP server with Clovertown processors. > 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 :-) ). > Nod. > 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. Ok, no problem. As you said, it is an internal function and we can change it if some future code motion warrants such a change. Regards, Jim -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list