[libvirt] [PATCH 0/2] Fix virsh save-restore/migration with memory hotplug.
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
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@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
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@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
Signed-off-by: Nitesh Konkar <nitkon12@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
participants (2)
-
Nitesh Konkar -
Peter Krempa