1:02 p.m.
Virsh restore/migration fails on a VM which has gone
through a series of memory attach and detach where
the sequence of detach is random(not in LIFO order).
With memory hotplug, the dimms are looked up
using the object-id and so, the restore/migration
fails because the qemu command that gets generated
has memdev and id values which wouldn't exist for
the given slotid.
This patch ensures that the source and destination would
always have same memdev-objectid-slotid values.
Steps to hit the issue:
1. Hot add two memory dimms twice.
virsh attach-device VMName memHotplug.xml --live
virsh attach-device VMName memHotplug.xml --live
2. cat memHotplug.xml
<memory model='dimm' >
<target>
<size unit='MiB'>128</size>
<node>0</node>
</target>
</memory>
3. Now hot unplug the first memory module attached.
virsh detach-device VMName detach.xml --live
4. cat detach.xml
<memory model='dimm'>
<target>
<size unit='MiB'>128</size>
<node>0</node>
</target>
<alias name='dimm0'/>
<address type='dimm' slot='0' base='0x100000000'/>
</memory>
5.Save the VM and then restore. Restore fails..
virsh save VMName ./VMName.vmsav
virsh restore ./VMName.vmsav
Error: Unknown ramblock "memdimm1".
6. Example qemu log without current patch.
dimm,node=0,memdev=memdimm0,id=dimm0,slot=1,addr=4429185024 -object
memory-backend-ram,id=memdimm1,size=134217728 -device
pc-dimm,node=0,memdev=memdimm1,id=dimm1,slot=2,addr=4563402752 -object
memory-backend-ram,id=memdimm2,size=134217728 -device
pc-dimm,node=0,memdev=memdimm2,id=dimm2,slot=3,addr=4697620480 -object
memory-backend-ram,id=memdimm3,size=134217728 -device
pc-dimm,node=0,memdev=memdimm3,id=dimm3,slot=4,addr=4831838208 -object
memory-backend-ram,id=memdimm4,size=134217728 -device
pc-dimm,node=0,memdev=memdimm4,id=dimm4,slot=5,addr=4966055936
7. With the current patch we have kept the dimm name as dimmX where X=slot number.In such
a
case the memdev and id generated are always correct w.r.t the slotid and the virsh
restore/
migrate succeeds.
dimm,node=0,memdev=memdimm1,id=dimm1,slot=1,addr=4429185024 -object
memory-backend-ram,id=memdimm2,size=134217728 -device
pc-dimm,node=0,memdev=memdimm2,id=dimm2,slot=2,addr=4563402752 -object
memory-backend-ram,id=memdimm3,size=134217728 -device
pc-dimm,node=0,memdev=memdimm3,id=dimm3,slot=3,addr=4697620480 -object
memory-backend-ram,id=memdimm4,size=134217728 -device
pc-dimm,node=0,memdev=memdimm4,id=dimm4,slot=4,addr=4831838208 -object
memory-backend-ram,id=memdimm5,size=134217728 -device
pc-dimm,node=0,memdev=memdimm5,id=dimm5,slot=5,addr=4966055936
Nitesh Konkar (2):
qemu_alias:Set dimm alias name to dimmX where X=slot number.
tests/qemuxml2argvdata:Add a testcase
src/qemu/qemu_alias.c | 51 +++++++++---
.../qemuxml2argv-memory-hotplug-dimm-alias.args | 35 ++++++++
.../qemuxml2argv-memory-hotplug-dimm-alias.xml | 94 ++++++++++++++++++++++
tests/qemuxml2argvtest.c | 2 +
4 files changed, 172 insertions(+), 10 deletions(-)
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm-alias.args
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm-alias.xml
--
1.9.3
1:02 p.m.
New subject: [libvirt] [PATCH 1/2] qemu_alias:Set dimm alias name to dimmX where X=slot number.
When we hotplug a memory module, qemu will assign
the smallest slot available. At the time of setting
the alias we find the smallest available slot and
assign alias name as dimmX where X=slot number.
Signed-off-by: Nitesh Konkar <nitkon12(a)linux.vnet.ibm.com>
---
src/qemu/qemu_alias.c | 51 +++++++++++++++++++++++++++++++++++++++++----------
1 file changed, 41 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c
index 0102c96..23ffdfd 100644
--- a/src/qemu/qemu_alias.c
+++ b/src/qemu/qemu_alias.c
@@ -28,6 +28,7 @@
#include "virlog.h"
#include "virstring.h"
#include "network/bridge_driver.h"
+#include "qemu_process.h"
#define QEMU_DRIVE_HOST_PREFIX "drive-"
@@ -331,21 +332,47 @@ qemuAssignDeviceRNGAlias(virDomainDefPtr def,
return 0;
}
+/* Find the smallest available slot. */
+static int
+qemuGetSmallestSlotIdx(virDomainDefPtr def)
+{
+ size_t i, j;
+ int minidx = 0, idx;
+
+ for (i = 0; i < def->nmems; i++) {
+ if ((idx = qemuDomainDeviceAliasIndex(&def->mems[i]->info,
"dimm")) != i) {
+ for (j = 0; j < def->nmems; j++) {
+ if ((idx = qemuDomainDeviceAliasIndex(&def->mems[j]->info,
"dimm")) == i)
+ break;
+ }
+
+ if (j >= def->nmems) {
+ minidx = i;
+ break;
+ }
+ }
+
+ if ((idx = qemuDomainDeviceAliasIndex(&def->mems[i]->info,
"dimm")) >= minidx)
+ minidx = idx + 1;
+ }
+
+ return minidx;
+}
+/* If the slot is not mentioned then find the smallest slot(X) available
+ and assign alias as dimmX. */
int
qemuAssignDeviceMemoryAlias(virDomainDefPtr def,
virDomainMemoryDefPtr mem)
{
- size_t i;
- int maxidx = 0;
- int idx;
+ int minidx = 0;
- for (i = 0; i < def->nmems; i++) {
- if ((idx = qemuDomainDeviceAliasIndex(&def->mems[i]->info,
"dimm")) >= maxidx)
- maxidx = idx + 1;
- }
+ if (mem->info.addr.dimm.base)
+ minidx = mem->info.addr.dimm.slot;
+ else
+ minidx = qemuGetSmallestSlotIdx(def);
- if (virAsprintf(&mem->info.alias, "dimm%d", maxidx) < 0)
+ if (virAsprintf(&mem->info.alias, "dimm%d", minidx) < 0)
return -1;
return 0;
@@ -445,8 +472,12 @@ qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr
qemuCaps)
return -1;
}
for (i = 0; i < def->nmems; i++) {
- if (virAsprintf(&def->mems[i]->info.alias, "dimm%zu", i) <
0)
- return -1;
+ if (!def->mems[i]->info.addr.dimm.base) {
+ if (virAsprintf(&def->mems[i]->info.alias, "dimm%zu", i)
< 0)
+ return -1;
+ } else if (virAsprintf(&def->mems[i]->info.alias, "dimm%d",
def->mems[i]->info.addr.dimm.slot) < 0) {
+ return -1;
+ }
}
return 0;
--
1.9.3
3:53 a.m.
New subject: [libvirt] [PATCH 1/2] qemu_alias:Set dimm alias name to dimmX where X=slot number.
On Sun, Sep 25, 2016 at 23:32:05 +0530, Nitesh Konkar wrote:
When we hotplug a memory module, qemu will assign
the smallest slot available. At the time of setting
the alias we find the smallest available slot and
assign alias name as dimmX where X=slot number.
This commit message doesn't really describe why this is necessary. The
problem with qemu is that the migration stream references the memory
blocks by name (which is supplied by libvirt) rather than by the order
of those. This means that we must keep them stable accross migrations.
With the current code that is assigning aliases for memory backend
objects this won't happen and since qemu is treating the memory object
links differently migration does not work in such case.
This means that the requirement is to keep the slot number consistent
with the alias.
To achieve this the below code is not really necessary as once the VM is
started the slot numbers are populated (and thankfully correctly since
qemu numbers the slots starting from 0).
The code below is based on parsing aliases which is not necessary since
you already have the slot numbers available.
The general idea is to do slot number alocation for the memory modules
beforehand so that we are 100% sure which slot number we will get and
then generate alias based on the slot number. I'm not sure though
whether qemu accepts a partial address information consisting of the
slot number only and I did not get around to check it yet.
Signed-off-by: Nitesh Konkar <nitkon12(a)linux.vnet.ibm.com>
---
src/qemu/qemu_alias.c | 51 +++++++++++++++++++++++++++++++++++++++++----------
1 file changed, 41 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c
index 0102c96..23ffdfd 100644
--- a/src/qemu/qemu_alias.c
+++ b/src/qemu/qemu_alias.c
@@ -28,6 +28,7 @@
#include "virlog.h"
#include "virstring.h"
#include "network/bridge_driver.h"
+#include "qemu_process.h"
Why do you need this? The code below doesn't seem to use it in any way.
#define QEMU_DRIVE_HOST_PREFIX "drive-"
@@ -331,21 +332,47 @@ qemuAssignDeviceRNGAlias(virDomainDefPtr def,
return 0;
}
+/* Find the smallest available slot. */
+static int
+qemuGetSmallestSlotIdx(virDomainDefPtr def)
+{
+ size_t i, j;
Please don't declare multiple variables on a line.
+ int minidx = 0, idx;
Also declaring multiple on same line and initializing some of them is
even worse.
+
+ for (i = 0; i < def->nmems; i++) {
+ if ((idx = qemuDomainDeviceAliasIndex(&def->mems[i]->info,
"dimm")) != i) {
[1] You've parsed the alias here once now.
+ for (j = 0; j < def->nmems; j++) {
+ if ((idx = qemuDomainDeviceAliasIndex(&def->mems[j]->info,
"dimm")) == i)
+ break;
+ }
+
+ if (j >= def->nmems) {
+ minidx = i;
+ break;
+ }
This is a rather weird algorithm.
+ }
+
+ if ((idx = qemuDomainDeviceAliasIndex(&def->mems[i]->info,
"dimm")) >= minidx)
You are doing the same as in [1]. Seems like a waste.
+ minidx = idx + 1;
+ }
+
+ return minidx;
+}
I don't see a reason to have the function above if we switch to
pre-allocating the numbers properly in the first place. Also then it
will be possible to find the index just by looking at the slot number
rather than parsing the alias over and over.
If you're not into making sure that qemu accepts slots and doing the
slot allocation I can do that since I was up to fix this problem
anyways.
Peter
1:02 p.m.
New subject: [libvirt] [PATCH 2/2] tests/qemuxml2argvdata:Add a testcase
Signed-off-by: Nitesh Konkar <nitkon12(a)linux.vnet.ibm.com>
---
.../qemuxml2argv-memory-hotplug-dimm-alias.args | 35 ++++++++
.../qemuxml2argv-memory-hotplug-dimm-alias.xml | 94 ++++++++++++++++++++++
tests/qemuxml2argvtest.c | 2 +
3 files changed, 131 insertions(+)
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm-alias.args
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm-alias.xml
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm-alias.args
b/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm-alias.args
new file mode 100644
index 0000000..0e9f8e0
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm-alias.args
@@ -0,0 +1,35 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=none \
+/home/usr/qemu/x86_64-softmmu/qemu-system-x86_64 \
+-name Fedora \
+-S \
+-M pc-i440fx-2.7 \
+-m size=1048576k,slots=32,maxmem=3145728k \
+-smp 8,maxcpus=160,sockets=40,cores=2,threads=2 \
+-numa node,nodeid=0,cpus=0-3,mem=512 \
+-numa node,nodeid=1,cpus=4-7,mem=512 \
+-object memory-backend-ram,id=memdimm2,size=134217728 \
+-device pc-dimm,node=0,memdev=memdimm2,id=dimm2,slot=2,addr=4563402752 \
+-object memory-backend-ram,id=memdimm4,size=134217728 \
+-device pc-dimm,node=0,memdev=memdimm4,id=dimm4,slot=4,addr=4966055936 \
+-object memory-backend-ram,id=memdimm0,size=134217728 \
+-device pc-dimm,node=0,memdev=memdimm0,id=dimm0,slot=0,addr=4294967296 \
+-object memory-backend-ram,id=memdimm1,size=134217728 \
+-device pc-dimm,node=0,memdev=memdimm1,id=dimm1,slot=1,addr=4429185024 \
+-object memory-backend-ram,id=memdimm3,size=134217728 \
+-device pc-dimm,node=0,memdev=memdimm3,id=dimm3,slot=3,addr=4697620480 \
+-uuid aecf3e5e-6f9a-42a3-9d6a-223a75569a66 \
+-nographic \
+-nodefaults \
+-monitor unix:/tmp/lib/domain--1-Fedora/monitor.sock,server,nowait \
+-boot c \
+-usb \
+-drive file=/home/nitesh/Fedora24.qcow2,format=qcow2,if=none,id=drive-ide0-0-0 \
+-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \
+-serial pty \
+-device usb-mouse,id=input0,bus=usb.0,port=1 \
+-vga cirrus \
+-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x6
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm-alias.xml
b/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm-alias.xml
new file mode 100644
index 0000000..9c0625d
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm-alias.xml
@@ -0,0 +1,94 @@
+<domain type='kvm'>
+ <name>Fedora</name>
+ <uuid>aecf3e5e-6f9a-42a3-9d6a-223a75569a66</uuid>
+ <maxMemory slots='32' unit='KiB'>3145728</maxMemory>
+ <memory unit='KiB'>1703936</memory>
+ <currentMemory unit='KiB'>1572864</currentMemory>
+ <vcpu placement='static' current='8'>160</vcpu>
+ <os>
+ <type arch='x86_64' machine='pc-i440fx-2.7'>hvm</type>
+ <boot dev='hd'/>
+ <bootmenu enable='yes'/>
+ </os>
+ <features>
+ <acpi/>
+ </features>
+ <cpu>
+ <topology sockets='40' cores='2' threads='2'/>
+ <numa>
+ <cell id='0' cpus='0-3' memory='524288'
unit='KiB'/>
+ <cell id='1' cpus='4-7' memory='524288'
unit='KiB'/>
+ </numa>
+ </cpu>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/home/usr/qemu/x86_64-softmmu/qemu-system-x86_64</emulator>
+ <disk type='file' device='disk'>
+ <driver name='qemu' type='qcow2'/>
+ <source file='/home/nitesh/Fedora24.qcow2'/>
+ <target dev='hda' bus='ide'/>
+ <address type='drive' controller='0' bus='0'
target='0' unit='0'/>
+ </disk>
+ <controller type='pci' index='0' model='pci-root'/>
+ <controller type='ide' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00'
slot='0x01' function='0x1'/>
+ </controller>
+ <serial type='pty'>
+ <target port='0'/>
+ </serial>
+ <console type='pty'>
+ <target type='serial' port='0'/>
+ </console>
+ <input type='mouse' bus='usb'>
+ <address type='usb' bus='0' port='1'/>
+ </input>
+ <input type='mouse' bus='ps2'/>
+ <input type='keyboard' bus='ps2'/>
+ <video>
+ <model type='cirrus' vram='16384' heads='1'
primary='yes'/>
+ <address type='pci' domain='0x0000' bus='0x00'
slot='0x02' function='0x0'/>
+ </video>
+ <memballoon model='virtio'>
+ <address type='pci' domain='0x0000' bus='0x00'
slot='0x06' function='0x0'/>
+ </memballoon>
+ <memory model='dimm'>
+ <target>
+ <size unit='KiB'>131072</size>
+ <node>0</node>
+ </target>
+ <address type='dimm' slot='2' base='0x110000000'/>
+ </memory>
+ <memory model='dimm'>
+ <target>
+ <size unit='KiB'>131072</size>
+ <node>0</node>
+ </target>
+ <address type='dimm' slot='4' base='0x128000000'/>
+ </memory>
+ <memory model='dimm'>
+ <target>
+ <size unit='KiB'>131072</size>
+ <node>0</node>
+ </target>
+ <address type='dimm' slot='0' base='0x100000000'/>
+ </memory>
+ <memory model='dimm'>
+ <target>
+ <size unit='KiB'>131072</size>
+ <node>0</node>
+ </target>
+ <address type='dimm' slot='1' base='0x108000000'/>
+ </memory>
+ <memory model='dimm'>
+ <target>
+ <size unit='KiB'>131072</size>
+ <node>0</node>
+ </target>
+ <address type='dimm' slot='3' base='0x118000000'/>
+ </memory>
+ </devices>
+ <seclabel type='none' model='none'/>
+</domain>
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index aaf00c2..a592c40 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -1992,6 +1992,8 @@ mymain(void)
QEMU_CAPS_OBJECT_MEMORY_FILE);
DO_TEST("memory-hotplug-ppc64-nonuma", QEMU_CAPS_KVM,
QEMU_CAPS_DEVICE_PC_DIMM, QEMU_CAPS_NUMA,
QEMU_CAPS_OBJECT_MEMORY_RAM, QEMU_CAPS_OBJECT_MEMORY_FILE);
+ DO_TEST("memory-hotplug-dimm-alias", QEMU_CAPS_KVM,
QEMU_CAPS_DEVICE_PC_DIMM, QEMU_CAPS_NUMA,
+ QEMU_CAPS_OBJECT_MEMORY_RAM, QEMU_CAPS_OBJECT_MEMORY_FILE);
DO_TEST("machine-aeskeywrap-on-caps",
QEMU_CAPS_MACHINE_OPT, QEMU_CAPS_AES_KEY_WRAP,
--
1.9.3
2982
days inactive
2983
days old
3 comments
2 participants
participants (2)
-
Nitesh Konkar -
Peter Krempa