7:10 a.m.
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