[libvirt] [PATCH] fix size parameter in calls to numa_node_to_cpus

[Dave Allan]

Jim Meyering wrote:
> Dave Allan <dallan at redhat.com> wrote:
>> I was getting the error:
>>
>> map size mismatch; abort
>>
>> when running the daemon-conf tests.
>>
>> I traced it to two places in which we were passing a parameter as a
>> number of bits to numa_node_to_cpus which expects a number of bytes
>> (see numa_node_to_cpus_compat in numa.h in the numactl source).
>>
>> The attached patch fixes the problem.
>>
>> Dave
>> diff --git a/src/qemu_conf.c b/src/qemu_conf.c
>> index e6c378f..7019c14 100644
>> --- a/src/qemu_conf.c
>> +++ b/src/qemu_conf.c
>> @@ -324,7 +324,7 @@ qemudCapsInitNUMA(virCapsPtr caps)
>>
>>      for (n = 0 ; n <= numa_max_node() ; n++) {
>>
>> -        if (numa_node_to_cpus(n, mask, MAX_CPUS_MASK_LEN) < 0)
>> +        if (numa_node_to_cpus(n, mask, MAX_CPUS_MASK_LEN / 8) < 0)
> 
> Hi Dave,
> 
> [Sorry I didn't catch this earlier. ]
> 
> With that patch, I am now seeing that out of memory message in
> "make check" test failures, e.g.,
> 
>     testing with corrupted config: min_workers
>     --- expected-err 2008-12-12 15:30:20.000000000 +0100
>     +++ err 2008-12-12 15:30:20.000000000 +0100
>     @@ -1,2 +1,5 @@
>     +map size mismatch; abort
>     +: Success
>     +umlStartup: out of memory
>      remoteReadConfigFile: in23.conf: min_workers: invalid type: got string; expected long
> 
> Since we're using NUMA_VERSION1_COMPATIBILITY mode (anyone know why?)
> 
>   #if HAVE_NUMACTL
>   #define NUMA_VERSION1_COMPATIBILITY 1
>   #include <numa.h>
>   #endif
> 
> numa_node_to_cpus expands to a call to numa_node_to_cpus_compat,
> which is defined in numa.h:
> 
>     static inline int numa_node_to_cpus_compat(int node, unsigned long *buffer,
>                                                             int buffer_len)
>     {
>             struct bitmask tmp;
> 
>             tmp.maskp = (unsigned long *)buffer;
>             tmp.size = buffer_len * 8;
>             return numa_node_to_cpus(node, &tmp);
>     }
> 
> 
> So the question is, what units to use in "tmp.size".
> To answer that, I looked above, also in numa.h at this:
> 
>     struct bitmask {
>             unsigned long size; /* number of bits in the map */
>             unsigned long *maskp;
>     };
> 
> And since MAX_CPUS_MASK_LEN is a buffer length, in bytes,
> 
>     from qemu_conf.c:
>     qemudCapsInitNUMA(virCapsPtr caps)
>     {
>     ...
>         if (VIR_ALLOC_N(mask, MAX_CPUS_MASK_LEN) < 0)
>             goto cleanup;
> 
> and tmp.size is measured in bits, we have to continue to use that
> MAX_CPUS_MASK_LEN parameter, and not divide it by 8:
> 
> So I propose this change:
> (with it, "make check" does pass once again)
> 
> From ceae19496cd60bd9a7b3527046b42e9f8460ae82 Mon Sep 17 00:00:00 2001
> From: Jim Meyering <meyering at redhat.com>
> Date: Fri, 12 Dec 2008 16:22:34 +0100
> Subject: [PATCH] Revert "fix bits/bytes memory request mismatches"
> 
> * src/qemu_conf.c (qemudCapsInitNUMA): Do pass MAX_CPUS_MASK_LEN
> as buffer length in call to numa_node_to_cpus.
> * src/uml_conf.c (umlCapsInitNUMA): Likewise.
> ---
>  src/qemu_conf.c |    2 +-
>  src/uml_conf.c  |    2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/qemu_conf.c b/src/qemu_conf.c
> index 59171e7..206fb0b 100644
> --- a/src/qemu_conf.c
> +++ b/src/qemu_conf.c
> @@ -324,7 +324,7 @@ qemudCapsInitNUMA(virCapsPtr caps)
> 
>      for (n = 0 ; n <= numa_max_node() ; n++) {
> 
> -        if (numa_node_to_cpus(n, mask, MAX_CPUS_MASK_LEN / 8) < 0)
> +        if (numa_node_to_cpus(n, mask, MAX_CPUS_MASK_LEN) < 0)
>              goto cleanup;
> 
>          for (ncpus = 0, i = 0 ; i < MAX_CPUS ; i++)
> diff --git a/src/uml_conf.c b/src/uml_conf.c
> index 3659c6b..7eb630d 100644
> --- a/src/uml_conf.c
> +++ b/src/uml_conf.c
> @@ -80,7 +80,7 @@ umlCapsInitNUMA(virCapsPtr caps)
> 
>      for (n = 0 ; n <= numa_max_node() ; n++) {
> 
> -        if (numa_node_to_cpus(n, mask, MAX_CPUS_MASK_LEN / 8) < 0)
> +        if (numa_node_to_cpus(n, mask, MAX_CPUS_MASK_LEN) < 0)
>              goto cleanup;
> 
>          for (ncpus = 0, i = 0 ; i < MAX_CPUS ; i++)
> --
> 1.6.0.4.1044.g77718

With the patch reverted, I am back to having the errors.  I'll track it 
down.  I must have a different numa version from you guys.

Dave