On 01/21/13 12:31, Martin Kletzander wrote:
On 01/19/2013 12:06 AM, Peter Krempa wrote:
This will allow storing additional topology data in the NUMA topology definition.
This patch changes the storage type and fixes fallback of the change across the drivers using it.
This patch also changes semantics of adding new NUMA cell information. Until now the data were re-allocated and copied to the topology definition. This patch changes the addition function to steal the pointer to a pre-allocated structure to simplify the code. --- src/conf/capabilities.c | 29 ++++++++++++++++++----------- src/conf/capabilities.h | 14 ++++++++++++-- src/nodeinfo.c | 22 ++++++++++++---------- src/qemu/qemu_process.c | 2 +- src/test/test_driver.c | 15 ++++++++++++--- src/xen/xend_internal.c | 24 +++++++++++++----------- 6 files changed, 68 insertions(+), 38 deletions(-)
[...]
diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h index 19b99c6..124d8c1 100644 --- a/src/conf/capabilities.h +++ b/src/conf/capabilities.h @@ -84,12 +84,21 @@ struct _virCapsGuest { virCapsGuestFeaturePtr *features; };
+typedef struct _virCapsHostNUMACellCPU virCapsHostNUMACellCPU; +typedef virCapsHostNUMACellCPU *virCapsHostNUMACellCPUPtr; +struct _virCapsHostNUMACellCPU { + int id; + int socket_id; + int core_id; + char *siblings_list;
We don't actually need this for anything right now, but it would be nice to have it stored in virBitmap in case we'll want to work with it in the future.
[...]
diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 477104f..dffe7d1 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"
s/PATH"/PATH "/
# 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 @@ -1476,9 +1478,10 @@ nodeCapsInitNUMA(virCapsPtr caps) int n; unsigned long *mask = NULL; unsigned long *allonesmask = NULL; - int *cpus = NULL; + virCapsHostNUMACellCPUPtr cpus = NULL; int ret = -1; int max_n_cpus = NUMA_MAX_N_CPUS; + int ncpus = 0;
if (numa_available() < 0) return 0; @@ -1492,7 +1495,6 @@ nodeCapsInitNUMA(virCapsPtr caps)
for (n = 0 ; n <= numa_max_node() ; n++) { int i; - int ncpus; /* The first time this returns -1, ENOENT if node doesn't exist... */ if (numa_node_to_cpus(n, mask, mask_n_bytes) < 0) { VIR_WARN("NUMA topology for cell %d of %d not available, ignoring", @@ -1515,21 +1517,21 @@ nodeCapsInitNUMA(virCapsPtr caps)
for (ncpus = 0, i = 0 ; i < max_n_cpus ; i++) if (MASK_CPU_ISSET(mask, i)) - cpus[ncpus++] = i; + cpus[ncpus++].id = i;
- if (virCapabilitiesAddHostNUMACell(caps, - n, - ncpus, - cpus) < 0) + if (virCapabilitiesAddHostNUMACell(caps, n, ncpus, cpus) < 0) goto cleanup; - - VIR_FREE(cpus); + cpus = NULL;
I generally don't like stealing pointers because of similar contraptions. Even though it is clear from memory allocation POV and it saves a lot of allocations in this case, it's a bit harder to read IMHO.
Well, yeah, it might be harder to read, but I wanted to avoid doing a deep copy on each iteration. And the callers of this function freed the original array anyways so I think it was really a waste of operations.
Being the devil's advocate, I must say I'd be nice to have the tests for this since you've started modifying the test driver. Also documentation for all the new information should be added into 'docs/formatcaps.html.in' with some explanation.
I'm planing on adding docs and tests, but I was waiting for a review to settle the format before doing so. (And I forgot to write about that)
Xen part of this patch looks OK, although I don't have any XEN machine to try it on.
Because this and [PATCH 5/5] both require [PATCH 3/5] to be in, I'm cond-ACKing this patch with the same condition as [PATCH 3/5].
I assume that you mean patch 2/5 :)
Martin