Re: [PATCH v4 3/7] migration/dirtyrate: Implement qemuDomainCalculateDirtyRate

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Great thanks for these helpful suggestions. I'll introduce them into my next version.

On 2020/11/11 4:11, Michal Privoznik wrote:
> On 11/7/20 10:41 AM, Hao Wang wrote:
>> Implement qemuDomainCalculateDirtyRate which calculates domain's memory
>> dirty rate calling qmp "calc-dirty-rate".
>>
>> Signed-off-by: Hao Wang <wanghao232@xxxxxxxxxx>
>> Signed-off-by: Zhou Yimin <zhouyimin@xxxxxxxxxx>
>> Reviewed-by: Chuan Zheng <zhengchuan@xxxxxxxxxx>
>> ---
>>   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,
> 
> The caller (implemented later in the series) has pointer to the driver. Might as well pass it here. Alternativelly, there is a piggy back pointer stored inside @vm's private data: QEMU_DOMAIN_PRIVATE(vm)->driver
> 
>> +                             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;
> 
> It's okay to initialize priv when declaring it.
> 
>> +
>> +    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,
> 
> I'm not convinced on this typecast. qemuMonitorJSONMakeCommand() will under the hood pop long long. But anyway, @sec shouldn't be long long in the first place.
> 
>> +                                           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);
>>
> 
> I think the following should be squashed in:
> 
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index c89ee39831..b468c68d2d 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -5839,25 +5839,22 @@ qemuMigrationSrcFetchMirrorStats(virQEMUDriverPtr driver,
> 
> 
>  int
> -qemuDomainCalculateDirtyRate(virDomainPtr dom,
> +qemuDomainCalculateDirtyRate(virQEMUDriverPtr driver,
>                               virDomainObjPtr vm,
>                               long long sec)
>  {
> -    virQEMUDriverPtr driver = dom->conn->privateData;
> -    qemuDomainObjPrivatePtr priv;
> -    int ret = -1;
> +    qemuDomainObjPrivatePtr priv = vm->privateData;
> +    int ret;
> +
> +    VIR_DEBUG("Calculate dirty rate during %lld seconds", sec);
> 
>      if (!virDomainObjIsActive(vm)) {
>          virReportError(VIR_ERR_OPERATION_INVALID,
>                         "%s", _("domain is not running"));
> -        return ret;
> +        return -1;
>      }
> 
> -    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;
> diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h
> index 0522b375c0..ee5f39655e 100644
> --- a/src/qemu/qemu_migration.h
> +++ b/src/qemu/qemu_migration.h
> @@ -260,6 +260,6 @@ qemuMigrationSrcFetchMirrorStats(virQEMUDriverPtr driver,
>                                   qemuDomainJobInfoPtr jobInfo);
> 
>  int
> -qemuDomainCalculateDirtyRate(virDomainPtr dom,
> +qemuDomainCalculateDirtyRate(virQEMUDriverPtr driver,
>                               virDomainObjPtr vm,
>                               long long sec);
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 53c2a6521e..1f9d65b235 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -9645,7 +9645,7 @@ qemuMonitorJSONCalculateDirtyRate(qemuMonitorPtr mon,
>      g_autoptr(virJSONValue) reply = NULL;
> 
>      if (!(cmd = qemuMonitorJSONMakeCommand("calc-dirty-rate",
> -                                           "I:calc-time", (long)sec,
> +                                           "I:calc-time", sec,
>                                             NULL)))
>          return -1;
> 
> 
> Michal
> 
> .





[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux