[libvirt] [PATCH v2] nodeinfo: fix to parse present cpus rather than possible cpus

[John Ferlan]

On 06/16/2015 05:38 AM, Kothapally Madhu Pavan wrote:
> Currently we are parsing all the possible cpus to get the
> nodeinfo. This fix will perform a check for present cpus
> before parsing.
> 
> Signed-off-by: Kothapally Madhu Pavan <kmp at linux.vnet.ibm.com>
> ---
>  src/nodeinfo.c |   13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/src/nodeinfo.c b/src/nodeinfo.c
> index 2fafe2d..0134aba 100644
> --- a/src/nodeinfo.c
> +++ b/src/nodeinfo.c
> @@ -57,6 +57,7 @@
>  #define VIR_FROM_THIS VIR_FROM_NONE
>  
>  VIR_LOG_INIT("nodeinfo");
> +virBitmapPtr nodeGetPresentCPUBitmap(void);

^^^

Didn't notice this the first time - if you had #include "nodeinfo.h",
then this is unnecessary

>  
>  #if defined(__FreeBSD__) || defined(__APPLE__)
>  static int
> @@ -418,6 +419,7 @@ virNodeParseNode(const char *node,
>      int processors = 0;
>      DIR *cpudir = NULL;
>      struct dirent *cpudirent = NULL;
> +    virBitmapPtr present_cpumap = NULL;
>      int sock_max = 0;
>      cpu_set_t sock_map;
>      int sock;
> @@ -438,12 +440,18 @@ virNodeParseNode(const char *node,
>          goto cleanup;
>      }
>  
> +    present_cpumap = nodeGetPresentCPUBitmap();
> +
>      /* enumerate sockets in the node */
>      CPU_ZERO(&sock_map);
>      while ((direrr = virDirRead(cpudir, &cpudirent, node)) > 0) {
>          if (sscanf(cpudirent->d_name, "cpu%u", &cpu) != 1)
>              continue;
>  
> +        if (present_cpumap)
> +            if (!(virBitmapIsBitSet(present_cpumap, cpu)))

if (present_cpumap && !(virBitmapIsSet(present_cpumap, cpu)))

> +                continue;
> +
>          if ((online = virNodeGetCpuValue(node, cpu, "online", 1)) < 0)
>              goto cleanup;
>  

Expanding a bit below here one finds

           if (!online)
               continue;

> @@ -477,6 +485,10 @@ virNodeParseNode(const char *node,
>          if (sscanf(cpudirent->d_name, "cpu%u", &cpu) != 1)
>              continue;
>  
> +        if (present_cpumap)
> +            if (!(virBitmapIsBitSet(present_cpumap, cpu)))

if (present_cpumap && !(virBitmapIsSet(present_cpumap, cpu)))

> +                continue;
> +
>          if ((online = virNodeGetCpuValue(node, cpu, "online", 1)) < 0)
>              goto cleanup;

Expanding a bit below here one finds

           if (!online) {
               (*offline)++;
               continue;
           }


What's not clear to me is what you're trying to accomplish by avoiding a
CPU in this second loop since it appears from my reading that there is a
desire to know the offline count - that is the set of CPU's you're
perhaps trying to avoid by non including present CPU's?

Perhaps better rephrased....

The nodeGetPresentCPUBitmap() returns a bitmap of
"/sys/devices/system/cpu/present".  Does the directory structure in
/sys/devices/system/node/nodeN/cpuM" still exist for CPU's that aren't
within the 'present' bitmap?  If so, then wouldn't this second loop need
to be able to mark/count those offline cpu's?

Perhaps would be nice to have a test that could be run before and after
the patch to ensure no "functionality" is lost here.

Obviously my "knowledge" of this cpu/node filesystem is lacking, but
what's being done in this code seems to me to perhaps want to know about
those not present cpu's.

Since both loops continue back to the top once a CPU directory either
doesn't have the "online" file or it contains a 0 in the file, so other
than perhaps a slight optimization in the first loop to not look at the
online file, what is the value gained for this patch?  What problem are
you trying to solve.


John
>  
> @@ -537,6 +549,7 @@ virNodeParseNode(const char *node,
>          ret = -1;
>      }
>      VIR_FREE(core_maps);
> +    virBitmapFree(present_cpumap);
>  
>      return ret;
>  }
> 
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>