[libvirt] [PATCH 3/3] qemu: Generate -numa command line option

[Bharata B Rao]

On Mon, Nov 07, 2011 at 11:47:10AM -0700, Eric Blake wrote:
> On 11/06/2011 06:59 AM, Bharata B Rao wrote:
> >qemu: Generate -numa option
> >
> >From: Bharata B Rao<bharata at linux.vnet.ibm.com>
> >
> >Add routines to generate -numa QEMU command line option based on
> ><numa>  ...</numa>  XML specifications.
> >
> >Signed-off-by: Bharata B Rao<bharata at linux.vnet.ibm.com>
> 
> >
> >+static int
> >+qemuBuildNumaCPUArgStr(char *cpumask, virBufferPtr buf)
> >+{
> >+    int i, first, last;
> >+    int cpuSet = 0;
> >+
> 
> What happens if cpumask is all 0?
> 
> >+    for (i = 0; i<  VIR_DOMAIN_CPUMASK_LEN; i++) {
> >+        if (cpumask[i]) {
> 
> This if branch is skipped,
> 
> >+            if (cpuSet)
> >+                last = i;
> >+            else {
> >+                first = last = i;
> >+                cpuSet = 1;
> >+            }
> >+        } else {
> >+            if (!cpuSet)
> >+                continue;
> 
> so this branch always continues,
> 
> >+            if (first == last)
> >+                virBufferAsprintf(buf, "%d,", first);
> >+            else
> >+                virBufferAsprintf(buf, "%d-%d,", first, last);
> >+            cpuSet = 0;
> >+	}
> >+    }
> >+
> >+    if (cpuSet) {
> 
> and this if is skipped,
> 
> >+        if (first == last)
> >+            virBufferAsprintf(buf, "%d,", first);
> >+        else
> >+            virBufferAsprintf(buf, "%d-%d,", first, last);
> >+    }
> >+
> >+    /* Remove the trailing comma */
> >+    return virBufferTruncate(buf, 1);
> 
> 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?

Good catch. I am now ensuring that this doesn't get a mask with no cpus.

> 
> >+}
> >+
> >+static int
> >+qemuBuildNumaArgStr(const virDomainDefPtr def, virCommandPtr cmd)
> >+{
> >+    int i;
> >+    char *node;
> >+    virBuffer buf = VIR_BUFFER_INITIALIZER;
> >+
> >+    for (i = 0; i<  def->cpu->ncells; i++) {
> >+        virCommandAddArg(cmd, "-numa");
> >+        virBufferAsprintf(&buf, "%s", "node");
> 
> More efficient as virBufferAddLit(&buf, "node"), or...

Right.

> 
> >+        virBufferAsprintf(&buf, ",nodeid=%d", def->cpu->cells[i].cellid);
> 
> merge these two into a single:
> 
> virBufferAsprintf(&buf, "node,nodeid=%d", ...);
> 
> >+        virBufferAsprintf(&buf, ",cpus=");
> 
> Again, with no % in the format string, it is more efficient to use
> virBufferAddLit.
> 
> >+
> >+        if (qemuBuildNumaCPUArgStr(def->cpu->cells[i].cpumask,&buf))
> 
> Generally, we prefer an explicit < 0 comparison when checking for failure.

Ok as you prefer :)

> 
> >+            goto error;
> >+
> >+        virBufferAsprintf(&buf, ",mems=%d", def->cpu->cells[i].mem);
> >+
> 
> 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

This is what I mentioned in my last iteration. But since you hinted
at extending virBuffer, I went on that path. Another motivation was to
keep qemuBuildNumaArgStr() generic enough so that it gives out comma/dash
separate CPU string without the ending comma in case this functionality
is needed elsewhere in libvirt. But I guess I will stick with not appending
comma to "memory" in which case this extension to virBuffer isn't needed.

> (that said, I still think
> virBufferTruncate will be a useful addition in other contexts).

I think we should add virBufferTruncate only when we get any user for that.
Hence I am not including virBufferTruncate in v3 patchset.

> 
> >+        if (virBufferError(&buf))
> >+            goto error;
> >+
> >+        node = virBufferContentAndReset(&buf);
> >+        virCommandAddArg(cmd, node);
> >+        VIR_FREE(node);
> 
> It's more efficient to replace these five lines with one:
> 
> virCommandAddArgBuffer(cmd, &buf);

I can't because this is in a loop and I want to break out from
the loop in case of buffer error. virCommandAddArgBuffer doesn't allow that.
But I did replace the last 3 lines with virCommandAddArgBuffer :)

> 
> >@@ -3414,6 +3482,9 @@ qemuBuildCommandLine(virConnectPtr conn,
> >      virCommandAddArg(cmd, smp);
> >      VIR_FREE(smp);
> >
> >+    if (def->cpu&&  def->cpu->ncells&&  qemuBuildNumaArgStr(def, cmd))
> 
> Again, explicit < 0 check when looking for errors.

Sure.

> 
> >+<topology sockets="2" cores="4" threads="2"/>
> >+<numa>
> >+<cell cpus="0-7" mems="109550"/>
> >+<cell cpus="8-15" mems="109550"/>
> 
> Of course, this will need tweaking to match any XML changes made in
> 1/3, but thanks for adding test cases!

Tweaked the testcases.

BTW the testcases were useful to me as they uncovered 2 bugs in my code!

> 
> 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.

Will ensure [PATCH v3] for the next iteration.

Thanks. v3 on its way.

Regards,
Bharata.