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@xxxxxxxxxxxxxxxxxx>
> >
> >Add routines to generate -numa QEMU command line option based on
> ><numa> ...</numa> XML specifications.
> >
> >Signed-off-by: Bharata B Rao<bharata@xxxxxxxxxxxxxxxxxx>
>
> >
> >+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.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list