[prev in list] [next in list] [prev in thread] [next in thread] 

List:       libvir-list
Subject:    Re: [PATCH Libvirt 03/11] qemu_driver: Implement qemuDomainSetVcpuDirtyLimit
From:       Peter Krempa <pkrempa () redhat ! com>
Date:       2023-07-31 8:19:37
Message-ID: ZMdumYwFgVwzWmqx () angien ! pipo ! sk
[Download RAW message or body]

On Wed, Aug 03, 2022 at 00:27:47 +0800, ~hyman wrote:
> From: Hyman Huang(黄勇) <yong.huang@smartx.com>
> 
> Implement qemuDomainSetVcpuDirtyLimit which set dirty page
> rate upper limit.
> 
> Signed-off-by: Hyman Huang(黄勇) <yong.huang@smartx.com>
> ---
>  src/qemu/qemu_driver.c       | 52 ++++++++++++++++++++++++++++++++++++
>  src/qemu/qemu_monitor.c      | 13 +++++++++
>  src/qemu/qemu_monitor.h      |  5 ++++
>  src/qemu/qemu_monitor_json.c | 45 +++++++++++++++++++++++++++++++
>  src/qemu/qemu_monitor_json.h |  5 ++++
>  5 files changed, 120 insertions(+)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 497923ffee..61b992fc51 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -19912,6 +19912,57 @@ qemuDomainFDAssociate(virDomainPtr domain,
>      return ret;
>  }
>  
> +static int
> +qemuDomainSetVcpuDirtyLimit(virDomainPtr domain,
> +                            int vcpu,
> +                            unsigned long long rate,
> +                            unsigned int flags)
> +{
> +    virDomainObj *vm = NULL;
> +    qemuDomainObjPrivate *priv;
> +    int ret = -1;
> +
> +    if (!(vm = qemuDomainObjFromDomain(domain)))
> +        return -1;
> +
> +    if (virDomainSetVcpuDirtyLimitEnsureACL(domain->conn, vm->def))
> +        goto cleanup;
> +
> +    priv = vm->privateData;
> +    if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VCPU_DIRTY_LIMIT)) {

note that priv->qemuCaps is NULL when the VM is not running, so you'll
get ...

> +        virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> +                       _("QEMU does not support setting dirty page rate limit"));

... this error.

> +        goto cleanup;
> +    }
> +
> +    if (virDomainObjBeginJob(vm, VIR_JOB_MODIFY) < 0)
> +        goto cleanup;
> +
> +    if (virDomainObjCheckActive(vm) < 0)

Instead of this one. You need to reorder these, but that'll be needed
regardless when you add the logic for handlign the
virDomainModificationImpact flags.

> +        goto endjob;
> +
> +    qemuDomainObjEnterMonitor(vm);
> +    if (flags & VIR_DOMAIN_DIRTYLIMIT_VCPU) {
> +        if (vcpu < 0) {
> +           virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> +                          _("Require cpu index to limit dirty page rate"));


So is there a need to pass 'vcpu' as 'int' rather than unsigned int?

> +           goto endjob;

This skips over the call to qemuDomainObjExitMonitor;

> +        }
> +        ret = qemuMonitorSetVcpuDirtyLimit(priv->mon, vcpu, rate);
> +        VIR_DEBUG("Set vcpu[%d] dirty page rate limit %lld", vcpu, rate);
> +    } else {
> +        ret = qemuMonitorSetVcpuDirtyLimit(priv->mon, -1, rate);
> +        VIR_DEBUG("Set all vcpus dirty page rate limit %lld of vm", rate);
> +    }
> +    qemuDomainObjExitMonitor(vm);
> +
> + endjob:
> +    virDomainObjEndJob(vm);
> +
> + cleanup:
> +    virDomainObjEndAPI(&vm);
> +    return ret;
> +}
>  
>  static virHypervisorDriver qemuHypervisorDriver = {
>      .name = QEMU_DRIVER_NAME,
> @@ -20162,6 +20213,7 @@ static virHypervisorDriver qemuHypervisorDriver = {
>      .domainStartDirtyRateCalc = qemuDomainStartDirtyRateCalc, /* 7.2.0 */
>      .domainSetLaunchSecurityState = qemuDomainSetLaunchSecurityState, /* 8.0.0 */
>      .domainFDAssociate = qemuDomainFDAssociate, /* 9.0.0 */
> +    .domainSetVcpuDirtyLimit = qemuDomainSetVcpuDirtyLimit, /* 9.6.0 */
>  };
>  
>  
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index c680c4b804..87926edec6 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -4505,3 +4505,16 @@ qemuMonitorGetStatsByQOMPath(virJSONValue *arr,
>  
>      return NULL;
>  }
> +
> +
> +int
> +qemuMonitorSetVcpuDirtyLimit(qemuMonitor *mon,
> +                             int vcpu,
> +                             unsigned long long rate)
> +{
> +    VIR_DEBUG("set vcpu %d dirty page rate limit %lld", vcpu, rate);

'rate' is unsigned.

> +
> +    QEMU_CHECK_MONITOR(mon);
> +
> +    return qemuMonitorJSONSetVcpuDirtyLimit(mon, vcpu, rate);
> +}
> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> index 6c590933aa..07a05365cf 100644
> --- a/src/qemu/qemu_monitor.h
> +++ b/src/qemu/qemu_monitor.h
> @@ -1579,3 +1579,8 @@ qemuMonitorExtractQueryStats(virJSONValue *info);
>  virJSONValue *
>  qemuMonitorGetStatsByQOMPath(virJSONValue *arr,
>                               char *qom_path);
> +
> +int
> +qemuMonitorSetVcpuDirtyLimit(qemuMonitor *mon,
> +                             int vcpu,
> +                             unsigned long long rate);
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index d9e9a4481c..732366ab29 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -8889,3 +8889,48 @@ qemuMonitorJSONQueryStats(qemuMonitor *mon,
>  
>      return virJSONValueObjectStealArray(reply, "return");
>  }
> +
> +/**
> + * qemuMonitorJSONSetVcpuDirtyLimit:
> + * @mon: monitor object
> + * @vcpu: virtual cpu index to be set, -1 means all virtual cpus
> + * @rate: dirty page rate upper limit to be set
> + *
> + * Returns -1 on failure.
> + */
> +int
> +qemuMonitorJSONSetVcpuDirtyLimit(qemuMonitor *mon,
> +                                 int vcpu,
> +                                 unsigned long long rate)
> +{
> +    g_autoptr(virJSONValue) cmd = NULL;
> +    g_autoptr(virJSONValue) reply = NULL;
> +
> +    if (vcpu < -1)
> +        return -1;

This doesn't report an error, why everything else does. Also this
doesn't really seem to be needed.

> +
> +    if (vcpu >= 0) {
> +        /* set vcpu dirty page rate limit */
> +        if (!(cmd = qemuMonitorJSONMakeCommand("set-vcpu-dirty-limit",
> +                                               "i:cpu-index", vcpu,
> +                                               "U:dirty-rate", rate,
> +                                               NULL))) {
> +            return -1;
> +        }
> +    } else if (vcpu == -1) {
> +        /* set vm dirty page rate limit */
> +         if (!(cmd = qemuMonitorJSONMakeCommand("set-vcpu-dirty-limit",
> +                                               "U:dirty-rate", rate,
> +                                               NULL))) {

Since both use the same command, you can use e.g. 'k:cpu-index':

  * k: signed integer value, omitted if negative 

And get rid of the whole if/elseif thing.

> +            return -1;
> +        }
> +    }
> +
> +    if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0)
> +        return -1;
> +
> +    if (qemuMonitorJSONCheckError(cmd, reply) < 0)
> +        return -1;
> +
> +    return 0;
> +}
> diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
> index 06023b98ea..89f61b3052 100644
> --- a/src/qemu/qemu_monitor_json.h
> +++ b/src/qemu/qemu_monitor_json.h
> @@ -825,3 +825,8 @@ qemuMonitorJSONQueryStats(qemuMonitor *mon,
>                            qemuMonitorQueryStatsTargetType target,
>                            char **vcpus,
>                            GPtrArray *providers);
> +
> +int
> +qemuMonitorJSONSetVcpuDirtyLimit(qemuMonitor *mon,
> +                                 int vcpu,
> +                                 unsigned long long rate);
> -- 
> 2.38.5
> 

[prev in list] [next in list] [prev in thread] [next in thread]