Re: [PATCH v4 2/7] migration/dirtyrate: set up framwork of domainGetDirtyRateInfo API

[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:
>> Introduce DomainGetDirtyRateInfo API for domain's memory dirty rate
>> calculation and query.
>>
>> Signed-off-by: Hao Wang <wanghao232@xxxxxxxxxx>
>> Signed-off-by: Zhou Yimin <zhouyimin@xxxxxxxxxx>
>> Reviewed-by: Chuan Zheng <zhengchuan@xxxxxxxxxx>
>> ---
>>   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
> 
> .





[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