[PATCH Libvirt 02/11] libvirt: Add virDomainSetVcpuDirtyLimit API

[Peter Krempa]

On Tue, Aug 02, 2022 at 22:13:40 +0800, ~hyman wrote:
> From: Hyman Huang(黄勇) <yong.huang at smartx.com>
> 
> Introduce virDomainSetVcpuDirtyLimit API to set upper limit
> of dirty page rate.
> 
> Signed-off-by: Hyman Huang(黄勇) <yong.huang at smartx.com>
> ---
>  include/libvirt/libvirt-domain.h | 16 ++++++++++
>  src/driver-hypervisor.h          |  7 +++++
>  src/libvirt-domain.c             | 54 ++++++++++++++++++++++++++++++++
>  src/libvirt_public.syms          |  5 +++
>  src/remote/remote_driver.c       |  1 +
>  src/remote/remote_protocol.x     | 15 ++++++++-
>  src/remote_protocol-structs      |  7 +++++
>  7 files changed, 104 insertions(+), 1 deletion(-)

[...]

> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index ec42bb9a53..878d2a6d8c 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -14057,5 +14057,59 @@ virDomainFDAssociate(virDomainPtr domain,
>  
>   error:
>      virDispatchError(conn);
> +}
> +
> +/**
> + * virDomainSetVcpuDirtyLimit:
> + * @domain: pointer to domain object, or NULL for Domain0

This API certainly will not work with Domain0 as it's not being
implemented for XEN, so please remove that part.

> + * @vcpu: mandatory parameter only if the specified index of the
> + *        virtual CPU is limited; ignored otherwise.
> + * @rate: upper limit of dirty page rate (MB/s) for virtual CPUs
> + * @flags: bitwise-OR of supported virDomainDirtyLimitFlags
> + *
> + * Dynamically set the upper dirty page rate limit of the virtual CPUs.

While you've arbitrarily chose to not implement persisting of this
configuration into the XML, it certainly is a possibility that this
might be useful. As of such you should add the appropriate flags (
VIR_DOMAIN_AFFECT_CURRENT/VIR_DOMAIN_AFFECT_LIVE/VIR_DOMAIN_AFFECT_CONFIG)
(Note that these take up the first two bits of flags).

You then add logic that allows the API only when the VM is live and live
update is requested later in the code implementing it.

That way the API will stay flexible.

Additionally the description of what this actually does doesn't feel
sufficient. Please enhance the documentation to clearly state what's
going on and what impact it has on the VM itself.

Also what is the reason for expressing 'rate' as MB/s? Generally bytes/s
have definitely enough range in a 64bit variable and you don't get into
trouble of having to check it when you need to multiply up to bytes/s.

You can add a disclaimer that hypervisors are free to round up/down to
the nearest mebibyte/s


> + *
> + * Returns 0 in case of success, -1 in case of failure.
> + *
> + * Since: 9.6.0
> + */
> +int
> +virDomainSetVcpuDirtyLimit(virDomainPtr domain,
> +                           int vcpu,
> +                           unsigned long long rate,
> +                           unsigned int flags)
> +{
> +    virConnectPtr conn;
> +
> +    VIR_DOMAIN_DEBUG(domain, "vcpu=%d, dirty page rate limit=%lld",

'rate' is _unsigned_, but the format substitution calls for 'lld'.

> +                     vcpu, rate);

'flags' are not logged.

> +
> +    virCheckFlags(VIR_DOMAIN_DIRTYLIMIT_VCPU |
> +                  VIR_DOMAIN_DIRTYLIMIT_ALL, -1);

None other API uses virCheckFlags in this file. It's used in the
implementation. Additionally this would skip dispatching the error in
the first place.

> +
> +    virResetLastError();
> +
> +    virCheckDomainReturn(domain, -1);
> +    conn = domain->conn;
> +
> +    virCheckReadOnlyGoto(conn->flags, error);
> +    virCheckPositiveArgGoto(rate, error);

Since you request 'rate' to be positive, maybe you can use '0' as a
special case to cancel the limit instead of adding a new api.

> +
> +    VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_DIRTYLIMIT_VCPU,
> +                             VIR_DOMAIN_DIRTYLIMIT_ALL,
> +                             error);
> +
> +    if (conn->driver->domainSetVcpuDirtyLimit) {
> +        int ret;
> +        ret = conn->driver->domainSetVcpuDirtyLimit(domain, vcpu, rate, flags);
> +        if (ret < 0)
> +            goto error;
> +        return ret;
> +    }
> +
> +    virReportUnsupportedError();
> +
> + error:
> +    virDispatchError(domain->conn);
>      return -1;
>  }