[libvirt] [PATCH 0/2] qemuBuildMemoryBackendStr: Couple of fixes

The first one fixes a bug we have for a long time. The second one is not that ancient but still worth fixing. Michal Privoznik (2): qemuBuildMemoryBackendStr: Fix hugepages lookup process qemuBuildMemoryBackendStr: Honour passed @pagesize src/qemu/qemu_command.c | 57 +++++----- .../qemuxml2argv-hugepages-numa.args | 49 +++++++++ .../qemuxml2argv-hugepages-numa.xml | 118 +++++++++++++++++++++ .../qemuxml2argv-memory-hotplug-dimm-addr.args | 4 +- .../qemuxml2argv-memory-hotplug-dimm-addr.xml | 2 +- .../qemuxml2argv-memory-hotplug-dimm.args | 4 +- .../qemuxml2argv-memory-hotplug-dimm.xml | 2 +- tests/qemuxml2argvtest.c | 14 ++- 8 files changed, 217 insertions(+), 33 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hugepages-numa.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hugepages-numa.xml -- 2.3.6

https://bugzilla.redhat.com/show_bug.cgi?id=1196644 So this function is called to construct the guest NUMA part of the qemu command line. At the beginning, the configured hugepages are searched to find the best match for given guest NUMA node. Configured hugepages can have a @nodeset attribute to specify on which guest NUMA nodes should be the hugepages backing used. There is, however, one 'corner case'. Users may just tell 'use hugepages to back all the nodes'. In other words: <memoryBacking> <hugepages/> </memoryBacking> <cpu> <numa> <cell id='0' cpus='0-1' memory='1024000' unit='KiB'/> </numa> </cpu> Our code fails in this case. Well, since there's no @nodeset (nor any <page/> child element to <hugepages/>) we fail to lookup the default hugepage size to use. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 41 ++++---- .../qemuxml2argv-hugepages-numa.args | 46 +++++++++ .../qemuxml2argv-hugepages-numa.xml | 107 +++++++++++++++++++++ tests/qemuxml2argvtest.c | 8 ++ 4 files changed, 183 insertions(+), 19 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hugepages-numa.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hugepages-numa.xml diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 99755f1..cb31fac 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4760,9 +4760,6 @@ qemuBuildMemoryBackendStr(unsigned long long size, hugepage = master_hugepage; } - if (hugepage) - pagesize = hugepage->size; - if (hugepage && hugepage->size == system_page_size) { /* However, if user specified to use "huge" page * of regular system page size, it's as if they @@ -4778,23 +4775,29 @@ qemuBuildMemoryBackendStr(unsigned long long size, goto cleanup; } - /* Now lets see, if the huge page we want to use is even mounted - * and ready to use */ - for (i = 0; i < cfg->nhugetlbfs; i++) { - if (cfg->hugetlbfs[i].size == hugepage->size) - break; - } - - if (i == cfg->nhugetlbfs) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unable to find any usable hugetlbfs mount for %llu KiB"), - pagesize); - goto cleanup; - } - VIR_FREE(mem_path); - if (!(mem_path = qemuGetHugepagePath(&cfg->hugetlbfs[i]))) - goto cleanup; + if (hugepage->size) { + /* Now lets see, if the huge page we want to use is even mounted + * and ready to use */ + for (i = 0; i < cfg->nhugetlbfs; i++) { + if (cfg->hugetlbfs[i].size == hugepage->size) + break; + } + + if (i == cfg->nhugetlbfs) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to find any usable hugetlbfs mount for %llu KiB"), + hugepage->size); + goto cleanup; + } + + if (!(mem_path = qemuGetHugepagePath(&cfg->hugetlbfs[i]))) + goto cleanup; + } else { + if (!(mem_path = qemuGetDefaultHugepath(cfg->hugetlbfs, + cfg->nhugetlbfs))) + goto cleanup; + } *backendType = "memory-backend-file"; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-numa.args b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-numa.args new file mode 100644 index 0000000..c47e097 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-numa.args @@ -0,0 +1,46 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=spice \ +/usr/bin/qemu-system-x86_64 \ +-S \ +-M pc-i440fx-2.3 \ +-cpu Haswell \ +-m 1024 \ +-smp 2 \ +-object memory-backend-file,id=ram-node0,prealloc=yes,\ +mem-path=/dev/hugepages2M/libvirt/qemu,size=1073741824 \ +-numa node,nodeid=0,cpus=0-1,memdev=ram-node0 \ +-nodefaults \ +-monitor unix:/tmp/test-monitor,server,nowait \ +-rtc base=utc,driftfix=slew \ +-no-kvm-pit-reinjection \ +-global PIIX4_PM.disable_s3=1 \ +-global PIIX4_PM.disable_s4=1 \ +-boot c \ +-device ich9-usb-ehci1,id=usb,bus=pci.0,addr=0x6.0x7 \ +-device ich9-usb-uhci1,masterbus=usb.0,firstport=0,bus=pci.0,multifunction=on,addr=0x6 \ +-device ich9-usb-uhci2,masterbus=usb.0,firstport=2,bus=pci.0,addr=0x6.0x1 \ +-device ich9-usb-uhci3,masterbus=usb.0,firstport=4,bus=pci.0,addr=0x6.0x2 \ +-device virtio-serial-pci,id=virtio-serial0,bus=pci.0,addr=0x5 \ +-drive file=/var/lib/libvirt/images/fedora.qcow2,if=none,id=drive-virtio-disk0 \ +-device virtio-blk-pci,bus=pci.0,addr=0x7,drive=drive-virtio-disk0,id=virtio-disk0 \ +-drive if=none,media=cdrom,id=drive-ide0-0-0 \ +-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ +-serial pty \ +-chardev socket,id=charchannel0,\ +path=/var/lib/libvirt/qemu/channel/target/fedora.org.qemu.guest_agent.0,server,nowait \ +-device virtserialport,bus=virtio-serial0.0,nr=1,\ +chardev=charchannel0,id=channel0,name=org.qemu.guest_agent.0 \ +-chardev spicevmc,id=charchannel1,name=vdagent \ +-device virtserialport,bus=virtio-serial0.0,nr=2,\ +chardev=charchannel1,id=channel1,name=com.redhat.spice.0 \ +-device usb-tablet,id=input0 \ +-spice port=0 \ +-vga qxl \ +-global qxl-vga.ram_size=67108864 \ +-global qxl-vga.vram_size=67108864 \ +-device intel-hda,id=sound0,bus=pci.0,addr=0x4 \ +-device hda-duplex,id=sound0-codec0,bus=sound0.0,cad=0 \ +-chardev spicevmc,id=charredir0,name=usbredir \ +-device usb-redir,chardev=charredir0,id=redir0 \ +-chardev spicevmc,id=charredir1,name=usbredir \ +-device usb-redir,chardev=charredir1,id=redir1 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x8 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-numa.xml b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-numa.xml new file mode 100644 index 0000000..70a8835 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-numa.xml @@ -0,0 +1,107 @@ +<domain type='qemu'> + <name>fedora</name> + <uuid>63840878-0deb-4095-97e6-fc444d9bc9fa</uuid> + <memory unit='KiB'>1572863</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <memoryBacking> + <hugepages/> + </memoryBacking> + <vcpu placement='static'>2</vcpu> + <os> + <type arch='x86_64' machine='pc-i440fx-2.3'>hvm</type> + <boot dev='hd'/> + </os> + <features> + <acpi/> + <apic/> + <pae/> + </features> + <cpu mode='custom' match='exact'> + <model fallback='allow'>Haswell</model> + <numa> + <cell id='0' cpus='0-1' memory='1048576' unit='KiB'/> + </numa> + </cpu> + <clock offset='utc'> + <timer name='rtc' tickpolicy='catchup'/> + <timer name='pit' tickpolicy='delay'/> + <timer name='hpet' present='no'/> + </clock> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <pm> + <suspend-to-mem enabled='no'/> + <suspend-to-disk enabled='no'/> + </pm> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <disk type='file' device='disk'> + <driver name='qemu' type='qcow2'/> + <source file='/var/lib/libvirt/images/fedora.qcow2'/> + <target dev='vda' bus='virtio'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x07' function='0x0'/> + </disk> + <disk type='block' device='cdrom'> + <driver name='qemu' type='raw'/> + <target dev='hda' bus='ide'/> + <readonly/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0' model='ich9-ehci1'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x7'/> + </controller> + <controller type='usb' index='0' model='ich9-uhci1'> + <master startport='0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x0' multifunction='on'/> + </controller> + <controller type='usb' index='0' model='ich9-uhci2'> + <master startport='2'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x1'/> + </controller> + <controller type='usb' index='0' model='ich9-uhci3'> + <master startport='4'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x2'/> + </controller> + <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> + <controller type='virtio-serial' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/> + </controller> + <serial type='pty'> + <target port='0'/> + </serial> + <console type='pty'> + <target type='serial' port='0'/> + </console> + <channel type='unix'> + <source mode='bind' path='/var/lib/libvirt/qemu/channel/target/fedora.org.qemu.guest_agent.0'/> + <target type='virtio' name='org.qemu.guest_agent.0'/> + <address type='virtio-serial' controller='0' bus='0' port='1'/> + </channel> + <channel type='spicevmc'> + <target type='virtio' name='com.redhat.spice.0'/> + <address type='virtio-serial' controller='0' bus='0' port='2'/> + </channel> + <input type='tablet' bus='usb'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <graphics type='spice' autoport='yes'/> + <sound model='ich6'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> + </sound> + <video> + <model type='qxl' ram='65536' vram='65536' vgamem='16384' heads='1'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + </video> + <redirdev bus='usb' type='spicevmc'> + </redirdev> + <redirdev bus='usb' type='spicevmc'> + </redirdev> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x08' function='0x0'/> + </memballoon> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index bb71991..ce7c613 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -683,6 +683,14 @@ mymain(void) DO_TEST("pmu-feature-off", NONE); DO_TEST("hugepages", QEMU_CAPS_MEM_PATH); + DO_TEST("hugepages-numa", QEMU_CAPS_RTC, QEMU_CAPS_NO_KVM_PIT, + QEMU_CAPS_DISABLE_S3, QEMU_CAPS_DISABLE_S4, + QEMU_CAPS_DEVICE, QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_DRIVE, + QEMU_CAPS_ICH9_USB_EHCI1, QEMU_CAPS_PCI_MULTIFUNCTION, + QEMU_CAPS_SPICE, QEMU_CAPS_CHARDEV_SPICEVMC, + QEMU_CAPS_VGA, QEMU_CAPS_VGA_QXL, + QEMU_CAPS_HDA_DUPLEX, QEMU_CAPS_USB_REDIR, + QEMU_CAPS_MEM_PATH, QEMU_CAPS_OBJECT_MEMORY_FILE); DO_TEST_LINUX("hugepages-pages", QEMU_CAPS_MEM_PATH, QEMU_CAPS_OBJECT_MEMORY_RAM, QEMU_CAPS_OBJECT_MEMORY_FILE); -- 2.3.6

On Thu, Jun 25, 2015 at 18:13:02 +0200, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1196644
So this function is called to construct the guest NUMA part of the qemu command line. At the beginning, the configured hugepages
This function constructs the backend (host facing) part of the memory device.
are searched to find the best match for given guest NUMA node. Configured hugepages can have a @nodeset attribute to specify on which guest NUMA nodes should be the hugepages backing used. There is, however, one 'corner case'. Users may just tell 'use hugepages to back all the nodes'. In other words:
<memoryBacking> <hugepages/> </memoryBacking>
<cpu> <numa> <cell id='0' cpus='0-1' memory='1024000' unit='KiB'/> </numa> </cpu>
Our code fails in this case. Well, since there's no @nodeset (nor any <page/> child element to <hugepages/>) we fail to lookup the default hugepage size to use.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 41 ++++---- .../qemuxml2argv-hugepages-numa.args | 46 +++++++++ .../qemuxml2argv-hugepages-numa.xml | 107 +++++++++++++++++++++ tests/qemuxml2argvtest.c | 8 ++ 4 files changed, 183 insertions(+), 19 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hugepages-numa.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hugepages-numa.xml
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 99755f1..cb31fac 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4760,9 +4760,6 @@ qemuBuildMemoryBackendStr(unsigned long long size, hugepage = master_hugepage; }
- if (hugepage) - pagesize = hugepage->size; -
This hunk is a bit awkward here since you add it back in the next patch. A rebase mistake perhaps?
if (hugepage && hugepage->size == system_page_size) { /* However, if user specified to use "huge" page * of regular system page size, it's as if they @@ -4778,23 +4775,29 @@ qemuBuildMemoryBackendStr(unsigned long long size, goto cleanup; }
- /* Now lets see, if the huge page we want to use is even mounted - * and ready to use */ - for (i = 0; i < cfg->nhugetlbfs; i++) { - if (cfg->hugetlbfs[i].size == hugepage->size) - break; - } - - if (i == cfg->nhugetlbfs) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unable to find any usable hugetlbfs mount for %llu KiB"), - pagesize); - goto cleanup; - } - VIR_FREE(mem_path);
mem_path is NULL prior to this free statement, and it is not in a loop so the VIR_FREE can be removed here.
- if (!(mem_path = qemuGetHugepagePath(&cfg->hugetlbfs[i]))) - goto cleanup; + if (hugepage->size) { + /* Now lets see, if the huge page we want to use is even mounted + * and ready to use */ + for (i = 0; i < cfg->nhugetlbfs; i++) { + if (cfg->hugetlbfs[i].size == hugepage->size) + break; + } + + if (i == cfg->nhugetlbfs) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to find any usable hugetlbfs mount for %llu KiB"), + hugepage->size); + goto cleanup; + } + + if (!(mem_path = qemuGetHugepagePath(&cfg->hugetlbfs[i]))) + goto cleanup; + } else { + if (!(mem_path = qemuGetDefaultHugepath(cfg->hugetlbfs, + cfg->nhugetlbfs))) + goto cleanup; + }
*backendType = "memory-backend-file";
ACK, Peter

So far the argument has not much meaning and was practically ignored. This is not good since when doing memory hotplug, the size of desired hugepage backing is passed in that argument. Taking closer look at the tests I'm fixing reveals the bug. For instance, while the following is in the test: <memory model='dimm'> <source> <nodemask>1-3</nodemask> <pagesize unit='KiB'>4096</pagesize> </source> <target> <size unit='KiB'>524287</size> <node>0</node> </target> <address type='dimm' slot='0' base='0x100000000'/> </memory> the generated commandline corresponding to this XML was: -object memory-backend-ram,id=memdimm0,size=536870912,\ host-nodes=1-3,policy=bind Have you noticed? Yes, memory-backend-ram! Nothing can be further away from the right answer. The hugepage backing is requested in the XML and we happily ignore it. This is just not right. It's memory-backend-file which should have been used: -object memory-backend-file,id=memdimm0,prealloc=yes,\ mem-path=/dev/hugepages4M/libvirt/qemu,size=536870912,\ host-nodes=1-3,policy=bind The problem is, that @pagesize passed to qemuBuildMemoryBackendStr (where this part of commandline is built) was ignored. The hugepage to back memory was searched only and only by NUMA nodes pinning. This works only for regular guest NUMA nodes. Then, I'm changing the hugepages size in the test XMLs too. This is simply because in the test suite we create dummy mount points just for 2M and 1G hugepages. And in the test 4M was requested. I'm sticking to 2M, but 1G should just work too. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 28 ++++++++++++---------- .../qemuxml2argv-hugepages-numa.args | 5 +++- .../qemuxml2argv-hugepages-numa.xml | 11 +++++++++ .../qemuxml2argv-memory-hotplug-dimm-addr.args | 4 ++-- .../qemuxml2argv-memory-hotplug-dimm-addr.xml | 2 +- .../qemuxml2argv-memory-hotplug-dimm.args | 4 ++-- .../qemuxml2argv-memory-hotplug-dimm.xml | 2 +- tests/qemuxml2argvtest.c | 8 ++++--- 8 files changed, 42 insertions(+), 22 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index cb31fac..e0214d8 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4727,7 +4727,7 @@ qemuBuildMemoryBackendStr(unsigned long long size, virDomainNumatuneGetMode(def->numa, -1, &mode) < 0) mode = VIR_DOMAIN_NUMATUNE_MEM_STRICT; - if (pagesize == 0 || pagesize != system_page_size) { + if (pagesize == 0) { /* Find the huge page size we want to use */ for (i = 0; i < def->mem.nhugepages; i++) { bool thisHugepage = false; @@ -4760,15 +4760,19 @@ qemuBuildMemoryBackendStr(unsigned long long size, hugepage = master_hugepage; } - if (hugepage && hugepage->size == system_page_size) { - /* However, if user specified to use "huge" page - * of regular system page size, it's as if they - * hasn't specified any huge pages at all. */ - hugepage = NULL; - } + if (hugepage) + pagesize = hugepage->size; } - if (hugepage) { + if (pagesize == system_page_size) { + /* However, if user specified to use "huge" page + * of regular system page size, it's as if they + * hasn't specified any huge pages at all. */ + pagesize = 0; + hugepage = NULL; + } + + if (pagesize || hugepage) { if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_FILE)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("this qemu doesn't support hugepage memory backing")); @@ -4776,18 +4780,18 @@ qemuBuildMemoryBackendStr(unsigned long long size, } VIR_FREE(mem_path); - if (hugepage->size) { + if (pagesize) { /* Now lets see, if the huge page we want to use is even mounted * and ready to use */ for (i = 0; i < cfg->nhugetlbfs; i++) { - if (cfg->hugetlbfs[i].size == hugepage->size) + if (cfg->hugetlbfs[i].size == pagesize) break; } if (i == cfg->nhugetlbfs) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Unable to find any usable hugetlbfs mount for %llu KiB"), - hugepage->size); + pagesize); goto cleanup; } @@ -4854,7 +4858,7 @@ qemuBuildMemoryBackendStr(unsigned long long size, goto cleanup; } - if (!hugepage) { + if (!hugepage && !pagesize) { bool nodeSpecified = virDomainNumatuneNodeSpecified(def->numa, guestNode); if ((userNodeset || nodeSpecified || force) && diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-numa.args b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-numa.args index c47e097..b697942 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-numa.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-numa.args @@ -3,11 +3,14 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=spice \ -S \ -M pc-i440fx-2.3 \ -cpu Haswell \ --m 1024 \ +-m size=1048576k,slots=16,maxmem=1099511627776k \ -smp 2 \ -object memory-backend-file,id=ram-node0,prealloc=yes,\ mem-path=/dev/hugepages2M/libvirt/qemu,size=1073741824 \ -numa node,nodeid=0,cpus=0-1,memdev=ram-node0 \ +-object memory-backend-file,id=memdimm0,prealloc=yes,\ +mem-path=/dev/hugepages1G/libvirt/qemu,size=1073741824,host-nodes=1-3,policy=bind \ +-device pc-dimm,node=0,memdev=memdimm0,id=dimm0 \ -nodefaults \ -monitor unix:/tmp/test-monitor,server,nowait \ -rtc base=utc,driftfix=slew \ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-numa.xml b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-numa.xml index 70a8835..8c1f19c 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-numa.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-numa.xml @@ -1,6 +1,7 @@ <domain type='qemu'> <name>fedora</name> <uuid>63840878-0deb-4095-97e6-fc444d9bc9fa</uuid> + <maxMemory slots='16' unit='KiB'>1099511627776</maxMemory> <memory unit='KiB'>1572863</memory> <currentMemory unit='KiB'>1048576</currentMemory> <memoryBacking> @@ -103,5 +104,15 @@ <memballoon model='virtio'> <address type='pci' domain='0x0000' bus='0x00' slot='0x08' function='0x0'/> </memballoon> + <memory model='dimm'> + <source> + <nodemask>1-3</nodemask> + <pagesize unit='KiB'>1048576</pagesize> + </source> + <target> + <size unit='KiB'>1048576</size> + <node>0</node> + </target> + </memory> </devices> </domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm-addr.args b/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm-addr.args index 7c59454..e996675 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm-addr.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm-addr.args @@ -1,8 +1,8 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ /usr/bin/qemu -S -M pc -m size=219136k,slots=16,maxmem=1099511627776k -smp 2 \ -numa node,nodeid=0,cpus=0-1,mem=214 \ --object memory-backend-ram,id=memdimm0,size=536870912,host-nodes=1-3,\ -policy=bind \ +-object memory-backend-file,id=memdimm0,prealloc=yes,\ +mem-path=/dev/hugepages2M/libvirt/qemu,size=536870912,host-nodes=1-3,policy=bind \ -device pc-dimm,node=0,memdev=memdimm0,id=dimm0,slot=0,addr=4294967296 \ -nographic -nodefaults -monitor unix:/tmp/test-monitor,server,nowait \ -no-acpi -boot c -usb -hda /dev/HostVG/QEMUGuest1 \ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm-addr.xml b/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm-addr.xml index 2e302c4..49f2f10 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm-addr.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm-addr.xml @@ -33,7 +33,7 @@ <memory model='dimm'> <source> <nodemask>1-3</nodemask> - <pagesize unit='KiB'>4096</pagesize> + <pagesize unit='KiB'>2048</pagesize> </source> <target> <size unit='KiB'>524287</size> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm.args b/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm.args index 7fbde33..17325d7 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm.args @@ -3,8 +3,8 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ -numa node,nodeid=0,cpus=0-1,mem=214 \ -object memory-backend-ram,id=memdimm0,size=536870912 \ -device pc-dimm,node=0,memdev=memdimm0,id=dimm0 \ --object memory-backend-ram,id=memdimm1,size=536870912,host-nodes=1-3,\ -policy=bind \ +-object memory-backend-file,id=memdimm1,prealloc=yes,\ +mem-path=/dev/hugepages2M/libvirt/qemu,size=536870912,host-nodes=1-3,policy=bind \ -device pc-dimm,node=0,memdev=memdimm1,id=dimm1 \ -nographic -nodefaults -monitor unix:/tmp/test-monitor,server,nowait \ -no-acpi -boot c -usb -hda /dev/HostVG/QEMUGuest1 \ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm.xml b/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm.xml index fa6013a..3f468ec 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm.xml @@ -43,7 +43,7 @@ <memory model='dimm'> <source> <nodemask>1-3</nodemask> - <pagesize unit='KiB'>4096</pagesize> + <pagesize unit='KiB'>2048</pagesize> </source> <target> <size unit='KiB'>524287</size> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index ce7c613..d3435d4 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -690,7 +690,9 @@ mymain(void) QEMU_CAPS_SPICE, QEMU_CAPS_CHARDEV_SPICEVMC, QEMU_CAPS_VGA, QEMU_CAPS_VGA_QXL, QEMU_CAPS_HDA_DUPLEX, QEMU_CAPS_USB_REDIR, - QEMU_CAPS_MEM_PATH, QEMU_CAPS_OBJECT_MEMORY_FILE); + QEMU_CAPS_DEVICE_PC_DIMM, + QEMU_CAPS_MEM_PATH, QEMU_CAPS_OBJECT_MEMORY_FILE, + QEMU_CAPS_OBJECT_MEMORY_FILE); DO_TEST_LINUX("hugepages-pages", QEMU_CAPS_MEM_PATH, QEMU_CAPS_OBJECT_MEMORY_RAM, QEMU_CAPS_OBJECT_MEMORY_FILE); @@ -1605,9 +1607,9 @@ mymain(void) DO_TEST_FAILURE("memory-hotplug", NONE); DO_TEST("memory-hotplug", QEMU_CAPS_DEVICE_PC_DIMM, QEMU_CAPS_NUMA); DO_TEST("memory-hotplug-dimm", QEMU_CAPS_DEVICE_PC_DIMM, QEMU_CAPS_NUMA, - QEMU_CAPS_DEVICE, QEMU_CAPS_OBJECT_MEMORY_RAM); + QEMU_CAPS_DEVICE, QEMU_CAPS_OBJECT_MEMORY_RAM, QEMU_CAPS_OBJECT_MEMORY_FILE); DO_TEST("memory-hotplug-dimm-addr", QEMU_CAPS_DEVICE_PC_DIMM, QEMU_CAPS_NUMA, - QEMU_CAPS_DEVICE, QEMU_CAPS_OBJECT_MEMORY_RAM); + QEMU_CAPS_DEVICE, QEMU_CAPS_OBJECT_MEMORY_FILE); DO_TEST("machine-aeskeywrap-on-caps", QEMU_CAPS_MACHINE_OPT, QEMU_CAPS_AES_KEY_WRAP, -- 2.3.6

On Thu, Jun 25, 2015 at 18:13:03 +0200, Michal Privoznik wrote:
So far the argument has not much meaning and was practically ignored. This is not good since when doing memory hotplug, the size of desired hugepage backing is passed in that argument. Taking closer look at the tests I'm fixing reveals the bug. For instance, while the following is in the test:
<memory model='dimm'> <source> <nodemask>1-3</nodemask> <pagesize unit='KiB'>4096</pagesize> </source> <target> <size unit='KiB'>524287</size> <node>0</node> </target> <address type='dimm' slot='0' base='0x100000000'/> </memory>
the generated commandline corresponding to this XML was:
-object memory-backend-ram,id=memdimm0,size=536870912,\ host-nodes=1-3,policy=bind
Have you noticed? Yes, memory-backend-ram! Nothing can be further away from the right answer. The hugepage backing is requested in the XML and we happily ignore it. This is just not right. It's memory-backend-file which should have been used:
-object memory-backend-file,id=memdimm0,prealloc=yes,\ mem-path=/dev/hugepages4M/libvirt/qemu,size=536870912,\ host-nodes=1-3,policy=bind
The problem is, that @pagesize passed to qemuBuildMemoryBackendStr (where this part of commandline is built) was ignored. The hugepage to back memory was searched only and only by NUMA nodes pinning. This works only for regular guest NUMA nodes.
Then, I'm changing the hugepages size in the test XMLs too. This is simply because in the test suite we create dummy mount points just for 2M and 1G hugepages. And in the test 4M was requested. I'm sticking to 2M, but 1G should just work too.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 28 ++++++++++++---------- .../qemuxml2argv-hugepages-numa.args | 5 +++- .../qemuxml2argv-hugepages-numa.xml | 11 +++++++++ .../qemuxml2argv-memory-hotplug-dimm-addr.args | 4 ++-- .../qemuxml2argv-memory-hotplug-dimm-addr.xml | 2 +- .../qemuxml2argv-memory-hotplug-dimm.args | 4 ++-- .../qemuxml2argv-memory-hotplug-dimm.xml | 2 +- tests/qemuxml2argvtest.c | 8 ++++--- 8 files changed, 42 insertions(+), 22 deletions(-)
ACK, Peter
participants (2)
-
Michal Privoznik
-
Peter Krempa