Starting from Xen 4.2, libxl has all the bits and pieces in place for retrieving an adequate amount of information about the host NUMA topology. It is therefore possible, after a bit of shuffling, to arrange those information in the way libvirt wants to present them to the outside world. Therefore, with this patch, the <topology> section of the host capabilities is properly populated, when running on Xen, so that we can figure out whether or not we're running on a NUMA host, and what its characteristics are. [raistlin@Zhaman ~]$ sudo virsh --connect xen:/// capabilities <capabilities> <host> <cpu> .... <topology> <cells num='2'> <cell id='0'> <memory unit='KiB'>6291456</memory> <cpus num='8'> <cpu id='0' socket_id='1' core_id='0' siblings='0-1'/> <cpu id='1' socket_id='1' core_id='0' siblings='0-1'/> <cpu id='2' socket_id='1' core_id='1' siblings='2-3'/> <cpu id='3' socket_id='1' core_id='1' siblings='2-3'/> <cpu id='4' socket_id='1' core_id='9' siblings='4-5'/> <cpu id='5' socket_id='1' core_id='9' siblings='4-5'/> <cpu id='6' socket_id='1' core_id='10' siblings='6-7'/> <cpu id='7' socket_id='1' core_id='10' siblings='6-7'/> </cpus> </cell> <cell id='1'> <memory unit='KiB'>6881280</memory> <cpus num='8'> <cpu id='8' socket_id='0' core_id='0' siblings='8-9'/> <cpu id='9' socket_id='0' core_id='0' siblings='8-9'/> <cpu id='10' socket_id='0' core_id='1' siblings='10-11'/> <cpu id='11' socket_id='0' core_id='1' siblings='10-11'/> <cpu id='12' socket_id='0' core_id='9' siblings='12-13'/> <cpu id='13' socket_id='0' core_id='9' siblings='12-13'/> <cpu id='14' socket_id='0' core_id='10' siblings='14-15'/> <cpu id='15' socket_id='0' core_id='10' siblings='14-15'/> </cpus> </cell> </cells> </topology> </host> .... Signed-off-by: Dario Faggioli <dario.faggioli(a)citrix.com&gt; --- Changes from v1: * fixed a typo in the commit message, as requested during review; * fixed coding style (one function parameters per line and no spaces between variable definitions), as requested during review; * avoid zero-filling memory after VIR_ALLOC_N(), since it does that already, as requested during review; * improved out of memory error reporting, as requested during review; * libxlMakeNumaCapabilities() created, accommodating all the NUMA related additions, instead of having them within libxlMakeCapabilitiesInternal(), as suggested during review. --- src/libxl/libxl_conf.c | 141 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 140 insertions(+), 1 deletion(-) 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) +{ + virCapsHostNUMACellCPUPtr *cpus = NULL; + int *nr_cpus_node = NULL; + bool numa_failed = false; + int i; + + if (VIR_ALLOC_N(cpus, nr_nodes)) { + virReportOOMError(); + return caps; + } + + if (VIR_ALLOC_N(nr_cpus_node, nr_nodes)) { + VIR_FREE(cpus); + virReportOOMError(); + return caps; + } + + /* For each node, prepare a list of CPUs belonging to that node */ + for (i = 0; i < nr_cpus; i++) { + int node = cpu_topo[i].node; + + if (cpu_topo[i].core == LIBXL_CPUTOPOLOGY_INVALID_ENTRY) + continue; + + nr_cpus_node[node]++; + + if (nr_cpus_node[node] == 1) { + if (VIR_ALLOC(cpus[node]) < 0) { + virReportOOMError(); + numa_failed = true; + goto cleanup; + } + } + else { + if (VIR_REALLOC_N(cpus[node], nr_cpus_node[node]) < 0) { + virReportOOMError(); + numa_failed = true; + goto cleanup; + } + } + + /* Mapping between what libxl tells and what libvirt wants */ + cpus[node][nr_cpus_node[node]-1].id = i; + cpus[node][nr_cpus_node[node]-1].socket_id = cpu_topo[i].socket; + cpus[node][nr_cpus_node[node]-1].core_id = cpu_topo[i].core; + /* Allocate the siblings maps. We will be filling them later */ + cpus[node][nr_cpus_node[node]-1].siblings = virBitmapNew(nr_cpus); + if (!cpus[node][nr_cpus_node[node]-1].siblings) { + virReportOOMError(); + numa_failed = true; + goto cleanup; + } + } + + /* Let's now populate the siblings bitmaps */ + for (i = 0; i < nr_cpus; i++) { + int j, node = cpu_topo[i].node; + + if (cpu_topo[i].core == LIBXL_CPUTOPOLOGY_INVALID_ENTRY) + continue; + + for (j = 0; j < nr_cpus_node[node]; j++) { + if (cpus[node][j].core_id == cpu_topo[i].core) + ignore_value(virBitmapSetBit(cpus[node][j].siblings, i)); + } + } + + for (i = 0; i < nr_nodes; i++) { + if (numa_info[i].size == LIBXL_NUMAINFO_INVALID_ENTRY) + continue; + + if (virCapabilitiesAddHostNUMACell(caps, i, nr_cpus_node[i], + numa_info[i].size / 1024, + cpus[i]) < 0) { + virCapabilitiesClearHostNUMACellCPUTopology(cpus[i], + nr_cpus_node[i]); + numa_failed = true; + goto cleanup; + } + + /* This is safe, as the CPU list is now stored in the NUMA cell */ + cpus[i] = NULL; + } + + cleanup: + + if (numa_failed) { + /* Looks like something went wrong. Well, that's bad, but probably + * not enough to break the whole driver, so we log and carry on */ + for (i = 0; i < nr_nodes; i++) { + VIR_FREE(cpus[i]); + } + VIR_WARN("Failed to retrieve and build host NUMA topology properly,\n" + "disabling NUMA capabilities"); + virCapabilitiesFreeNUMAInfo(caps); + } + + VIR_FREE(cpus); + VIR_FREE(nr_cpus_node); + + return caps; +} + +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); + } + } + + caps = libxlMakeCapabilitiesInternal(virArchFromHost(), &phy_info, ver_info->capabilities); + + /* Add topology information to caps, if available */ + if (numa_info && cpu_topo) + caps = libxlMakeNumaCapabilities(numa_info, + nr_nodes, + cpu_topo, + nr_cpus, + caps); + + libxl_cputopology_list_free(cpu_topo, nr_cpus); + libxl_numainfo_list_free(numa_info, nr_nodes); + + return caps; } int

On lun, 2013-07-08 at 18:45 -0600, Jim Fehlig wrote: Well, of course I can do that (but see below) 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. 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. Makes sense, will do. Ok. 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)

Dario Faggioli wrote: BTW, the 'i' and 'j' iterators should be changed to size_t. danpb has a patchset to sanitize iterators that will be committed any time now, so might as well make this change in your next version https://www.redhat.com/archives/libvir-list/2013-July/msg00397.html Regards, Jim

domainGetNumaParameters has a string typed parameter, hence it is necessary for the libxl driver to support this. This change implements the connectSupportsFeature hook for the libxl driver, advertising that VIR_DRV_FEATURE_TYPED_PARAM_STRING is supported. Signed-off-by: Dario Faggioli <dario.faggioli(a)citrix.com&gt; Cc: Eric Blake <eblake(a)redhat.com&gt; --- Changes from v1: * commit message improved, as requested during review; * added VIR_TYPED_PARAM_STRING_OKAY handling code in libxlDomainGetSchedulerParametersFlags(); --- src/libxl/libxl_driver.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 1bae3d6..7555bc6 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -4364,7 +4364,10 @@ libxlDomainGetSchedulerParametersFlags(virDomainPtr dom, libxl_scheduler sched_id; int ret = -1; - virCheckFlags(0, -1); + virCheckFlags(VIR_TYPED_PARAM_STRING_OKAY, -1); + + /* We don't return strings, and thus trivially support this flag. */ + flags &= ~VIR_TYPED_PARAM_STRING_OKAY; libxlDriverLock(driver); vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); @@ -4665,6 +4668,20 @@ libxlConnectListAllDomains(virConnectPtr conn, return ret; } +/* Which features are supported by this driver? */ +static int +libxlConnectSupportsFeature(virConnectPtr conn, int feature) +{ + if (virConnectSupportsFeatureEnsureACL(conn) < 0) + return -1; + + switch (feature) { + case VIR_DRV_FEATURE_TYPED_PARAM_STRING: + return 1; + default: + return 0; + } +} static virDriver libxlDriver = { @@ -4745,6 +4762,7 @@ static virDriver libxlDriver = { .connectDomainEventRegisterAny = libxlConnectDomainEventRegisterAny, /* 0.9.0 */ .connectDomainEventDeregisterAny = libxlConnectDomainEventDeregisterAny, /* 0.9.0 */ .connectIsAlive = libxlConnectIsAlive, /* 0.9.8 */ + .connectSupportsFeature = libxlConnectSupportsFeature, /* 1.1.1 */ }; static virStateDriver libxlStateDriver = {