[libvirt] [PATCH] qemu: Use memory-object-file when needed
We were missing a check for '!nodemask' in the condition that decides whether we need memory-object-file or not. We only used userNodeset, but forgot that we might need to use nodemask even if that's not set. Even though there was a test for this, the args file was incorrect, so it is very nicely visible what this change does function-wise. Even with that I added the same test but instead of auto nodeset I specified one in numatune/memory just to make sure it fixes the problem. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_command.c | 3 ++- .../qemuxml2argv-numatune-auto-prefer.args | 4 ++- ...emuxml2argv-numatune-hugepages-no-memnode.args} | 3 ++- .../qemuxml2argv-numatune-hugepages-no-memnode.xml | 29 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 2 ++ 5 files changed, 38 insertions(+), 3 deletions(-) copy tests/qemuxml2argvdata/{qemuxml2argv-numatune-auto-prefer.args => qemuxml2argv-numatune-hugepages-no-memnode.args} (72%) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-numatune-hugepages-no-memnode.xml diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 5d3ab3a4eac5..debaea491be8 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5238,7 +5238,8 @@ qemuBuildMemoryBackendStr(unsigned long long size, } /* If none of the following is requested... */ - if (!pagesize && !userNodeset && !memAccess && !nodeSpecified && !force) { + if (!pagesize && !userNodeset && !memAccess && + !nodeSpecified && !nodemask && !force) { /* report back that using the new backend is not necessary * to achieve the desired configuration */ ret = 1; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-numatune-auto-prefer.args b/tests/qemuxml2argvdata/qemuxml2argv-numatune-auto-prefer.args index b4c49d44c525..710dae2aaa56 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-numatune-auto-prefer.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-numatune-auto-prefer.args @@ -10,7 +10,9 @@ QEMU_AUDIO_DRV=none \ -M pc \ -m 64 \ -smp 1 \ --numa node,nodeid=0,cpus=0,mem=64 \ +-object memory-backend-ram,id=ram-node0,size=67108864,host-nodes=0-3,\ +policy=preferred \ +-numa node,nodeid=0,cpus=0,memdev=ram-node0 \ -uuid 9f4b6512-e73a-4a25-93e8-5307802821ce \ -nographic \ -monitor unix:/tmp/test-monitor,server,nowait \ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-numatune-auto-prefer.args b/tests/qemuxml2argvdata/qemuxml2argv-numatune-hugepages-no-memnode.args similarity index 72% copy from tests/qemuxml2argvdata/qemuxml2argv-numatune-auto-prefer.args copy to tests/qemuxml2argvdata/qemuxml2argv-numatune-hugepages-no-memnode.args index b4c49d44c525..3ee81e9da7be 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-numatune-auto-prefer.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-numatune-hugepages-no-memnode.args @@ -10,7 +10,8 @@ QEMU_AUDIO_DRV=none \ -M pc \ -m 64 \ -smp 1 \ --numa node,nodeid=0,cpus=0,mem=64 \ +-object memory-backend-ram,id=ram-node0,size=67108864,host-nodes=1,policy=bind \ +-numa node,nodeid=0,cpus=0,memdev=ram-node0 \ -uuid 9f4b6512-e73a-4a25-93e8-5307802821ce \ -nographic \ -monitor unix:/tmp/test-monitor,server,nowait \ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-numatune-hugepages-no-memnode.xml b/tests/qemuxml2argvdata/qemuxml2argv-numatune-hugepages-no-memnode.xml new file mode 100644 index 000000000000..83fe891306ce --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-numatune-hugepages-no-memnode.xml @@ -0,0 +1,29 @@ +<domain type='qemu'> + <name>QEMUGuest</name> + <uuid>9f4b6512-e73a-4a25-93e8-5307802821ce</uuid> + <memory unit='KiB'>65536</memory> + <currentMemory unit='KiB'>65536</currentMemory> + <vcpu placement='static'>1</vcpu> + <numatune> + <memory nodeset='1'/> + </numatune> + <os> + <type arch='x86_64' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <cpu> + <numa> + <cell id='0' cpus='0' memory='65536' unit='KiB'/> + </numa> + </cpu> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/kvm</emulator> + <controller type='usb' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 7cd8c8e18f0e..1a7cfa3d9de3 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1344,6 +1344,8 @@ mymain(void) DO_TEST("numatune-auto-nodeset-invalid", NONE); DO_TEST("numatune-auto-prefer", QEMU_CAPS_OBJECT_MEMORY_RAM, QEMU_CAPS_OBJECT_MEMORY_FILE); + DO_TEST("numatune-hugepages-no-memnode", QEMU_CAPS_OBJECT_MEMORY_RAM, + QEMU_CAPS_OBJECT_MEMORY_FILE); DO_TEST_FAILURE("numatune-static-nodeset-exceed-hostnode", QEMU_CAPS_OBJECT_MEMORY_RAM); DO_TEST_PARSE_ERROR("numatune-memnode-nocpu", NONE); -- 2.7.0
On Tue, Jan 12, 2016 at 06:48:02PM +0100, Martin Kletzander wrote:
We were missing a check for '!nodemask' in the condition that decides whether we need memory-object-file or not. We only used userNodeset, but forgot that we might need to use nodemask even if that's not set.
Even though there was a test for this, the args file was incorrect, so it is very nicely visible what this change does function-wise. Even with that I added the same test but instead of auto nodeset I specified one in numatune/memory just to make sure it fixes the problem.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_command.c | 3 ++- .../qemuxml2argv-numatune-auto-prefer.args | 4 ++- ...emuxml2argv-numatune-hugepages-no-memnode.args} | 3 ++- .../qemuxml2argv-numatune-hugepages-no-memnode.xml | 29 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 2 ++ 5 files changed, 38 insertions(+), 3 deletions(-) copy tests/qemuxml2argvdata/{qemuxml2argv-numatune-auto-prefer.args => qemuxml2argv-numatune-hugepages-no-memnode.args} (72%) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-numatune-hugepages-no-memnode.xml
I forgot to run 'git commit --amend' before 'git send-email', so this needs to be considered squashed in: diff --git c/src/qemu/qemu_command.c i/src/qemu/qemu_command.c index debaea491be8..55f19a27272c 100644 --- c/src/qemu/qemu_command.c +++ i/src/qemu/qemu_command.c @@ -5238,8 +5238,7 @@ qemuBuildMemoryBackendStr(unsigned long long size, } /* If none of the following is requested... */ - if (!pagesize && !userNodeset && !memAccess && - !nodeSpecified && !nodemask && !force) { + if (!pagesize && !nodemask && !memAccess && !nodeSpecified && !force) { /* report back that using the new backend is not necessary * to achieve the desired configuration */ ret = 1; -- Basically I just used 'nodemask' instead of 'userNodeset'. It works even without this diff because nodemask will always be set if userNodeset is set, it's just redundant to specify both. Martin
Hi Martin, I see, the memory from the node specified in numatune is used correctly. Wit your change memory-backend-file is used to enforce that But, I see the migrations from old versions(my case, 1.2.5) seems to fail as there was no backend-file before and only backend-ram. Unknown ramblock "ppc_spapr.ram", cannot accept migration 2016-01-13T08:16:17.488267Z qemu-system-ppc64-hp: error while loading state for instance 0x0 of device 'ram' Should we be using -mem-path /dev/hugepages/libvirt/qemu along with memory-backend-ram instead? Thanks, Shivaprasad My guest : https://paste.fedoraproject.org/310103/67350814/ PS: I have some private changes in 1.2.5 but I believe that doesn't affect anything here. Please let me know if you cant hit it. On Tue, Jan 12, 2016 at 11:28 PM, Martin Kletzander <mkletzan@redhat.com> wrote:
On Tue, Jan 12, 2016 at 06:48:02PM +0100, Martin Kletzander wrote:
We were missing a check for '!nodemask' in the condition that decides whether we need memory-object-file or not. We only used userNodeset, but forgot that we might need to use nodemask even if that's not set.
Even though there was a test for this, the args file was incorrect, so it is very nicely visible what this change does function-wise. Even with that I added the same test but instead of auto nodeset I specified one in numatune/memory just to make sure it fixes the problem.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_command.c | 3 ++- .../qemuxml2argv-numatune-auto-prefer.args | 4 ++- ...emuxml2argv-numatune-hugepages-no-memnode.args} | 3 ++- .../qemuxml2argv-numatune-hugepages-no-memnode.xml | 29 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 2 ++ 5 files changed, 38 insertions(+), 3 deletions(-) copy tests/qemuxml2argvdata/{qemuxml2argv-numatune-auto-prefer.args => qemuxml2argv-numatune-hugepages-no-memnode.args} (72%) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-numatune-hugepages-no-memnode.xml
I forgot to run 'git commit --amend' before 'git send-email', so this needs to be considered squashed in:
diff --git c/src/qemu/qemu_command.c i/src/qemu/qemu_command.c index debaea491be8..55f19a27272c 100644 --- c/src/qemu/qemu_command.c +++ i/src/qemu/qemu_command.c @@ -5238,8 +5238,7 @@ qemuBuildMemoryBackendStr(unsigned long long size, }
/* If none of the following is requested... */ - if (!pagesize && !userNodeset && !memAccess && - !nodeSpecified && !nodemask && !force) { + if (!pagesize && !nodemask && !memAccess && !nodeSpecified && !force) { /* report back that using the new backend is not necessary * to achieve the desired configuration */ ret = 1; --
Basically I just used 'nodemask' instead of 'userNodeset'. It works even without this diff because nodemask will always be set if userNodeset is set, it's just redundant to specify both.
Martin
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Jan 13, 2016 at 02:14:58PM +0530, Shivaprasad bhat wrote:
Hi Martin,
I see, the memory from the node specified in numatune is used correctly. Wit your change memory-backend-file is used to enforce that
But, I see the migrations from old versions(my case, 1.2.5) seems to fail as there was no backend-file before and only backend-ram.
Oh, you are absolutely right, that's why we kept the code as is, I totally forgot about the reasoning behind this. If this doesn't affect guest ABI, then we could use a migration flag for deciding which approach to take. However, I don't feel strongly for the added complexity that such change would make. Not in the migration code, but rather the command-line change. Anyway, we need to keep doing what we were before, so this patch is a SNACK. We can decide what to do later on.
Unknown ramblock "ppc_spapr.ram", cannot accept migration 2016-01-13T08:16:17.488267Z qemu-system-ppc64-hp: error while loading state for instance 0x0 of device 'ram'
Should we be using -mem-path /dev/hugepages/libvirt/qemu along with memory-backend-ram instead?
Both or none, plus this is not the root cause.
participants (2)
-
Martin Kletzander -
Shivaprasad bhat