[PATCH] virnuma: Don't work around numa_node_to_cpus() for non-existent nodes

[Daniel P. Berrangé]

On Fri, Aug 21, 2020 at 05:12:08PM +0200, Michal Privoznik wrote:
> In a very distant past, we came around machines that has not
> continuous node IDs. This made us error out when constructing
> capabilities XML. We resolved it by utilizing strange behaviour
> of numa_node_to_cpus() in which it returned a mask with all bits
> set for a non-existent node. However, this is not the only case
> when it returns all ones mask - if the node exists and has enough
> CPUs to fill the mask up (e.g. 128 CPUs).
> 
> The fix consists of using nodemask_isset(&numa_all_nodes, ..)
> prior to calling numa_node_to_cpus() to determine if the node
> exists.
> 
> Fixes: 628c93574758abb59e71160042524d321a33543f
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1860231
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/util/virnuma.c | 18 +++++-------------
>  1 file changed, 5 insertions(+), 13 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange at redhat.com>

> 
> diff --git a/src/util/virnuma.c b/src/util/virnuma.c
> index eeca438f25..75d5628cff 100644
> --- a/src/util/virnuma.c
> +++ b/src/util/virnuma.c
> @@ -256,31 +256,23 @@ virNumaGetNodeCPUs(int node,
>      int mask_n_bytes = max_n_cpus / 8;
>      size_t i;
>      g_autofree unsigned long *mask = NULL;
> -    g_autofree unsigned long *allonesmask = NULL;
>      g_autoptr(virBitmap) cpumap = NULL;
>  
>      *cpus = NULL;
>  
> +    if (!nodemask_isset(&numa_all_nodes, node)) {
> +        VIR_DEBUG("NUMA topology for cell %d is not available, ignoring", node);
> +        return -2;
> +    }
> +
>      if (VIR_ALLOC_N(mask, mask_n_bytes / sizeof(*mask)) < 0)
>          return -1;
>  
> -    if (VIR_ALLOC_N(allonesmask, mask_n_bytes / sizeof(*mask)) < 0)
> -        return -1;
> -
> -    memset(allonesmask, 0xff, mask_n_bytes);
> -
> -    /* The first time this returns -1, ENOENT if node doesn't exist... */
>      if (numa_node_to_cpus(node, mask, mask_n_bytes) < 0) {
>          VIR_WARN("NUMA topology for cell %d is not available, ignoring", node);
>          return -2;
>      }
>  
> -    /* second, third... times it returns an all-1's mask */
> -    if (memcmp(mask, allonesmask, mask_n_bytes) == 0) {
> -        VIR_DEBUG("NUMA topology for cell %d is invalid, ignoring", node);
> -        return -2;
> -    }
> -
>      if (!(cpumap = virBitmapNew(max_n_cpus)))
>          return -1;

Nice, that's simpler than I expected the fix would be.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|