[PATCH 1/1] Optional free-page-reporting can be enabled/disabled for memballon device of model 'virtio'.

[Peter Krempa]

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 at 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.