On 11/7/20 10:41 AM, Hao Wang wrote: > Introduce virDomainDirtyRateInfo structure used for domain's memory dirty rate query. > > Signed-off-by: Hao Wang <wanghao232(a)huawei.com&gt; > Reviewed-by: Chuan Zheng <zhengchuan(a)huawei.com&gt; > --- > include/libvirt/libvirt-domain.h | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h > index b3310729bf..145f517068 100644 > --- a/include/libvirt/libvirt-domain.h > +++ b/include/libvirt/libvirt-domain.h > @@ -5096,4 +5096,28 @@ int virDomainBackupBegin(virDomainPtr domain, > char *virDomainBackupGetXMLDesc(virDomainPtr domain, > unsigned int flags); > +/** > + * virDomainDirtyRateInfo: > + * > + * a virDomainDirtyRateInfo is a structure filled by virDomainGetDirtyRate() and > + * extracting dirty rate infomation for a given active Domain. > + */ > + > +typedef struct _virDomainDirtyRateInfo virDomainDirtyRateInfo; > + > +struct _virDomainDirtyRateInfo { > + int status; /* the status of dirtyrate calculation */ > + long long dirtyRate; /* the dirtyrate in MB/s */ > + long long startTime; /* the start time of dirtyrate calculation */ > + long long calcTime; /* the period of dirtyrate calculation */ > +}; > + > +/** > + * virDomainDirtyRateInfoPtr: > + * > + * a virDomainDirtyRateInfoPtr is a pointer to a virDomainDirtyRateInfo structure. > + */ > + > +typedef virDomainDirtyRateInfo *virDomainDirtyRateInfoPtr; > + > #endif /* LIBVIRT_DOMAIN_H */ > This should be squashed with 2/7 and bits of 5/7 and 6/7 which modify this file. The key here is that public API should go into one patch, driver implementation into other - it's easier to backport patches this way.

On 11/7/20 10:41 AM, Hao Wang wrote: > Introduce DomainGetDirtyRateInfo API for domain's memory dirty rate > calculation and query. > > Signed-off-by: Hao Wang <wanghao232(a)huawei.com&gt; > Signed-off-by: Zhou Yimin <zhouyimin(a)huawei.com&gt; > Reviewed-by: Chuan Zheng <zhengchuan(a)huawei.com&gt; > --- >   include/libvirt/libvirt-domain.h |  5 ++++ >   src/driver-hypervisor.h          |  7 +++++ >   src/libvirt-domain.c             | 46 ++++++++++++++++++++++++++++++++ >   src/libvirt_public.syms          |  5 ++++ >   src/remote/remote_driver.c       |  1 + >   src/remote/remote_protocol.x     | 21 ++++++++++++++- >   6 files changed, 84 insertions(+), 1 deletion(-) > > diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h > index 145f517068..b950736b67 100644 > --- a/include/libvirt/libvirt-domain.h > +++ b/include/libvirt/libvirt-domain.h > @@ -5120,4 +5120,9 @@ struct _virDomainDirtyRateInfo { >     typedef virDomainDirtyRateInfo *virDomainDirtyRateInfoPtr; >   +int virDomainGetDirtyRateInfo(virDomainPtr domain, > +                              virDomainDirtyRateInfoPtr info, > +                              long long sec, > +                              unsigned int flags); > + >   #endif /* LIBVIRT_DOMAIN_H */ > diff --git a/src/driver-hypervisor.h b/src/driver-hypervisor.h > index bce023017d..a77c29de54 100644 > --- a/src/driver-hypervisor.h > +++ b/src/driver-hypervisor.h > @@ -1387,6 +1387,12 @@ typedef char * >   (*virDrvDomainBackupGetXMLDesc)(virDomainPtr domain, >                                   unsigned int flags); >   +typedef int > +(*virDrvDomainGetDirtyRateInfo)(virDomainPtr domain, > +                                virDomainDirtyRateInfoPtr info, > +                                long long sec, > +                                unsigned int flags); > + >   typedef struct _virHypervisorDriver virHypervisorDriver; >   typedef virHypervisorDriver *virHypervisorDriverPtr; >   @@ -1650,4 +1656,5 @@ struct _virHypervisorDriver { >       virDrvDomainAgentSetResponseTimeout domainAgentSetResponseTimeout; >       virDrvDomainBackupBegin domainBackupBegin; >       virDrvDomainBackupGetXMLDesc domainBackupGetXMLDesc; > +    virDrvDomainGetDirtyRateInfo domainGetDirtyRateInfo; >   }; > diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c > index 3c5f55176a..ce3c40edf8 100644 > --- a/src/libvirt-domain.c > +++ b/src/libvirt-domain.c > @@ -12758,3 +12758,49 @@ virDomainBackupGetXMLDesc(virDomainPtr domain, >       virDispatchError(conn); >       return NULL; >   } > + > + > +/** > + * virDomainGetDirtyRateInfo: > + * @domain: a domain object. > + * @info: return value of current domain's memory dirty rate info. > + * @sec: show dirty rate within specified seconds. > + * @flags: the flags of getdirtyrate action -- calculate and/or query. What are the flags? Which enum should I look at? > + * > + * Get the current domain's memory dirty rate (in MB/s). Can you expand on this a bit? Look at description to other APIs for inspiration. > + * > + * Returns 0 in case of success, -1 otherwise. > + */ > +int > +virDomainGetDirtyRateInfo(virDomainPtr domain, > +                          virDomainDirtyRateInfoPtr info, > +                          long long sec, Do we really need long long? That's more than 2.9*10^11 years. I don't think any virtual machine will ever run for so long (considering that the age of the universe is ~1.38*10^10 years) > +                          unsigned int flags) > +{ > +    virConnectPtr conn; > + > +    VIR_DOMAIN_DEBUG(domain, "info = %p, seconds=%lld", info, sec); @flags should also be logged. > + > +    virResetLastError(); > + > +    virCheckDomainReturn(domain, -1); > +    conn = domain->conn; > + > +    virCheckNonNullArgGoto(info, error); > +    virCheckReadOnlyGoto(conn->flags, error); > + > +    if (info) > +        memset(info, 0, sizeof(*info)); @info was checked for NULL just two lines above. > + > +    if (conn->driver->domainGetDirtyRateInfo) { > +        if (conn->driver->domainGetDirtyRateInfo(domain, info, sec, flags) < 0) > +            goto error; > +        VIR_DOMAIN_DEBUG(domain, "info = %p, seconds=%lld", info, sec); This is not very useful debug log. > +        return 0; > +    } > + > +    virReportUnsupportedError(); > + error: > +    virDispatchError(conn); > +    return -1; > +} > diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms > index 539d2e3943..11864f48b1 100644 > --- a/src/libvirt_public.syms > +++ b/src/libvirt_public.syms > @@ -873,4 +873,9 @@ LIBVIRT_6.0.0 { >           virDomainBackupGetXMLDesc; >   } LIBVIRT_5.10.0; >   +LIBVIRT_6.9.0 { > +    global: > +        virDomainGetDirtyRateInfo; > +} LIBVIRT_6.0.0; > + Unfortunately, this missed 6.9.0 so this must be 6.10.0 instead. >   # .... define new API here using predicted next version number .... > diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c > index 9cd2fd36ae..325968a22b 100644 > --- a/src/remote/remote_driver.c > +++ b/src/remote/remote_driver.c > @@ -8458,6 +8458,7 @@ static virHypervisorDriver hypervisor_driver = { >       .domainAgentSetResponseTimeout = remoteDomainAgentSetResponseTimeout, /* 5.10.0 */ >       .domainBackupBegin = remoteDomainBackupBegin, /* 6.0.0 */ >       .domainBackupGetXMLDesc = remoteDomainBackupGetXMLDesc, /* 6.0.0 */ > +    .domainGetDirtyRateInfo = remoteDomainGetDirtyRateInfo, /* 6.9.0 */ Same here. >   }; >     static virNetworkDriver network_driver = { I think the following should be squashed in: diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index e8bd890b29..594d7a2790 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -5096,28 +5096,18 @@ int virDomainBackupBegin(virDomainPtr domain,  char *virDomainBackupGetXMLDesc(virDomainPtr domain,                                  unsigned int flags); -/** - * virDomainDirtyRateFlags: - * - * Details on the flags used by getdirtyrate api. - */ - -typedef enum { -    VIR_DOMAIN_DIRTYRATE_CALC = 1 << 0,  /* calculate domain's dirtyrate */ -    VIR_DOMAIN_DIRTYRATE_QUERY = 1 << 1, /* query domain's dirtyrate */ -} virDomainDirtyRateFlags; -  /**   * virDomainDirtyRateStatus:   * - * Details on the cause of a dirtyrate calculation status. + * Details on the cause of a dirty rate calculation status.   */ -  typedef enum { -    VIR_DOMAIN_DIRTYRATE_UNSTARTED = 0, /* the dirtyrate calculation has not been started */ -    VIR_DOMAIN_DIRTYRATE_MEASURING = 1, /* the dirtyrate calculation is measuring */ +    VIR_DOMAIN_DIRTYRATE_UNSTARTED = 0, /* the dirtyrate calculation has not +                                           been started */ +    VIR_DOMAIN_DIRTYRATE_MEASURING = 1, /* the dirtyrate calculation is +                                           measuring */      VIR_DOMAIN_DIRTYRATE_MEASURED  = 2, /* the dirtyrate calculation is -completed */ +                                           completed */  # ifdef VIR_ENUM_SENTINELS      VIR_DOMAIN_DIRTYRATE_LAST @@ -5133,12 +5123,23 @@ completed */  typedef struct _virDomainDirtyRateInfo virDomainDirtyRateInfo;  typedef virDomainDirtyRateInfo *virDomainDirtyRateInfoPtr;  struct _virDomainDirtyRateInfo { -    int status;             /* the status of dirty rate calculation */ +    int status;             /* the status of dirty rate calculation, one of +                               virDomainDirtyRateStatus */      long long dirtyRate;    /* the dirty rate in MB/s */      long long startTime;    /* the start time of dirty rate calculation */      long long calcTime;     /* the period of dirty rate calculation */  }; +/** + * virDomainDirtyRateFlags: + * + * Details on the flags used by getdirtyrate api. + */ +typedef enum { +    VIR_DOMAIN_DIRTYRATE_CALC = 1 << 0,  /* calculate domain's dirty rate */ +    VIR_DOMAIN_DIRTYRATE_QUERY = 1 << 1, /* query domain's dirty rate */ +} virDomainGetDirtyRateInfoFlags; +  int virDomainGetDirtyRateInfo(virDomainPtr domain,                                virDomainDirtyRateInfoPtr info,                                long long sec, diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index ce3c40edf8..75029b0a4b 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -12762,10 +12762,10 @@ virDomainBackupGetXMLDesc(virDomainPtr domain,  /**   * virDomainGetDirtyRateInfo: - * @domain: a domain object. - * @info: return value of current domain's memory dirty rate info. - * @sec: show dirty rate within specified seconds. - * @flags: the flags of getdirtyrate action -- calculate and/or query. + * @domain: a domain object + * @info: pointer to current domain's memory dirty rate info + * @sec: show dirty rate within specified seconds + * @flags: extra flags; binary-OR of virDomainGetDirtyRateInfoFlags   *   * Get the current domain's memory dirty rate (in MB/s).   * @@ -12779,7 +12779,8 @@ virDomainGetDirtyRateInfo(virDomainPtr domain,  {      virConnectPtr conn; -    VIR_DOMAIN_DEBUG(domain, "info = %p, seconds=%lld", info, sec); +    VIR_DOMAIN_DEBUG(domain, "info=%p, sec=%lld, flags=0x%x", +                     info, sec, flags);      virResetLastError(); @@ -12789,14 +12790,14 @@ virDomainGetDirtyRateInfo(virDomainPtr domain,      virCheckNonNullArgGoto(info, error);      virCheckReadOnlyGoto(conn->flags, error); -    if (info) -        memset(info, 0, sizeof(*info)); +    memset(info, 0, sizeof(*info));      if (conn->driver->domainGetDirtyRateInfo) { -        if (conn->driver->domainGetDirtyRateInfo(domain, info, sec, flags) < 0) +        int ret; +        ret = conn->driver->domainGetDirtyRateInfo(domain, info, sec, flags); +        if (ret < 0)              goto error; -        VIR_DOMAIN_DEBUG(domain, "info = %p, seconds=%lld", info, sec); -        return 0; +        return ret;      }      virReportUnsupportedError(); Michal .

Implement qemuDomainCalculateDirtyRate which calculates domain's memory dirty rate calling qmp "calc-dirty-rate". Signed-off-by: Hao Wang <wanghao232(a)huawei.com&gt; Signed-off-by: Zhou Yimin <zhouyimin(a)huawei.com&gt; Reviewed-by: Chuan Zheng <zhengchuan(a)huawei.com&gt; --- src/qemu/qemu_migration.c | 28 ++++++++++++++++++++++++++++ src/qemu/qemu_migration.h | 5 +++++ src/qemu/qemu_monitor.c | 12 ++++++++++++ src/qemu/qemu_monitor.h | 4 ++++ src/qemu/qemu_monitor_json.c | 22 ++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 4 ++++ 6 files changed, 75 insertions(+) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 6f764b0c73..8029e24415 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -5836,3 +5836,31 @@ qemuMigrationSrcFetchMirrorStats(virQEMUDriverPtr driver, virHashFree(blockinfo); return 0; } + + +int +qemuDomainCalculateDirtyRate(virDomainPtr dom,

+ virDomainObjPtr vm, + long long sec) +{ + virQEMUDriverPtr driver = dom->conn->privateData; + qemuDomainObjPrivatePtr priv; + int ret = -1; + + if (!virDomainObjIsActive(vm)) { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("domain is not running")); + return ret; + } + + priv = vm->privateData;

+ + VIR_DEBUG("Calculate dirty rate during %lld seconds", sec); + qemuDomainObjEnterMonitor(driver, vm); + + ret = qemuMonitorCalculateDirtyRate(priv->mon, sec); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + ret = -1; + + return ret; +} diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h index fd9eb7cab0..0522b375c0 100644 --- a/src/qemu/qemu_migration.h +++ b/src/qemu/qemu_migration.h @@ -258,3 +258,8 @@ qemuMigrationSrcFetchMirrorStats(virQEMUDriverPtr driver, virDomainObjPtr vm, qemuDomainAsyncJob asyncJob, qemuDomainJobInfoPtr jobInfo); + +int +qemuDomainCalculateDirtyRate(virDomainPtr dom, + virDomainObjPtr vm, + long long sec); diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 5481bd99a0..06603b8691 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -4769,3 +4769,15 @@ qemuMonitorTransactionBackup(virJSONValuePtr actions, return qemuMonitorJSONTransactionBackup(actions, device, jobname, target, bitmap, syncmode); } + + +int +qemuMonitorCalculateDirtyRate(qemuMonitorPtr mon, + long long sec) +{ + VIR_DEBUG("seconds=%lld", sec); + + QEMU_CHECK_MONITOR(mon); + + return qemuMonitorJSONCalculateDirtyRate(mon, sec); +} diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 54fbb41ef7..afb97780cf 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -1527,3 +1527,7 @@ qemuMonitorTransactionBackup(virJSONValuePtr actions, const char *target, const char *bitmap, qemuMonitorTransactionBackupSyncMode syncmode); + +int +qemuMonitorCalculateDirtyRate(qemuMonitorPtr mon, + long long sec); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 843a555952..65691522fb 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -9635,3 +9635,25 @@ qemuMonitorJSONGetCPUMigratable(qemuMonitorPtr mon, return virJSONValueGetBoolean(virJSONValueObjectGet(reply, "return"), migratable); } + + +int +qemuMonitorJSONCalculateDirtyRate(qemuMonitorPtr mon, + long long sec) +{ + g_autoptr(virJSONValue) cmd = NULL; + g_autoptr(virJSONValue) reply = NULL; + + if (!(cmd = qemuMonitorJSONMakeCommand("calc-dirty-rate", + "I:calc-time", (long)sec,

+ NULL))) + 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 38e23ef3c5..c9556fc19a 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -710,3 +710,7 @@ int qemuMonitorJSONSetDBusVMStateIdList(qemuMonitorPtr mon, int qemuMonitorJSONGetCPUMigratable(qemuMonitorPtr mon, bool *migratable); + +int +qemuMonitorJSONCalculateDirtyRate(qemuMonitorPtr mon, + long long sec);

On 11/7/20 10:41 AM, Hao Wang wrote: > Implement qemuDomainQueryDirtyRate which query domain's memory > dirty rate calling qmp "query-dirty-rate". > > Signed-off-by: Hao Wang <wanghao232(a)huawei.com&gt; > Signed-off-by: Zhou Yimin <zhouyimin(a)huawei.com&gt; > Reviewed-by: Chuan Zheng <zhengchuan(a)huawei.com&gt; > --- >   src/qemu/qemu_migration.c    | 31 +++++++++++++++++++++++++++++++ >   src/qemu/qemu_migration.h    |  5 +++++ >   src/qemu/qemu_monitor.c      | 12 ++++++++++++ >   src/qemu/qemu_monitor.h      |  4 ++++ >   src/qemu/qemu_monitor_json.c | 22 ++++++++++++++++++++++ >   src/qemu/qemu_monitor_json.h |  4 ++++ >   6 files changed, 78 insertions(+) > > diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c > index 8029e24415..3d07ba3ac4 100644 > --- a/src/qemu/qemu_migration.c > +++ b/src/qemu/qemu_migration.c > @@ -5864,3 +5864,34 @@ qemuDomainCalculateDirtyRate(virDomainPtr dom, >         return ret; >   } > + > + > +int > +qemuDomainQueryDirtyRate(virDomainPtr dom, > +                         virDomainObjPtr vm, > +                         virDomainDirtyRateInfoPtr info) > +{ > +    virQEMUDriverPtr driver = dom->conn->privateData; Again, driver is accessible from the caller, just pass it directly. > +    qemuDomainObjPrivatePtr priv; > +    int ret = -1; > + > +    if (!virDomainObjIsActive(vm)) { > +        virReportError(VIR_ERR_OPERATION_INVALID, > +                       "%s", _("domain is not running")); > +        return ret; > +    } > + > +    priv = vm->privateData; > + > +    qemuDomainObjEnterMonitor(driver, vm); > + > +    ret = qemuMonitorQueryDirtyRate(priv->mon, info); > +    if (ret < 0) { > +        virReportError(VIR_ERR_OPERATION_FAILED, > +                       "%s", _("get vm's dirty rate failed.")); No, this is not correct, qemuMonitorQueryDirtyRate() will report an error if something goes wrong. And with pretty accurate error message. This overwrites it with more generic one. > +    } > +    if (qemuDomainObjExitMonitor(driver, vm) < 0) > +        ret = -1; > + > +    return ret; > +} > diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h > index 0522b375c0..8baae512b7 100644 > --- a/src/qemu/qemu_migration.h > +++ b/src/qemu/qemu_migration.h > @@ -263,3 +263,8 @@ int >   qemuDomainCalculateDirtyRate(virDomainPtr dom, >                                virDomainObjPtr vm, >                                long long sec); > + > +int > +qemuDomainQueryDirtyRate(virDomainPtr dom, > +                         virDomainObjPtr vm, > +                         virDomainDirtyRateInfoPtr info); > diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c > index 06603b8691..2fa6879467 100644 > --- a/src/qemu/qemu_monitor.c > +++ b/src/qemu/qemu_monitor.c > @@ -4781,3 +4781,15 @@ qemuMonitorCalculateDirtyRate(qemuMonitorPtr mon, >         return qemuMonitorJSONCalculateDirtyRate(mon, sec); >   } > + > + > +int > +qemuMonitorQueryDirtyRate(qemuMonitorPtr mon, > +                          virDomainDirtyRateInfoPtr info) > +{ > +    VIR_DEBUG("info=%p", info); > + > +    QEMU_CHECK_MONITOR(mon); > + > +    return qemuMonitorJSONQueryDirtyRate(mon, info); > +} > diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h > index afb97780cf..25105c3ad9 100644 > --- a/src/qemu/qemu_monitor.h > +++ b/src/qemu/qemu_monitor.h > @@ -1531,3 +1531,7 @@ qemuMonitorTransactionBackup(virJSONValuePtr actions, >   int >   qemuMonitorCalculateDirtyRate(qemuMonitorPtr mon, >                                 long long sec); > + > +int > +qemuMonitorQueryDirtyRate(qemuMonitorPtr mon, > +                          virDomainDirtyRateInfoPtr info); > diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c > index 65691522fb..1924c7229b 100644 > --- a/src/qemu/qemu_monitor_json.c > +++ b/src/qemu/qemu_monitor_json.c > @@ -9657,3 +9657,25 @@ qemuMonitorJSONCalculateDirtyRate(qemuMonitorPtr mon, >         return 0; >   } > + > + > +int > +qemuMonitorJSONQueryDirtyRate(qemuMonitorPtr mon, > +                              virDomainDirtyRateInfoPtr info) > +{ > +    g_autoptr(virJSONValue) cmd = NULL; > +    g_autoptr(virJSONValue) reply = NULL; > + > +    if (!(cmd = qemuMonitorJSONMakeCommand("query-dirty-rate", NULL))) > +        return -1; > + > +    if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) > +        return -1; > + > +    if (qemuMonitorJSONCheckError(cmd, reply) < 0) > +        return -1; > + > +    /* TODO: extract dirtyrate data from reply and store in virDomainDirtyRateInfoPtr */ How about instead of noting down to finish this, squash 5/7 here? It's not like somebody could backport only this or that patch. > +    info->status = 0; > +    return 0; > +} > diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h > index c9556fc19a..487f2e6e58 100644 > --- a/src/qemu/qemu_monitor_json.h > +++ b/src/qemu/qemu_monitor_json.h > @@ -714,3 +714,7 @@ qemuMonitorJSONGetCPUMigratable(qemuMonitorPtr mon, >   int >   qemuMonitorJSONCalculateDirtyRate(qemuMonitorPtr mon, >                                     long long sec); > + > +int > +qemuMonitorJSONQueryDirtyRate(qemuMonitorPtr mon, > +                              virDomainDirtyRateInfoPtr info); > I think the following should be squashed in: diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 5a253bbe2f..c0df2589da 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -5864,29 +5864,21 @@ qemuDomainCalculateDirtyRate(virQEMUDriverPtr driver,  int -qemuDomainQueryDirtyRate(virDomainPtr dom, +qemuDomainQueryDirtyRate(virQEMUDriverPtr driver,                           virDomainObjPtr vm,                           virDomainDirtyRateInfoPtr info)  { -    virQEMUDriverPtr driver = dom->conn->privateData; -    qemuDomainObjPrivatePtr priv; -    int ret = -1; +    qemuDomainObjPrivatePtr priv = vm->privateData; +    int ret;      if (!virDomainObjIsActive(vm)) {          virReportError(VIR_ERR_OPERATION_INVALID,                         "%s", _("domain is not running")); -        return ret; +        return -1;      } -    priv = vm->privateData; -      qemuDomainObjEnterMonitor(driver, vm); -      ret = qemuMonitorQueryDirtyRate(priv->mon, info); -    if (ret < 0) { -        virReportError(VIR_ERR_OPERATION_FAILED, -                       "%s", _("get vm's dirty rate failed.")); -    }      if (qemuDomainObjExitMonitor(driver, vm) < 0)          ret = -1; diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h index f03bbaa3d9..c4be340ecb 100644 --- a/src/qemu/qemu_migration.h +++ b/src/qemu/qemu_migration.h @@ -265,6 +265,6 @@ qemuDomainCalculateDirtyRate(virQEMUDriverPtr driver,                               long long sec);  int -qemuDomainQueryDirtyRate(virDomainPtr dom, +qemuDomainQueryDirtyRate(virQEMUDriverPtr driver,                           virDomainObjPtr vm,                           virDomainDirtyRateInfoPtr info); Michal .

Implement qemuMonitorJSONExtractDirtyRateInfo to deal with the return from qmp "query-dirty-rate", and store them in virDomainDirtyRateInfo. Signed-off-by: Hao Wang <wanghao232(a)huawei.com&gt; Reviewed-by: Chuan Zheng <zhengchuan(a)huawei.com&gt; --- include/libvirt/libvirt-domain.h | 17 +++++++++ src/qemu/qemu_monitor_json.c | 59 ++++++++++++++++++++++++++++++-- 2 files changed, 73 insertions(+), 3 deletions(-) diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index b950736b67..51d8685086 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -5096,6 +5096,23 @@ int virDomainBackupBegin(virDomainPtr domain, char *virDomainBackupGetXMLDesc(virDomainPtr domain, unsigned int flags); +/** + * virDomainDirtyRateStatus: + * + * Details on the cause of a dirtyrate calculation status. + */ + +typedef enum { + VIR_DOMAIN_DIRTYRATE_UNSTARTED = 0, /* the dirtyrate calculation has not been started */ + VIR_DOMAIN_DIRTYRATE_MEASURING = 1, /* the dirtyrate calculation is measuring */ + VIR_DOMAIN_DIRTYRATE_MEASURED = 2, /* the dirtyrate calculation is +completed */ + +# ifdef VIR_ENUM_SENTINELS + VIR_DOMAIN_DIRTYRATE_LAST +# endif +} virDomainDirtyRateStatus; +

/** * virDomainDirtyRateInfo: * diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 1924c7229b..ca7d8d23c0 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -9659,12 +9659,61 @@ qemuMonitorJSONCalculateDirtyRate(qemuMonitorPtr mon, } +VIR_ENUM_DECL(qemuDomainDirtyRateStatus); +VIR_ENUM_IMPL(qemuDomainDirtyRateStatus, + VIR_DOMAIN_DIRTYRATE_LAST, + "unstarted", + "measuring", + "measured"); + +static int +qemuMonitorJSONExtractDirtyRateInfo(virJSONValuePtr data, + virDomainDirtyRateInfoPtr info) +{ + const char *status; + int statusID; + + if (!(status = virJSONValueObjectGetString(data, "status"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("query-dirty-rate reply was missing 'status' data")); + return -1; + } + + if ((statusID = qemuDomainDirtyRateStatusTypeFromString(status)) < 0) { + return -1;

+ } + info->status = statusID; + + if ((info->status == VIR_DOMAIN_DIRTYRATE_MEASURED) && + (virJSONValueObjectGetNumberLong(data, "dirty-rate", &(info->dirtyRate)) < 0)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("query-dirty-rate reply was missing 'dirty-rate' data")); + return -1; + } + + if (virJSONValueObjectGetNumberLong(data, "start-time", &(info->startTime)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("query-dirty-rate reply was missing 'start-time' data")); + return -1; + } + + if (virJSONValueObjectGetNumberLong(data, "calc-time", &(info->calcTime)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("query-dirty-rate reply was missing 'calc-time' data")); + return -1; + } + + return 0; +} + + int qemuMonitorJSONQueryDirtyRate(qemuMonitorPtr mon, virDomainDirtyRateInfoPtr info) { g_autoptr(virJSONValue) cmd = NULL; g_autoptr(virJSONValue) reply = NULL; + virJSONValuePtr data = NULL; if (!(cmd = qemuMonitorJSONMakeCommand("query-dirty-rate", NULL))) return -1; @@ -9675,7 +9724,11 @@ qemuMonitorJSONQueryDirtyRate(qemuMonitorPtr mon, if (qemuMonitorJSONCheckError(cmd, reply) < 0) return -1; - /* TODO: extract dirtyrate data from reply and store in virDomainDirtyRateInfoPtr */ - info->status = 0; - return 0; + if (!(data = virJSONValueObjectGetObject(reply, "return"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("query-dirty-rate reply was missing 'return' data")); + return -1; + } + + return qemuMonitorJSONExtractDirtyRateInfo(data, info); }

On 11/7/20 10:41 AM, Hao Wang wrote: > Implement qemuDomainGetDirtyRateInfo: > using flags to control behaviors -- calculate and/or query dirtyrate. > > Signed-off-by: Hao Wang <wanghao232(a)huawei.com&gt; > Reviewed-by: Chuan Zheng <zhengchuan(a)huawei.com&gt; > --- >   include/libvirt/libvirt-domain.h | 11 ++++++ >   src/qemu/qemu_driver.c           | 68 ++++++++++++++++++++++++++++++++ >   2 files changed, 79 insertions(+) > > diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h > index 51d8685086..fc45f42dcf 100644 > --- a/include/libvirt/libvirt-domain.h > +++ b/include/libvirt/libvirt-domain.h > @@ -5096,6 +5096,17 @@ int virDomainBackupBegin(virDomainPtr domain, >   char *virDomainBackupGetXMLDesc(virDomainPtr domain, >                                   unsigned int flags); >   +/** > + * virDomainDirtyRateFlags: > + * > + * Details on the flags used by getdirtyrate api. > + */ > + > +typedef enum { > +    VIR_DOMAIN_DIRTYRATE_CALC = 1 << 0,  /* calculate domain's dirtyrate */ > +    VIR_DOMAIN_DIRTYRATE_QUERY = 1 << 1, /* query domain's dirtyrate */ > +} virDomainDirtyRateFlags; > + >   /** >    * virDomainDirtyRateStatus: >    * Again, doesn't belong here. > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index cb56fbbfcf..93d5a23630 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -20121,6 +20121,73 @@ qemuDomainAgentSetResponseTimeout(virDomainPtr dom, >   } >     +#define MIN_DIRTYRATE_CALCULATION_PERIOD    1  /* supported min dirtyrate calc time: 1s */ > +#define MAX_DIRTYRATE_CALCULATION_PERIOD    60 /* supported max dirtyrate calc time: 60s */ > + > +static int > +qemuDomainGetDirtyRateInfo(virDomainPtr dom, > +                           virDomainDirtyRateInfoPtr info, > +                           long long sec, > +                           unsigned int flags) > +{ > +    virDomainObjPtr vm = NULL; > +    virQEMUDriverPtr driver = dom->conn->privateData; > +    int ret = -1; > + > +    if (!(vm = qemuDomainObjFromDomain(dom))) > +        return ret; > + > +    if (virDomainGetDirtyRateInfoEnsureACL(dom->conn, vm->def) < 0) > +        goto cleanup; > + > +    if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) > +        goto cleanup; > + > +    if (!qemuMigrationSrcIsAllowed(driver, vm, false, 0)) > +        goto endjob; > + Why is this check needed? I don't understand it, can you please explain?

> +    if (flags & VIR_DOMAIN_DIRTYRATE_CALC) { > +        if (sec < MIN_DIRTYRATE_CALCULATION_PERIOD || sec > MAX_DIRTYRATE_CALCULATION_PERIOD) { > +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > +                           _("seconds=%lld is invalid, please choose value within [1, 60]."), > +                           sec); > +            goto endjob; > +        } > + > +        if (qemuDomainCalculateDirtyRate(dom, vm, sec) < 0) { > +            virReportError(VIR_ERR_OPERATION_FAILED, "%s", > +                           _("can't calculate domain's dirty rate")); This overwrites a more accurate error reported by qemuDomainCalculateDirtyRate(). > +            goto endjob; > +        } > +    } > + > +    if (flags & VIR_DOMAIN_DIRTYRATE_QUERY) { > +        if (flags & VIR_DOMAIN_DIRTYRATE_CALC) { > +            struct timespec ts = { .tv_sec = sec, .tv_nsec = 50 * 1000 * 1000ull }; > + > +            virObjectUnlock(vm); > +            nanosleep(&ts, NULL); > +            virObjectLock(vm); At first I was afraid, I was petrified that this waits for 50 seconds, until I realized it's nanosleep(). Perhaps g_usleep(50*1000); would look better? > +        } > + > +        if (qemuDomainQueryDirtyRate(dom, vm, info) < 0) { > +            virReportError(VIR_ERR_OPERATION_FAILED, "%s", > +                           _("can't query domain's dirty rate")); Again, error overwrite. > +            goto endjob; > +        } > +    } So if no flag is specified then nothing happens? I know you handle that in virsh, but I think that logic should live here. And perhaps the public API description should document that no flags is equal to specifying both flags together. > + > +    ret = 0; > + > + endjob: > +    qemuDomainObjEndJob(driver, vm); > + > + cleanup: > +    virDomainObjEndAPI(&vm); > +    return ret; > +} > + > + >   static virHypervisorDriver qemuHypervisorDriver = { >       .name = QEMU_DRIVER_NAME, >       .connectURIProbe = qemuConnectURIProbe, > @@ -20360,6 +20427,7 @@ static virHypervisorDriver qemuHypervisorDriver = { >       .domainAgentSetResponseTimeout = qemuDomainAgentSetResponseTimeout, /* 5.10.0 */ >       .domainBackupBegin = qemuDomainBackupBegin, /* 6.0.0 */ >       .domainBackupGetXMLDesc = qemuDomainBackupGetXMLDesc, /* 6.0.0 */ > +    .domainGetDirtyRateInfo = qemuDomainGetDirtyRateInfo, /* 6.9.0 */ 6.10.0 :-( >   }; >     I think the following should be squashed in: diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 4cbbe8dd84..4058fb487c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -20130,12 +20130,12 @@ qemuDomainGetDirtyRateInfo(virDomainPtr dom,                             long long sec,                             unsigned int flags)  { -    virDomainObjPtr vm = NULL;      virQEMUDriverPtr driver = dom->conn->privateData; +    virDomainObjPtr vm = NULL;      int ret = -1;      if (!(vm = qemuDomainObjFromDomain(dom))) -        return ret; +        return -1;      if (virDomainGetDirtyRateInfoEnsureACL(dom->conn, vm->def) < 0)          goto cleanup; @@ -20147,18 +20147,18 @@ qemuDomainGetDirtyRateInfo(virDomainPtr dom,          goto endjob;      if (flags & VIR_DOMAIN_DIRTYRATE_CALC) { -        if (sec < MIN_DIRTYRATE_CALCULATION_PERIOD || sec > MAX_DIRTYRATE_CALCULATION_PERIOD) { +        if (sec < MIN_DIRTYRATE_CALCULATION_PERIOD || +            sec > MAX_DIRTYRATE_CALCULATION_PERIOD) {              virReportError(VIR_ERR_CONFIG_UNSUPPORTED, -                           _("seconds=%lld is invalid, please choose value within [1, 60]."), -                           sec); +                           _("seconds=%lld is invalid, please choose value within [%d, %d]."), +                           sec, +                           MIN_DIRTYRATE_CALCULATION_PERIOD, +                           MAX_DIRTYRATE_CALCULATION_PERIOD);              goto endjob;          } -        if (qemuDomainCalculateDirtyRate(dom, vm, sec) < 0) { -            virReportError(VIR_ERR_OPERATION_FAILED, "%s", -                           _("can't calculate domain's dirty rate")); +        if (qemuDomainCalculateDirtyRate(driver, vm, sec) < 0)              goto endjob; -        }      }      if (flags & VIR_DOMAIN_DIRTYRATE_QUERY) { @@ -20170,11 +20170,8 @@ qemuDomainGetDirtyRateInfo(virDomainPtr dom,              virObjectLock(vm);          } -        if (qemuDomainQueryDirtyRate(dom, vm, info) < 0) { -            virReportError(VIR_ERR_OPERATION_FAILED, "%s", -                           _("can't query domain's dirty rate")); +        if (qemuDomainQueryDirtyRate(driver, vm, info) < 0)              goto endjob; -        }      }      ret = 0; @@ -20427,7 +20424,7 @@ static virHypervisorDriver qemuHypervisorDriver = {      .domainAgentSetResponseTimeout = qemuDomainAgentSetResponseTimeout, /* 5.10.0 */      .domainBackupBegin = qemuDomainBackupBegin, /* 6.0.0 */      .domainBackupGetXMLDesc = qemuDomainBackupGetXMLDesc, /* 6.0.0 */ -    .domainGetDirtyRateInfo = qemuDomainGetDirtyRateInfo, /* 6.9.0 */ +    .domainGetDirtyRateInfo = qemuDomainGetDirtyRateInfo, /* 6.10.0 */  }; Michal .

Signed-off-by: Hao Wang <wanghao232(a)huawei.com&gt; Signed-off-by: Zhou Yimin <zhouyimin(a)huawei.com&gt; Reviewed-by: Chuan Zheng <zhengchuan(a)huawei.com&gt; --- tools/virsh-domain.c | 112 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 112 insertions(+) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index ef347585e8..52608e2c1b 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -14374,6 +14374,112 @@ cmdGuestInfo(vshControl *ctl, const vshCmd *cmd) return ret; } +/* + * "querydirtyrate" command + */ +static const vshCmdInfo info_getdirtyrate[] = { + {.name = "help", + .data = N_("Get a vm's memory dirty rate") + }, + {.name = "desc", + .data = N_("Get memory dirty rate of a domain in order to decide" + " whether it's proper to be migrated out or not.") + }, + {.name = NULL} +}; + +static const vshCmdOptDef opts_getdirtyrate[] = { + VIRSH_COMMON_OPT_DOMAIN_FULL(0), + {.name = "seconds", + .type = VSH_OT_INT, + .help = N_("calculate memory dirty rate within specified seconds," + " a valid range of values is [1, 60], and would default to 1s.") + }, + {.name = "calculate", + .type = VSH_OT_BOOL, + .help = N_("calculate dirty rate only, can be used together with --query," + " either or both is expected, otherwise would default to both.") + }, + {.name = "query", + .type = VSH_OT_BOOL, + .help = N_("query dirty rate only, can be used together with --calculate," + " either or both is expected, otherwise would default to both.") + }, + {.name = NULL} +}; + +static bool +cmdGetDirtyRateInfo(vshControl *ctl, const vshCmd *cmd) +{ + virDomainPtr dom = NULL; + virDomainDirtyRateInfo info; + long long sec = 0; + const char *status = NULL; + unsigned int flags = 0; + int rc; + bool ret = false; + bool calc = vshCommandOptBool(cmd, "calculate"); + bool query = vshCommandOptBool(cmd, "query"); + + if (calc) + flags |= VIR_DOMAIN_DIRTYRATE_CALC; + if (query) + flags |= VIR_DOMAIN_DIRTYRATE_QUERY; + + /* if flag option is missing, default to both --calculate and --query */ + if (!calc && !query) + flags |= VIR_DOMAIN_DIRTYRATE_CALC | VIR_DOMAIN_DIRTYRATE_QUERY; + + if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) + return false; + + rc = vshCommandOptLongLong(ctl, cmd, "seconds", &sec); + if (rc < 0) + goto done; + + /* if --seconds option is missing, default to 1s */ + if (!rc) + sec = 1; + + if (virDomainGetDirtyRateInfo(dom, &info, sec, flags) < 0) { + vshError(ctl, "%s", _("Get memory dirty-rate failed."));

+ goto done; + } + + if (flags & VIR_DOMAIN_DIRTYRATE_QUERY) { + switch (info.status) { + case VIR_DOMAIN_DIRTYRATE_UNSTARTED: + status = _("unstarted"); + break; + case VIR_DOMAIN_DIRTYRATE_MEASURING: + status = _("measuring"); + break; + case VIR_DOMAIN_DIRTYRATE_MEASURED: + status = _("measured"); + break; + default: + status = _("unknown"); + } + + vshPrint(ctl, _("status: %s\n"), status); + vshPrint(ctl, _("start time: %lld\n"), info.startTime); + vshPrint(ctl, _("calc time: %lld s\n"), info.calcTime);

+ + if (info.status == VIR_DOMAIN_DIRTYRATE_MEASURED) + vshPrint(ctl, _("dirty rate: %lld MB/s\n"), info.dirtyRate); + else + vshPrint(ctl, _("dirty rate: the calculation is %s, please query results later\n"), + status); + } else { + vshPrint(ctl, _("Memory dirty rate is calculating, use --query option to display results.\n")); + } + + ret = true; + done: + virshDomainFree(dom); + return ret; +} + const vshCmdDef domManagementCmds[] = { {.name = "attach-device", .handler = cmdAttachDevice, @@ -15001,5 +15107,11 @@ const vshCmdDef domManagementCmds[] = { .info = info_guestinfo, .flags = 0 }, + {.name = "getdirtyrate", + .handler = cmdGetDirtyRateInfo, + .opts = opts_getdirtyrate, + .info = info_getdirtyrate, + .flags = 0 + }, {.name = NULL} };