Sorry for the late reply.
On Mon, Jul 31, 2023 at 4:11 PM Peter Krempa <pkre...@redhat.com> wrote:
> On Tue, Aug 02, 2022 at 22:13:40 +0800, ~hyman wrote:
> > From: Hyman Huang(黄勇) <yong.hu...@smartx.com>
> >
> > Introduce virDomainSetVcpuDirtyLimit API to set upper limit
> > of dirty page rate.
> >
> > Signed-off-by: Hyman Huang(黄勇) <yong.hu...@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.
>
Ok
>
> > + * @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
>
I'm also tangled in it when writing the first version, so you prefer to
implement persisting. I'll introduce the limit info in the
struct _virDomainVcpuDef to track the state and enforce the
dirty limit setup after launching VM next version, what do you think?
Like the 'virsh setvcpu' does.
> 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).
>
Ok, get it, introduce it in the next version.
>
> 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.
OK, I'll enrich the comment.
>
> 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.
>
Get it.
>
> > +
> > + 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.
>
Yes, this simplifies the implementation absolutely, I'll drop the
cancellation
API and reuse this in the next version.
>
> > +
> > + 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;
> > }
>
>
--
Best regards
