[libvirt] [PATCH] Add NUMA memory information to virsh capabilities output.

[Eric Blake]

On 03/07/2013 09:03 AM, Dusty Mabe wrote:
> ---

The commit message should mention the XML changes being made.  I'm
modifying it to capture this snippet of your testcase:

    <topology>
      <cells num='2'>
        <cell id='0'>
          <memory unit='KiB'>12572412</memory>
          <cpus num='12'>
          ...

>  docs/schemas/capability.rng               | 10 ++++
>  src/conf/capabilities.c                   |  8 +++
>  src/conf/capabilities.h                   |  2 +
>  src/nodeinfo.c                            | 58 +++++++++++++++++++-
>  src/test/test_driver.c                    |  2 +-
>  src/xen/xend_internal.c                   |  2 +-
>  tests/capabilityschemadata/caps-test3.xml | 88 +++++++++++++++++++++++++++++++
>  7 files changed, 167 insertions(+), 3 deletions(-)
>  create mode 100644 tests/capabilityschemadata/caps-test3.xml
> 
>  
> +  <define name='memory'>
> +    <element name='memory'>
> +        <ref name='scaledInteger'/>

Indentation is a bit off (not that it affects correct behavior).

> @@ -712,6 +714,12 @@ virCapabilitiesFormatNUMATopology(virBufferPtr xml,
>      virBufferAsprintf(xml, "      <cells num='%zu'>\n", ncells);
>      for (i = 0; i < ncells; i++) {
>          virBufferAsprintf(xml, "        <cell id='%d'>\n", cells[i]->num);
> +
> +        /* Print out the numacell memory total if it is available */
> +        if (cells[i]->mem)
> +        virBufferAsprintf(xml, "          <memory unit='KiB'>%llu</memory>\n",

Indentation is off.

> +++ b/src/nodeinfo.c
> @@ -102,6 +102,7 @@ static int linuxNodeGetMemoryStats(FILE *meminfo,
>                                     int cellNum,
>                                     virNodeMemoryStatsPtr params,
>                                     int *nparams);
> +static unsigned long long nodeGetCellMemory(int cell);

Generally, a forward declaration of a static function indicates that the
file is not topologically sorted correctly.  But given that the two
implementations are inside #ifdef, and you were copying from other
examples, it's not worth changing.

> @@ -1562,6 +1564,11 @@ nodeCapsInitNUMA(virCapsPtr caps)
>              continue;
>          }
>  
> +        /* Detect the amount of memory in the numa cell */
> +        memory = nodeGetCellMemory(n);
> +        if (memory == 0)
> +            goto cleanup;

Why give up on the rest of useful information?  This should only go to
cleanup on failure, not on lack of information, so that the cpu
capabilities aren't lost (if there IS a kernel where memory information
cannot be obtained, we don't want to regress and output less information
than before).  The alternative is to have nodeGetCellMemory take an
unsigned long long * argument, and have an int return (0 for success,
including unknown; -1 for complete failure which warrants ditching any
capability return); but I thought that was overkill since we already
special-cased the xml output to skip memory if it was 0.

ACK once this is squashed in, so I pushed it.  Thanks again for the
patch, and for persisting with addressing our feedback, even if it took
a while.

diff --git i/docs/schemas/capability.rng w/docs/schemas/capability.rng
index b479070..106ca73 100644
--- i/docs/schemas/capability.rng
+++ w/docs/schemas/capability.rng
@@ -195,7 +195,7 @@

   <define name='memory'>
     <element name='memory'>
-        <ref name='scaledInteger'/>
+      <ref name='scaledInteger'/>
     </element>
   </define>

diff --git i/src/conf/capabilities.c w/src/conf/capabilities.c
index 9824f0c..d53d5a3 100644
--- i/src/conf/capabilities.c
+++ w/src/conf/capabilities.c
@@ -1,7 +1,7 @@
 /*
  * capabilities.c: hypervisor capabilities
  *
- * Copyright (C) 2006-2008, 2010-2011 Red Hat, Inc.
+ * Copyright (C) 2006-2008, 2010-2011, 2013 Red Hat, Inc.
  * Copyright (C) 2006-2008 Daniel P. Berrange
  *
  * This library is free software; you can redistribute it and/or
@@ -717,8 +717,9 @@ virCapabilitiesFormatNUMATopology(virBufferPtr xml,

         /* Print out the numacell memory total if it is available */
         if (cells[i]->mem)
-        virBufferAsprintf(xml, "          <memory
unit='KiB'>%llu</memory>\n",
-                          cells[i]->mem);
+            virBufferAsprintf(xml,
+                              "          <memory
unit='KiB'>%llu</memory>\n",
+                              cells[i]->mem);

         virBufferAsprintf(xml, "          <cpus num='%d'>\n",
cells[i]->ncpus);
         for (j = 0; j < cells[i]->ncpus; j++) {
diff --git i/src/nodeinfo.c w/src/nodeinfo.c
index a75f73f..b80e389 100644
--- i/src/nodeinfo.c
+++ w/src/nodeinfo.c
@@ -1,7 +1,7 @@
 /*
  * nodeinfo.c: Helper routines for OS specific node information
  *
- * Copyright (C) 2006-2008, 2010-2012 Red Hat, Inc.
+ * Copyright (C) 2006-2008, 2010-2013 Red Hat, Inc.
  * Copyright (C) 2006 Daniel P. Berrange
  *
  * This library is free software; you can redistribute it and/or
@@ -1566,8 +1566,6 @@ nodeCapsInitNUMA(virCapsPtr caps)

         /* Detect the amount of memory in the numa cell */
         memory = nodeGetCellMemory(n);
-        if (memory == 0)
-            goto cleanup;

         for (ncpus = 0, i = 0 ; i < max_n_cpus ; i++)
             if (MASK_CPU_ISSET(mask, i))
@@ -1680,7 +1678,7 @@ cleanup:
  * the amount of total memory in bytes. It is then converted to
  * KiB and returned.
  *
- * Returns 0 on failure, amount of memory in KiB on success.
+ * Returns 0 if unavailable, amount of memory in KiB on success.
  */
 static unsigned long long nodeGetCellMemory(int cell)
 {


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 621 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20130308/178481c4/attachment-0001.sig>