Re: [libvirt] [PATCHv2 6/7] capabilities: Add additional data to the NUMA topology info

On Tue, Jan 22, 2013 at 10:30:25PM +0100, Peter Krempa wrote: > This patch adds data gathering to the NUMA gathering files and adds > support for outputting the data. The test driver and xend driver need to > be adapted to fill sensible data to the structure. > --- > src/conf/capabilities.c | 31 ++++++++++++++++++++++----- > src/nodeinfo.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++--- > src/test/test_driver.c | 3 +++ > src/xen/xend_internal.c | 1 + > 4 files changed, 84 insertions(+), 8 deletions(-) > > diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c > index 14d3498..c9036f4 100644 > --- a/src/conf/capabilities.c > +++ b/src/conf/capabilities.c > @@ -686,27 +686,47 @@ virCapabilitiesDefaultGuestEmulator(virCapsPtr caps, > return NULL; > } > > -static void > +static int > virCapabilitiesFormatNUMATopology(virBufferPtr xml, > size_t ncells, > virCapsHostNUMACellPtr *cells) > { > int i; > int j; > + char *siblings; > > virBufferAddLit(xml, " <topology>\n"); > virBufferAsprintf(xml, " <cells num='%zu'>\n", ncells); > for (i = 0; i < ncells; i++) { > virBufferAsprintf(xml, " <cell id='%d'>\n", cells[i]->num); > virBufferAsprintf(xml, " <cpus num='%d'>\n", cells[i]->ncpus); > - for (j = 0; j < cells[i]->ncpus; j++) > - virBufferAsprintf(xml, " <cpu id='%d'/>\n", > + for (j = 0; j < cells[i]->ncpus; j++) { > + virBufferAsprintf(xml, " <cpu id='%d'", > cells[i]->cpus[j].id); > + > + if (cells[i]->cpus[j].siblings) { > + if (!(siblings = virBitmapFormat(cells[i]->cpus[j].siblings))) { > + virReportOOMError(); > + return -1; > + } > + > + virBufferAsprintf(xml, > + " socket_id='%d' core_id='%d' siblings='%s'", > + cells[i]->cpus[j].socket_id, > + cells[i]->cpus[j].core_id, > + siblings); > + VIR_FREE(siblings); > + } > + virBufferAddLit(xml, "/>\n"); > + } > + > virBufferAddLit(xml, " </cpus>\n"); > virBufferAddLit(xml, " </cell>\n"); > } > virBufferAddLit(xml, " </cells>\n"); > virBufferAddLit(xml, " </topology>\n"); > + > + return 0; > } > > /** > @@ -782,9 +802,10 @@ virCapabilitiesFormatXML(virCapsPtr caps) > virBufferAddLit(&xml, " </migration_features>\n"); > } > > - if (caps->host.nnumaCell) > + if (caps->host.nnumaCell && > virCapabilitiesFormatNUMATopology(&xml, caps->host.nnumaCell, > - caps->host.numaCell); > + caps->host.numaCell) < 0) > + return NULL; > > for (i = 0; i < caps->host.nsecModels; i++) { > virBufferAddLit(&xml, " <secmodel>\n"); > diff --git a/src/nodeinfo.c b/src/nodeinfo.c > index 5febcfb..dc6771a 100644 > --- a/src/nodeinfo.c > +++ b/src/nodeinfo.c > @@ -79,9 +79,11 @@ freebsdNodeGetCPUCount(void) > #ifdef __linux__ > # define CPUINFO_PATH "/proc/cpuinfo" > # define SYSFS_SYSTEM_PATH "/sys/devices/system" > +# define SYSFS_CPU_PATH SYSFS_SYSTEM_PATH"/cpu" > # define PROCSTAT_PATH "/proc/stat" > # define MEMINFO_PATH "/proc/meminfo" > # define SYSFS_MEMORY_SHARED_PATH "/sys/kernel/mm/ksm" > +# define SYSFS_THREAD_SIBLINGS_LIST_LENGHT_MAX 1024 > > # define LINUX_NB_CPU_STATS 4 > # define LINUX_NB_MEMORY_STATS_ALL 4 > @@ -1473,6 +1475,52 @@ cleanup: > # define MASK_CPU_ISSET(mask, cpu) \ > (((mask)[((cpu) / n_bits(*(mask)))] >> ((cpu) % n_bits(*(mask)))) & 1) > > +static virBitmapPtr > +virNodeGetSiblingsList(const char *dir, int cpu_id) > +{ > + char *path = NULL; > + char *buf = NULL; > + virBitmapPtr ret = NULL; > + > + if (virAsprintf(&path, "%s/cpu%u/topology/thread_siblings_list", > + dir, cpu_id) < 0) { > + virReportOOMError(); > + goto cleanup; > + } > + > + if (virFileReadAll(path, SYSFS_THREAD_SIBLINGS_LIST_LENGHT_MAX, &buf) < 0) > + goto cleanup; > + > + if (virBitmapParse(buf, ',', &ret, NUMA_MAX_N_CPUS) < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("Failed to parse thread siblings")); > + goto cleanup; > + } > + > +cleanup: > + VIR_FREE(buf); > + VIR_FREE(path); > + return ret; > +} > + > +static int > +virNodeCapsFillCPUInfo(int cpu_id, virCapsHostNUMACellCPUPtr cpu) > +{ > + cpu->id = cpu_id; > + cpu->socket_id = virNodeGetCpuValue(SYSFS_CPU_PATH, cpu_id, > + "topology/physical_package_id", -1); > + cpu->core_id = virNodeGetCpuValue(SYSFS_CPU_PATH, cpu_id, > + "topology/core_id", -1); > + > + if (cpu->socket_id == -1 || cpu->core_id == -1) > + return 0; > + > + if (!(cpu->siblings = virNodeGetSiblingsList(SYSFS_CPU_PATH, cpu_id))) > + return -1; > + > + return 0; > +} > + > int > nodeCapsInitNUMA(virCapsPtr caps) > { > @@ -1516,9 +1564,12 @@ nodeCapsInitNUMA(virCapsPtr caps) > if (VIR_ALLOC_N(cpus, ncpus) < 0) > goto cleanup; > > - for (ncpus = 0, i = 0 ; i < max_n_cpus ; i++) > - if (MASK_CPU_ISSET(mask, i)) > - cpus[ncpus++].id = i; > + for (ncpus = 0, i = 0 ; i < max_n_cpus ; i++) { > + if (MASK_CPU_ISSET(mask, i)) { > + if (virNodeCapsFillCPUInfo(i, cpus + ncpus++) < 0) > + goto cleanup; > + } > + } > > if (virCapabilitiesAddHostNUMACell(caps, n, ncpus, cpus) < 0) > goto cleanup; > diff --git a/src/test/test_driver.c b/src/test/test_driver.c > index 6909fa4..038f627 100644 > --- a/src/test/test_driver.c > +++ b/src/test/test_driver.c > @@ -559,6 +559,9 @@ static int testOpenDefault(virConnectPtr conn) { > } > for (u = 0 ; u < 16 ; u++) { > privconn->cells[u % 2].cpus[(u / 2)].id = u; > + privconn->cells[u % 2].cpus[(u / 2)].socket_id = -1; > + privconn->cells[u % 2].cpus[(u / 2)].core_id = -1; > + privconn->cells[u % 2].cpus[(u / 2)].siblings = NULL; > } This is wrong because these fields are unsigned int.

> diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c > index 57d8325..434f558 100644 > --- a/src/xen/xend_internal.c > +++ b/src/xen/xend_internal.c > @@ -1161,6 +1161,7 @@ sexpr_to_xend_topology(const struct sexpr *root, > ignore_value(virBitmapGetBit(cpuset, cpu, &used)); > if (used) { > cpuInfo[n].id = cpu; > + cpuInfo[n].siblings = NULL; As mentioned before, this should be initializing based on the nodeinfo. Here you've allowed socket_id + core_id to all initialize to 0 which is wrong.

Daniel