[libvirt] [PATCH 4/5] capabilities: Switch CPU data in NUMA topology to a struct

[Peter Krempa]

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
>