Re: [libvirt] [PATCH 3/3] qemu: Generate -numa command line option
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 Raobhar...@linux.vnet.ibm.com
Add routines to generate -numa QEMU command line option based on
numa .../numa XML specifications.
Signed-off-by: Bharata B Raobhar...@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
Re: [libvirt] [PATCH 3/3] qemu: Generate -numa command line option
On 11/06/2011 06:59 AM, Bharata B Rao wrote:
qemu: Generate -numa option
From: Bharata B Raobhar...@linux.vnet.ibm.com
Add routines to generate -numa QEMU command line option based on
numa .../numa XML specifications.
Signed-off-by: Bharata B Raobhar...@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?
+}
+
+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...
+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.
+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 (that said, I still think
virBufferTruncate will be a useful addition in other contexts).
+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);
@@ -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.
+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!
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 ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 3/3] qemu: Generate -numa command line option
qemu: Generate -numa option
From: Bharata B Rao bhar...@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 bhar...@linux.vnet.ibm.com
---
src/qemu/qemu_command.c| 71
tests/qemuxml2argvdata/qemuxml2argv-cpu-numa1.args |5 +
tests/qemuxml2argvdata/qemuxml2argv-cpu-numa1.xml | 25 +++
tests/qemuxml2argvdata/qemuxml2argv-cpu-numa2.args |6 ++
tests/qemuxml2argvdata/qemuxml2argv-cpu-numa2.xml | 25 +++
tests/qemuxml2argvtest.c |2 +
6 files changed, 134 insertions(+), 0 deletions(-)
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-cpu-numa1.args
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-cpu-numa1.xml
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-cpu-numa2.args
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-cpu-numa2.xml
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index dc92fa3..b042e34 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3251,6 +3251,74 @@ qemuBuildSmpArgStr(const virDomainDefPtr def,
return virBufferContentAndReset(buf);
}
+static int
+qemuBuildNumaCPUArgStr(char *cpumask, virBufferPtr buf)
+{
+int i, first, last;
+int cpuSet = 0;
+
+for (i = 0; i VIR_DOMAIN_CPUMASK_LEN; i++) {
+if (cpumask[i]) {
+if (cpuSet)
+last = i;
+else {
+first = last = i;
+cpuSet = 1;
+}
+} else {
+if (!cpuSet)
+continue;
+if (first == last)
+virBufferAsprintf(buf, %d,, first);
+else
+virBufferAsprintf(buf, %d-%d,, first, last);
+cpuSet = 0;
+ }
+}
+
+if (cpuSet) {
+if (first == last)
+virBufferAsprintf(buf, %d,, first);
+else
+virBufferAsprintf(buf, %d-%d,, first, last);
+}
+
+/* Remove the trailing comma */
+return virBufferTruncate(buf, 1);
+}
+
+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);
+virBufferAsprintf(buf, ,nodeid=%d, def-cpu-cells[i].cellid);
+virBufferAsprintf(buf, ,cpus=);
+
+if (qemuBuildNumaCPUArgStr(def-cpu-cells[i].cpumask, buf))
+goto error;
+
+virBufferAsprintf(buf, ,mems=%d, def-cpu-cells[i].mem);
+
+if (virBufferError(buf))
+goto error;
+
+node = virBufferContentAndReset(buf);
+virCommandAddArg(cmd, node);
+VIR_FREE(node);
+}
+return 0;
+
+error:
+virBufferFreeAndReset(buf);
+virReportOOMError();
+return -1;
+}
/*
* Constructs a argv suitable for launching qemu with config defined
@@ -3414,6 +3482,9 @@ qemuBuildCommandLine(virConnectPtr conn,
virCommandAddArg(cmd, smp);
VIR_FREE(smp);
+if (def-cpu def-cpu-ncells qemuBuildNumaArgStr(def, cmd))
+goto error;
+
if (qemuCapsGet(qemuCaps, QEMU_CAPS_NAME)) {
virCommandAddArg(cmd, -name);
if (driver-setProcessName
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-cpu-numa1.args
b/tests/qemuxml2argvdata/qemuxml2argv-cpu-numa1.args
new file mode 100644
index 000..e8fd277
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-cpu-numa1.args
@@ -0,0 +1,5 @@
+LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test ./qemu.sh -S -M pc \
+-m 214 -smp 16 -numa node,nodeid=0,cpus=0-7,mems=109550 \
+-numa node,nodeid=1,cpus=8-15,mems=109550 -nographic -monitor \
+unix:/tmp/test-monitor,server,nowait -no-acpi -boot n -net none -serial none \
+-parallel none -usb
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-cpu-numa1.xml
b/tests/qemuxml2argvdata/qemuxml2argv-cpu-numa1.xml
new file mode 100644
index 000..07fe891
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-cpu-numa1.xml
@@ -0,0 +1,25 @@
+domain type='qemu'
+ nameQEMUGuest1/name
+ uuidc7a5fdbd-edaf-9455-926a-d65c16db1809/uuid
+ memory219100/memory
+ currentMemory219100/currentMemory
+ vcpu16/vcpu
+ os
+type arch='x86_64' machine='pc'hvm/type
+boot dev='network'/
+ /os
+ cpu
+topology sockets=2 cores=4 threads=2/
+numa
+ cell cpus=0-7 mems=109550/
+ cell cpus=8-15 mems=109550/
+/numa
+ /cpu
+ clock offset='utc'/
+ on_poweroffdestroy/on_poweroff
+ on_rebootrestart/on_reboot
+ on_crashdestroy/on_crash
+ devices
+ emulator/./qemu.sh/emulator
+ /devices
+/domain
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-cpu-numa2.args
b/tests/qemuxml2argvdata/qemuxml2argv-cpu-numa2.args
new file mode 100644
index 000..bc96cef
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-cpu-numa2.args
@@ -0,0
