[PATCH v5 1/5] migration/dirtyrate: Introduce DomainGetDirtyRateInfo API

[Hao Wang]

This's quite helpful suggestions. I'll refactor the APIs following the advices.

BR,
Hao

On 2021/2/1 22:32, Michal Privoznik wrote:
> On 2/1/21 10:55 AM, Hao Wang wrote:
>> Introduce DomainGetDirtyRateInfo API to get domain's memory dirty rate
>> info which may be expected by user in order to decide whether it's proper
>> to be migrated out or not. Using flags to control the action of the API:
>>
>> If the VIR_DOMAIN_DIRTYRATE_CALC flag is set, this will calculate
>> domain's memory dirty rate within specific time.
>>
>> If the VIR_DOMAIN_DIRTYRATE_QUERY flag is set, this will query the
>> dirty rate info calculated last time.
>>
>> The VIR_DOMAIN_DIRTYRATE_DEFAULT flag is equal to both
>> VIR_DOMAIN_DIRTYRATE_CALC and VIR_DOMAIN_DIRTYRATE_QUERY.
>>
>> Signed-off-by: Hao Wang <wanghao232 at huawei.com>
>> Signed-off-by: Zhou Yimin <zhouyimin at huawei.com>
>> Reviewed-by: Chuan Zheng <zhengchuan at huawei.com>
>> ---
>>   include/libvirt/libvirt-domain.h | 59 ++++++++++++++++++++++++++++++++
>>   src/driver-hypervisor.h          |  7 ++++
>>   src/libvirt-domain.c             | 56 ++++++++++++++++++++++++++++++
>>   src/libvirt_public.syms          |  5 +++
>>   src/remote/remote_driver.c       |  1 +
>>   src/remote/remote_protocol.x     | 21 +++++++++++-
>>   6 files changed, 148 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
>> index de2456812c..77b46c2018 100644
>> --- a/include/libvirt/libvirt-domain.h
>> +++ b/include/libvirt/libvirt-domain.h
>> @@ -5119,4 +5119,63 @@ int virDomainAuthorizedSSHKeysSet(virDomainPtr domain,
>>                                     unsigned int nkeys,
>>                                     unsigned int flags);
>>   +/**
>> + * virDomainDirtyRateStatus:
>> + *
>> + * 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_MEASURED  = 2, /* the dirtyrate calculation is
>> +                                           completed */
>> +
>> +# ifdef VIR_ENUM_SENTINELS
>> +    VIR_DOMAIN_DIRTYRATE_LAST
>> +# endif
>> +} virDomainDirtyRateStatus;
>> +
>> +/**
>> + * 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, one of
>> +                               virDomainDirtyRateStatus */
>> +    long long dirtyRate;    /* the dirtyrate in MB/s */
> 
> I guess you meant MiB/s.
> 
>> +    long long startTime;    /* the start time of dirtyrate calculation */
>> +    int calcTime;           /* the period of dirtyrate calculation */
>> +};
> 
> Do we need to expose this as a struct? IIRC, in review of v4 Peter was suggesting this to be exposed as a new set of virTypedParameter under virDomainListGetStats() and virConnectGetAllDomainStats().
> 
> Problem with structures is that once they are released, we can not ever change them (the best we can do is update comments), we can not even change order of members, because might break how structure is organized in memory (compiler might put padding at different place than originally) and thus we would break ABI. Therefore, if we ever need to report one member more, we can't. Well, various projects approach this differently. Some put intentional padding at the end of structure to reserve extra bytes for future use. That's ugly and not scalable.
> 
> What we invented for this purpose are so called typed parameters: basically an array of <key, type, value> tuples. Users can then iterate over returned array and look for items interesting to them. For instance:
> 
> virsh domstats fedora
> Domain: 'fedora'
>   state.state=1
>   state.reason=1
>   cpu.time=77689980240
>   cpu.user=700000000
>   cpu.system=74490000000
>   cpu.cache.monitor.count=0
>   ...
> 
>> +
>> +/**
>> + * virDomainDirtyRateInfoPtr:
>> + *
>> + * a virDomainDirtyRateInfoPtr is a pointer to a virDomainDirtyRateInfo structure.
>> + */
>> +
>> +typedef virDomainDirtyRateInfo *virDomainDirtyRateInfoPtr;
>> +
>> +/**
>> + * virDomainDirtyRateFlags:
>> + *
>> + * Details on the flags used by getdirtyrate api.
>> + */
>> +typedef enum {
>> +    VIR_DOMAIN_DIRTYRATE_DEFAULT = 0,      /* default domdirtyrate behavior:
>> +                                              calculate and query */
>> +    VIR_DOMAIN_DIRTYRATE_CALC    = 1 << 0, /* calculate domain's dirtyrate */
>> +    VIR_DOMAIN_DIRTYRATE_QUERY   = 1 << 1, /* query domain's dirtyrate */
>> +} virDomainDirtyRateFlags;
>> +
>> +int virDomainGetDirtyRateInfo(virDomainPtr domain,
>> +                              virDomainDirtyRateInfoPtr info,
>> +                              int sec,
>> +                              unsigned int flags);
> 
> Looking into the future, this is supposed to work like this: with a single API call I can both set the time calc time and get the results back:
> 
>   virDomainGetDirtyRateInfo(dom, &info, 10, CALC | QUERY);
> 
> This call will block for 11.5 seconds. That sounds like bad design, esp. since QEMU tells caller the state the calculation's in (measuring vs. measured). How about splitting this into two APIs?
> 
>   virDomainStartDirtyRateCalc(dom, seconds, flags);
> 
> for starting the calculation, and then the second:
> 
>   new typed params for domstats mentioned above
> 
> Note that @flags in new API will probably be unused for now and that's okay. It's more future proof this way.
> There's also virDomainSetMemoryParameters() which we could use instead of new API - if new typed param is introduced for starting calculation, but that feels wrong (I can't tell why really, it's just a gut feeling).
> 
> Michal
> 
> .