XML definitions for guest NUMA and parsing routines. From: Bharata B Rao <bharata(a)linux.vnet.ibm.com&gt; This patch adds XML definitions for guest NUMA specification and contains routines to parse the same. The guest NUMA specification looks like this: <cpu> ... <topology sockets='2' cores='4' threads='2'/> <numa> <cell cpus='0-7' mems='512000'/> <cell cpus='8-15' mems='512000'/> </numa> ... </cpu> Signed-off-by: Bharata B Rao <bharata(a)linux.vnet.ibm.com&gt; --- docs/formatdomain.html.in | 32 +++++++++++++++++++++ docs/schemas/domaincommon.rng | 32 +++++++++++++++++++++ src/conf/cpu_conf.c | 62 +++++++++++++++++++++++++++++++++++++++++ src/conf/cpu_conf.h | 12 ++++++++ src/conf/domain_conf.c | 7 +++++ 5 files changed, 145 insertions(+), 0 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index c3e7752..8d070ca 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -628,6 +628,38 @@ </dd> </dl> + <p> + Guest NUMA topology can be specifed using <code>numa</code> element. + <span class="since">Since X.X.X</span> + </p> + +<pre> + ... + &lt;cpu&gt; + ... + &lt;numa&gt; + &lt;cell cpus='0-3' mems='512000'/&gt; + &lt;cell cpus='4-7' mems='512000'/&gt; + &lt;/numa&gt; + ... + &lt;/cpu&gt; + ...</pre> + + <p> + Each <code>cell</code> element specifies a NUMA cell or a NUMA node. + <code>cpus</code> specifies the CPU or range of CPUs that are part of + the node. <code>mems</code> specifies the node memory in kilobytes + (i.e. blocks of 1024 bytes). Each cell or node is assigned cellid + or nodeid in the increasing order starting from 0. + </p> + + <p> + This guest NUMA specification translates to <code>-numa</code> command + line option for QEMU/KVM. For the above example, the following QEMU + command line option is generated: + <code>-numa node,nodeid=0,cpus=0-3,mems=512000 -numa node,nodeid=1,cpus=4-7,mems=512000</code> + </p> + <h3><a name="elementsLifecycle">Lifecycle control</a></h3> <p> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index b6f858e..b09060a 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2311,6 +2311,9 @@ <zeroOrMore> <ref name="cpuFeature"/> </zeroOrMore> + <optional> + <ref name="cpuNuma"/> + </optional> </interleave> </group> </choice> @@ -2371,6 +2374,25 @@ </element> </define> + <define name="cpuNuma"> + <element name="numa"> + <oneOrMore> + <ref name="numaCell"/> + </oneOrMore> + </element> + </define> + + <define name="numaCell"> + <element name="cell"> + <attribute name="cpus"> + <ref name="Cellcpus"/> + </attribute> + <attribute name="mems"> + <ref name="Cellmems"/> + </attribute> + </element> + </define> + <!-- System information specification: Placeholder for system specific informations likes the ones @@ -2745,4 +2767,14 @@ <param name="pattern">[a-zA-Z0-9_\.:]+</param> </data> </define> + <define name="Cellcpus"> + <data type="string"> + <param name="pattern">([0-9]+(-[0-9]+)?|\^[0-9]+)(,([0-9]+(-[0-9]+)?|\^[0-9]+))*</param> + </data> + </define> + <define name="Cellmems"> + <data type="unsignedInt"> + <param name="pattern">[0-9]+</param> + </data> + </define> </grammar> diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c index 41e997e..e55897f 100644 --- a/src/conf/cpu_conf.c +++ b/src/conf/cpu_conf.c @@ -28,6 +28,7 @@ #include "util.h" #include "buf.h" #include "cpu_conf.h" +#include "domain_conf.h" #define VIR_FROM_THIS VIR_FROM_CPU @@ -67,6 +68,10 @@ virCPUDefFree(virCPUDefPtr def) VIR_FREE(def->features[i].name); VIR_FREE(def->features); + for (i = 0 ; i < def->ncells ; i++) + VIR_FREE(def->cells[i].cpumask); + VIR_FREE(def->cells); + VIR_FREE(def); } @@ -109,6 +114,19 @@ no_memory: return NULL; } +static int +virCPUDefNumaCPUs(virCPUDefPtr def) +{ + int i, j, ncpus = 0; + + for (i = 0; i < def->ncells; i++) { + for (j = 0; j < VIR_DOMAIN_CPUMASK_LEN; j++) { + if (def->cells[i].cpumask[j]) + ncpus++; + } + } + return ncpus; +} virCPUDefPtr virCPUDefParseXML(const xmlNodePtr node, @@ -289,6 +307,50 @@ virCPUDefParseXML(const xmlNodePtr node, def->features[i].policy = policy; } + if (virXPathNode("./numa[1]", ctxt)) { + VIR_FREE(nodes); + n = virXPathNodeSet("./numa[1]/cell", ctxt, &nodes); + if (n < 0 || n == 0) { + virCPUReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("NUMA topology defined without NUMA cells")); + goto error; + } + + if (VIR_RESIZE_N(def->cells, def->ncells_max, + def->ncells, n) < 0) + goto no_memory; + + def->ncells = n; + + for (i = 0 ; i < n ; i++) { + char *cpus; + int cpumasklen = VIR_DOMAIN_CPUMASK_LEN; + unsigned long ul; + int ret; + + def->cells[i].cellid = i; + cpus = virXMLPropString(nodes[i], "cpus"); + + if (VIR_ALLOC_N(def->cells[i].cpumask, cpumasklen) < 0) + goto no_memory; + + if (virDomainCpuSetParse((const char **)&cpus, + 0, def->cells[i].cpumask, + cpumasklen) < 0) + goto error; + + ret = virXPathULong("string(./numa[1]/cell/@mems)", + ctxt, &ul); + if (ret < 0) { + virCPUReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("Missing 'mems' attribute in NUMA topology")); + goto error; + } + def->cells[i].mem = (unsigned int) ul; + } + def->cells_cpus = virCPUDefNumaCPUs(def); + } + cleanup: VIR_FREE(nodes); diff --git a/src/conf/cpu_conf.h b/src/conf/cpu_conf.h index 4406cba..bf4270a 100644 --- a/src/conf/cpu_conf.h +++ b/src/conf/cpu_conf.h @@ -67,6 +67,14 @@ struct _virCPUFeatureDef { int policy; /* enum virCPUFeaturePolicy */ }; +typedef struct _virCellDef virCellDef; +typedef virCellDef *virCellDefPtr; +struct _virCellDef { + int cellid; + char *cpumask; /* CPUs that are part of this node */ + unsigned int mem; /* Node memory */ +}; + typedef struct _virCPUDef virCPUDef; typedef virCPUDef *virCPUDefPtr; struct _virCPUDef { @@ -81,6 +89,10 @@ struct _virCPUDef { size_t nfeatures; size_t nfeatures_max; virCPUFeatureDefPtr features; + size_t ncells; + size_t ncells_max; + virCellDefPtr cells; + unsigned int cells_cpus; }; diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6e2d421..02fc1e7 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7511,6 +7511,13 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, if (def->cpu == NULL) goto error; + + if (def->cpu->cells_cpus > def->maxvcpus) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Number of CPUs in <numa> exceeds the" + " <vcpu> count")); + goto error; + } } if ((node = virXPathNode("./sysinfo[1]", ctxt)) != NULL) {

On Mon, Nov 07, 2011 at 11:35:27AM -0700, Eric Blake wrote: Ok, changed to 0.9.8 in v3. Changed to memory. Ok, removed this paragraph. Ok, changed. Actually qemu allows both cpus and mem to be optional. But if we allow that, we will have specifications like this: <numa> <cell /> <cell /> </numa> That looks ugly. Either both of them should allowed to be optional or none of them, I am going for the latter specification. IOW lets make both of them mandatory in a numa cell. Fixed. Done. Yes provided we start storing CPUs in bitmasks rather than char arrays. But I saw other parts of the code (like cpuset) storing CPUs in array and hence used the same for numa too. In any case I got rid of this since we can arrive at this information from other means. Yes. I first accounted for the error case and later added a check for zero cells in <numa>. Fixed this. This was buggy. Fixed now. As explained above, I made both of them mandatory now. Done. Thanks for pointing this out. Added code for this now. Regards, Bharata.

On 11/06/2011 06:58 AM, Bharata B Rao wrote: What good is returning an error, given that callers already have to use virBufferError for detecting other errors, and given that you aren't marking the function as ATTRIBUTE_RETURN_CHECK to require callers to respect that error? One of the points of the virBuffer API is that most functions can return void (for example, see virBufferAdjustIndent), because the user doesn't care about error checking until the end. I tend to write the cast as (int), not (signed), if a cast is even needed. You don't catch all possible overflows, though - if buf->use is 2, and len is 4294967295 (aka (unsigned)-1), then you've proceeded to expand(!) buf->use to 3, skipping an uninitialized byte. A better filter would be: if (len > buf->use) This looks correct, once you filter out invalid len first. That gives no change in the expected output. I'd almost rather see: virBufferAsprintf(buf, "%d", 123); virBufferTruncate(buf, 1); as well as an update to the expected output to see output ending in "12". You're on the right track, but I don't think it is worth applying this until after 0.9.7 is released. -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

On Mon, Nov 07, 2011 at 11:12:07AM -0700, Eric Blake wrote: Ok, point taken. Agreed. The idea was to test with minimal change and hence resorted to this. In any case, I am dropping this patch for the reason explained in another thread. Regards, Bharata.

On 11/06/2011 06:59 AM, Bharata B Rao wrote: What happens if cpumask is all 0? This if branch is skipped, so this branch always continues, and this if is skipped, meaning that nothing was appended to buf, and you are now stripping unknown text, rather than a comma you just added. Do we need a sanity check to ensure that the cpumask specifies at least one cpu? And if so, would that mask be better here, or up front at the xml parsing time? More efficient as virBufferAddLit(&buf, "node"), or... merge these two into a single: virBufferAsprintf(&buf, "node,nodeid=%d", ...); Again, with no % in the format string, it is more efficient to use virBufferAddLit. Generally, we prefer an explicit < 0 comparison when checking for failure. Why do we need to bother with stripping a trailing comma in qemuBuildNumaCPUArgStr, if we will just be adding a comma back again here as the very next statement? You could skip all the hassle of adding virBufferTruncate by just transferring the comma out of this statement and into qemuBuildNumaCPUArgStr (that said, I still think virBufferTruncate will be a useful addition in other contexts). It's more efficient to replace these five lines with one: virCommandAddArgBuffer(cmd, &buf); Again, explicit < 0 check when looking for errors. Of course, this will need tweaking to match any XML changes made in 1/3, but thanks for adding test cases! Overall, I think we'll need a v3 (you may want to use git send-email --subject-prefix=PATCHv3; it wasn't very clear from the subject line that this was already a v2 series), but I like where it's heading. -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org