[PATCH 0/1] qemu: Add support for free-page-reporting
gitlab issue: https://gitlab.com/libvirt/libvirt/-/issues/79 The virtio-balloon device now has the ability to report free pages back to the hypervisor for reuse by other programs. This kernel feature allows for more stable and resource friendly systems. This feature is available in QEMU and is enabled with free-page-reporting=on default virt setting should be off but the user should be able to enable it. Nico Pache (1): Optional free-page-reporting can be enabled/disabled for memballon device of model 'virtio'. docs/formatdomain.rst | 6 ++++++ docs/schemas/domaincommon.rng | 5 +++++ src/conf/domain_conf.c | 21 +++++++++++++++++++ src/conf/domain_conf.h | 1 + src/qemu/qemu_capabilities.c | 4 +++- src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 5 +++++ src/qemu/qemu_validate.c | 7 +++++++ .../caps_5.1.0.x86_64.xml | 1 + .../caps_5.2.0.x86_64.xml | 1 + 10 files changed, 51 insertions(+), 1 deletion(-) -- 2.18.4
xml: <devices> <memballoon model='virtio' free-page-reporting='on'/> </devices> qemu: qemu -device virtio-balloon-pci,...,free-page-reporting=on This kernel feature allows for more stable and resource friendly systems. Signed-off-by: Nico Pache <npache@redhat.com> --- docs/formatdomain.rst | 6 ++++++ docs/schemas/domaincommon.rng | 5 +++++ src/conf/domain_conf.c | 21 +++++++++++++++++++ src/conf/domain_conf.h | 1 + src/qemu/qemu_capabilities.c | 4 +++- src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 5 +++++ src/qemu/qemu_validate.c | 7 +++++++ .../caps_5.1.0.x86_64.xml | 1 + .../caps_5.2.0.x86_64.xml | 1 + 10 files changed, 51 insertions(+), 1 deletion(-) diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index f3cf9e1fb3..f4c7765f5f 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -6755,6 +6755,12 @@ Example: manually added device with static PCI slot 2 requested release some memory at the last moment before a guest's process get killed by Out of Memory killer. :since:`Since 1.3.1, QEMU and KVM only` +``free-page-reporting`` + The optional ``free-page-reporting`` attribute allows to enable/disable + ("on"/"off", respectively) the ability of the QEMU virtio memory balloon to + return unused pages back to the hypervisor to be used by other guests or + processes. :since:`Since 5.1, QEMU and KVM only` + ``period`` The optional ``period`` allows the QEMU virtio memory balloon driver to provide statistics through the ``virsh dommemstat [domain]`` diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 4b7e460148..4c32b3555f 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -4539,6 +4539,11 @@ <ref name="virOnOff"/> </attribute> </optional> + <optional> + <attribute name="free-page-reporting"> + <ref name="virOnOff"/> + </attribute> + </optional> <interleave> <optional> <ref name="alias"/> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b80d7c7c6c..f4b8e8d8c3 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -15384,6 +15384,7 @@ virDomainMemballoonDefParseXML(virDomainXMLOptionPtr xmlopt, VIR_XPATH_NODE_AUTORESTORE(ctxt) unsigned int period = 0; g_autofree char *model = NULL; + g_autofree char *freepage_reporting = NULL; g_autofree char *deflate = NULL; if (VIR_ALLOC(def) < 0) @@ -15409,6 +15410,13 @@ virDomainMemballoonDefParseXML(virDomainXMLOptionPtr xmlopt, goto error; } + if ((freepage_reporting = virXMLPropString(node, "free-page-reporting")) && + (def->free_page_reporting = virTristateSwitchTypeFromString(freepage_reporting)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("invalid free-page-reporting attribute value '%s'"), freepage_reporting); + goto error; + } + ctxt->node = node; if (virXPathUInt("string(./stats/@period)", ctxt, &period) < -1) { virReportError(VIR_ERR_XML_ERROR, "%s", @@ -23630,6 +23638,15 @@ virDomainMemballoonDefCheckABIStability(virDomainMemballoonDefPtr src, return false; } + if (src->free_page_reporting != dst->free_page_reporting) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Target balloon free-page-reporting attribute value " + "'%s' does not match source '%s'"), + virTristateSwitchTypeToString(dst->free_page_reporting), + virTristateSwitchTypeToString(src->free_page_reporting)); + return false; + } + if (src->virtio && dst->virtio && !virDomainVirtioOptionsCheckABIStability(src->virtio, dst->virtio)) return false; @@ -27737,6 +27754,10 @@ virDomainMemballoonDefFormat(virBufferPtr buf, virBufferAsprintf(&attrBuf, " autodeflate='%s'", virTristateSwitchTypeToString(def->autodeflate)); + if (def->free_page_reporting != VIR_TRISTATE_SWITCH_ABSENT) + virBufferAsprintf(&attrBuf, " free-page-reporting='%s'", + virTristateSwitchTypeToString(def->free_page_reporting)); + if (def->period) virBufferAsprintf(&childrenBuf, "<stats period='%i'/>\n", def->period); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 9a44315519..f5f8baf32c 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1790,6 +1790,7 @@ struct _virDomainMemballoonDef { virDomainDeviceInfo info; int period; /* seconds between collections */ int autodeflate; /* enum virTristateSwitch */ + int free_page_reporting; /* enum virTristateSwitch */ virDomainVirtioOptionsPtr virtio; }; diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 5dcfcd574d..88a2c8aacb 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -600,7 +600,8 @@ VIR_ENUM_IMPL(virQEMUCaps, /* 380 */ "usb-host.hostdevice", - ); + "virtio-balloon-pci.free-page-reporting", + ); typedef struct _virQEMUCapsMachineType virQEMUCapsMachineType; @@ -1344,6 +1345,7 @@ static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsVirtioBalloon[] { "iommu_platform", QEMU_CAPS_VIRTIO_PCI_IOMMU_PLATFORM, NULL }, { "ats", QEMU_CAPS_VIRTIO_PCI_ATS, NULL }, { "packed", QEMU_CAPS_VIRTIO_PACKED_QUEUES, NULL }, + { "free-page-reporting", QEMU_CAPS_VIRTIO_BALLOON_FREE_PAGE_REPORTING, NULL }, }; diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 98d70cfa0e..6f39bf156f 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -580,6 +580,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */ /* 380 */ QEMU_CAPS_USB_HOST_HOSTDEVICE, /* -device usb-host.hostdevice */ + QEMU_CAPS_VIRTIO_BALLOON_FREE_PAGE_REPORTING, /*virtio balloon free-page-reporting */ QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index e9ba81d82f..f80efaf026 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3830,6 +3830,11 @@ qemuBuildMemballoonCommandLine(virCommandPtr cmd, virTristateSwitchTypeToString(def->memballoon->autodeflate)); } + if (def->memballoon->free_page_reporting != VIR_TRISTATE_SWITCH_ABSENT) { + virBufferAsprintf(&buf, ",free-page-reporting=%s", + virTristateSwitchTypeToString(def->memballoon->free_page_reporting)); + } + qemuBuildVirtioOptionsStr(&buf, def->memballoon->virtio); if (qemuCommandAddExtDevice(cmd, &def->memballoon->info) < 0) diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index a212605579..cfeb1e67c4 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -3926,6 +3926,13 @@ qemuValidateDomainDeviceDefMemballoon(const virDomainMemballoonDef *memballoon, return -1; } + if (memballoon->free_page_reporting != VIR_TRISTATE_SWITCH_ABSENT && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_BALLOON_FREE_PAGE_REPORTING)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("free-page-reporting is not supported by this QEMU binary")); + return -1; + } + if (qemuValidateDomainVirtioOptions(memballoon->virtio, qemuCaps) < 0) return -1; diff --git a/tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml index 987beb965e..1f1615b94b 100644 --- a/tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml @@ -243,6 +243,7 @@ <flag name='numa.hmat'/> <flag name='blockdev-hostdev-scsi'/> <flag name='usb-host.hostdevice'/> + <flag name='virtio-balloon-pci.free-page-reporting'/> <version>5001000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>43100242</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml index 3ee678ef8f..82f9051101 100644 --- a/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml @@ -243,6 +243,7 @@ <flag name='numa.hmat'/> <flag name='blockdev-hostdev-scsi'/> <flag name='usb-host.hostdevice'/> + <flag name='virtio-balloon-pci.free-page-reporting'/> <version>5001050</version> <kvmVersion>0</kvmVersion> <microcodeVersion>43100243</microcodeVersion> -- 2.18.4
On Fri, Oct 02, 2020 at 11:59:43 -0400, Nico Pache wrote:
xml: <devices> <memballoon model='virtio' free-page-reporting='on'/>
according to what you state in the cover-letter this is a 'virtio-balloon-pci' feature. We usually put stuff which depends on a specific model of the device into the <driver> subelement of the device element.
</devices>
qemu: qemu -device virtio-balloon-pci,...,free-page-reporting=on
This kernel feature allows for more stable and resource friendly systems.
Signed-off-by: Nico Pache <npache@redhat.com> ---
We require that changes are separated to smaller chunks according to the following logical boundaries:
docs/formatdomain.rst | 6 ++++++ docs/schemas/domaincommon.rng | 5 +++++ src/conf/domain_conf.c | 21 +++++++++++++++++++ src/conf/domain_conf.h | 1 +
Docs/RNG schema and parser/formatter needs to be separate
src/qemu/qemu_capabilities.c | 4 +++- src/qemu/qemu_capabilities.h | 1 +
.../caps_5.1.0.x86_64.xml | 1 + .../caps_5.2.0.x86_64.xml | 1 +
qemu caps changes should be separate
src/qemu/qemu_command.c | 5 +++++ src/qemu/qemu_validate.c | 7 +++++++
And qemu itself should be separate. In case you are going to add a NEWS.rst entry for this addition, any change to NEWS.rst also must be in a separate commit.
10 files changed, 51 insertions(+), 1 deletion(-)
diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index f3cf9e1fb3..f4c7765f5f 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -6755,6 +6755,12 @@ Example: manually added device with static PCI slot 2 requested release some memory at the last moment before a guest's process get killed by Out of Memory killer. :since:`Since 1.3.1, QEMU and KVM only`
+``free-page-reporting`` + The optional ``free-page-reporting`` attribute allows to enable/disable + ("on"/"off", respectively) the ability of the QEMU virtio memory balloon to + return unused pages back to the hypervisor to be used by other guests or + processes. :since:`Since 5.1, QEMU and KVM only`
There's a discrepancy between the feature name and the description. The feature name seems to suggest that you can query the amount of free pages somewhere, whereas the description states that it's actually returning the pages to the host. If the latter is true the feature should be named accordingly to prevent confusion. [...]
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 5dcfcd574d..88a2c8aacb 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -600,7 +600,8 @@ VIR_ENUM_IMPL(virQEMUCaps,
/* 380 */ "usb-host.hostdevice", - ); + "virtio-balloon-pci.free-page-reporting", + );
Plase don't change alignment of this brace.
typedef struct _virQEMUCapsMachineType virQEMUCapsMachineType;
[...]
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index a212605579..cfeb1e67c4 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -3926,6 +3926,13 @@ qemuValidateDomainDeviceDefMemballoon(const virDomainMemballoonDef *memballoon, return -1; }
+ if (memballoon->free_page_reporting != VIR_TRISTATE_SWITCH_ABSENT && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_BALLOON_FREE_PAGE_REPORTING)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("free-page-reporting is not supported by this QEMU binary")); + return -1; + }
Is it worth forbidding disabling the feature if it isn't supported?
+ if (qemuValidateDomainVirtioOptions(memballoon->virtio, qemuCaps) < 0) return -1;
The patch is also missing test XML addition for the qemuxml2argvtest and qemuxml2xmltest.
On 05.10.20 10:06, Peter Krempa wrote:
On Fri, Oct 02, 2020 at 11:59:43 -0400, Nico Pache wrote:
xml: <devices> <memballoon model='virtio' free-page-reporting='on'/>
according to what you state in the cover-letter this is a 'virtio-balloon-pci' feature. We usually put stuff which depends on a specific model of the device into the <driver> subelement of the device element.
IIRC, this should work for virtio-balloon-*, including virtio-balloon-ccw. -- Thanks, David / dhildenb
On Mon, Oct 05, 2020 at 11:11:54 +0200, David Hildenbrand wrote:
On 05.10.20 10:06, Peter Krempa wrote:
On Fri, Oct 02, 2020 at 11:59:43 -0400, Nico Pache wrote:
xml: <devices> <memballoon model='virtio' free-page-reporting='on'/>
according to what you state in the cover-letter this is a 'virtio-balloon-pci' feature. We usually put stuff which depends on a specific model of the device into the <driver> subelement of the device element.
IIRC, this should work for virtio-balloon-*, including virtio-balloon-ccw.
In that case the qemu capability detection should be added also for the ccw version and capability name needs to be modified to not include pci.
Thanks for the reviews! I'll get started on a V2 :) --Nico On Mon, Oct 5, 2020 at 5:32 AM Peter Krempa <pkrempa@redhat.com> wrote:
On Mon, Oct 05, 2020 at 11:11:54 +0200, David Hildenbrand wrote:
On 05.10.20 10:06, Peter Krempa wrote:
On Fri, Oct 02, 2020 at 11:59:43 -0400, Nico Pache wrote:
xml: <devices> <memballoon model='virtio' free-page-reporting='on'/>
according to what you state in the cover-letter this is a 'virtio-balloon-pci' feature. We usually put stuff which depends on a specific model of the device into the <driver> subelement of the device element.
IIRC, this should work for virtio-balloon-*, including virtio-balloon-ccw.
In that case the qemu capability detection should be added also for the ccw version and capability name needs to be modified to not include pci.
On Mon, Oct 05, 2020 at 10:06:20AM +0200, Peter Krempa wrote:
On Fri, Oct 02, 2020 at 11:59:43 -0400, Nico Pache wrote:
[...]
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index a212605579..cfeb1e67c4 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -3926,6 +3926,13 @@ qemuValidateDomainDeviceDefMemballoon(const virDomainMemballoonDef *memballoon, return -1; }
+ if (memballoon->free_page_reporting != VIR_TRISTATE_SWITCH_ABSENT && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_BALLOON_FREE_PAGE_REPORTING)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("free-page-reporting is not supported by this QEMU binary")); + return -1; + }
Is it worth forbidding disabling the feature if it isn't supported?
I would say that explicit usage of a feature that is not available in QEMU should result in error. Pavel
participants (4)
-
David Hildenbrand -
Nico Pache -
Pavel Hrdina -
Peter Krempa