Re: [PATCH v5 1/5] migration/dirtyrate: Introduce DomainGetDirtyRateInfo API
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 >> Signed-off-by: Zhou Yimin >> Reviewed-by: Chuan Zheng >> --- >> 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 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=7 > cpu.system=7449000 > 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
Re: [PATCH v5 1/5] migration/dirtyrate: Introduce DomainGetDirtyRateInfo API
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 Signed-off-by: Zhou Yimin Reviewed-by: Chuan Zheng --- 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 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=7 cpu.system=7449000 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, , 10, CALC | QUERY); This call will block for 11.5 seconds. That sounds like bad design, esp. since
Re: [PATCH v5 1/5] migration/dirtyrate: Introduce DomainGetDirtyRateInfo API
Thanks for reviewing my patchset. Actually I remember that I have passed all tests in my env before uploading. However, I doubel-check the results after receving Daniel's reply, and found that 'check-remote-protocol'does not lie in my test list for the reason that dwarves is missing in my env. After installing dwarves, I got same test error with Daniel. Hao On 2021/2/1 21:24, Michal Privoznik wrote: > On 2/1/21 1:11 PM, Daniel Henrique Barboza wrote: >> This patch is making the 'check-remote-protocol' test error out in >> my env: >> > > Indeed, any change to remote_protocol.x has to be coupled with change to > src/remote_protocol-structs. The idea for this test is that we take compiled > version of our RPC and use pdwtags to "decompile" it. Then, the output > generated by pwdtags is compared against well known output stored in git > (src/remote_protocol-structs). The idea is that we will catch incompatible > changes made by rpcgen/compiler/developer. BTW, that is the reasoning behind > Dan's patch: > > > commit e603efb6ec5d1a2295adfda934e79f022bb7bb0e > Author: Daniel P. Berrangé > AuthorDate: Mon Jan 25 18:13:57 2021 + > Commit: Daniel P. Berrangé > CommitDate: Tue Jan 26 12:33:31 2021 + > > gitlab: force dwarf4 format for debuginfo in Fedora rawhide > > Fedora 34 rawhide has pulled in a new GCC 11 build which now > defaults to dwarf5 format. This format is not compatible with > the pdwtags program used in our test suite to validate the > RPC files. > > We have no need for debuginfo in CI except for pdwtags, > so the simplest short term fix is to force the older dwarf > version in the hope that a fixed dwarves release will > arrive before Fedora 34 is released, or GCC 11 becomes more > widespread. Eventually we might need to figure out a way to > probe for compatibility but for now, we'll hope that any > distro with GCC 11 will be able to have a fixed dwarves too. > > https://bugzilla.redhat.com/show_bug.cgi?id=1919965 > Reviewed-by: Erik Skultety > Signed-off-by: Daniel P. Berrangé > > > In this case, it's developer's fault for not updating remote_protocol-structs > to contain additions made to remote_protocol.x. > > Anyway, I don't think we will need new RPC anyway. Let me comment to the > patch itself. > > Michal > > .
Re: [PATCH v5 1/5] migration/dirtyrate: Introduce DomainGetDirtyRateInfo API
On 2/1/21 1:11 PM, Daniel Henrique Barboza wrote: This patch is making the 'check-remote-protocol' test error out in my env: Indeed, any change to remote_protocol.x has to be coupled with change to src/remote_protocol-structs. The idea for this test is that we take compiled version of our RPC and use pdwtags to "decompile" it. Then, the output generated by pwdtags is compared against well known output stored in git (src/remote_protocol-structs). The idea is that we will catch incompatible changes made by rpcgen/compiler/developer. BTW, that is the reasoning behind Dan's patch: commit e603efb6ec5d1a2295adfda934e79f022bb7bb0e Author: Daniel P. Berrangé AuthorDate: Mon Jan 25 18:13:57 2021 + Commit: Daniel P. Berrangé CommitDate: Tue Jan 26 12:33:31 2021 + gitlab: force dwarf4 format for debuginfo in Fedora rawhide Fedora 34 rawhide has pulled in a new GCC 11 build which now defaults to dwarf5 format. This format is not compatible with the pdwtags program used in our test suite to validate the RPC files. We have no need for debuginfo in CI except for pdwtags, so the simplest short term fix is to force the older dwarf version in the hope that a fixed dwarves release will arrive before Fedora 34 is released, or GCC 11 becomes more widespread. Eventually we might need to figure out a way to probe for compatibility but for now, we'll hope that any distro with GCC 11 will be able to have a fixed dwarves too. https://bugzilla.redhat.com/show_bug.cgi?id=1919965 Reviewed-by: Erik Skultety Signed-off-by: Daniel P. Berrangé In this case, it's developer's fault for not updating remote_protocol-structs to contain additions made to remote_protocol.x. Anyway, I don't think we will need new RPC anyway. Let me comment to the patch itself. Michal
Re: [PATCH v5 1/5] migration/dirtyrate: Introduce DomainGetDirtyRateInfo API
This patch is making the 'check-remote-protocol' test error out in my env: Ok: 305 Expected Fail: 0 Fail: 1 Unexpected Pass:0 Skipped:4 Timeout:0 The output from the failed tests: 12/310 libvirt / check-remote_protocol FAIL 0.27s (exit status 1) --- command --- 11:55:05 LC_CTYPE='en_US.UTF-8' LANG='C' LC_ALL='' /usr/bin/python3 /home/danielhb/kvm-project/libvirt/scripts/check-remote-protocol.py remote_protocol virt_remote_driver /home/danielhb/kvm-project/libvirt/build/src/remote/libvirt_remote_driver.a /usr/bin/pdwtags /home/danielhb/kvm-project/libvirt/build/../src/remote_protocol-structs --- stdout --- --- /home/danielhb/kvm-project/libvirt/build/../src/remote_protocol-structs 2020-11-19 13:27:02.018823909 -0300 +++ - 2021-02-01 08:55:05.827296710 -0300 @@ -3162,6 +3162,17 @@ } keys; u_int flags; }; +struct remote_domain_get_dirty_rate_info_args { +remote_nonnull_domain dom; +intsec; +u_int flags; +}; +struct remote_domain_get_dirty_rate_info_ret { +intstatus; +int64_tdirtyRate; +int64_tstartTime; +intcalcTime; +}; enum remote_procedure { REMOTE_PROC_CONNECT_OPEN = 1, REMOTE_PROC_CONNECT_CLOSE = 2, @@ -3588,4 +3599,5 @@ REMOTE_PROC_DOMAIN_EVENT_MEMORY_FAILURE = 423, REMOTE_PROC_DOMAIN_AUTHORIZED_SSH_KEYS_GET = 424, REMOTE_PROC_DOMAIN_AUTHORIZED_SSH_KEYS_SET = 425, +REMOTE_PROC_DOMAIN_GET_DIRTY_RATE_INFO = 426, }; --- I'm not sure how to fix it. This error persists even if I apply all the patches, meaning that it's not something that is being declared too early. I'm afraid you're missing some step in the remote protocol implementation that is making this test fail or perhaps I'm the one messing this up. Let's see what others will say. Thanks, DHB On 2/1/21 6: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 Signed-off-by: Zhou Yimin Reviewed-by: Chuan Zheng --- 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 */ +long long startTime;/* the start time of dirtyrate calculation */ +int calcTime; /* the period of dirtyrate calculation */ +}; + +/** + * virDomainDirtyRateInfoPtr: + * + * a virDomainDirtyRateInfoPtr is a pointer to a virDomainDirtyRateInfo structure. + */ + +typedef virDomainDirtyRateInfo
[PATCH v5 1/5] migration/dirtyrate: Introduce DomainGetDirtyRateInfo API
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 Signed-off-by: Zhou Yimin Reviewed-by: Chuan Zheng --- 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 */ +long long startTime;/* the start time of dirtyrate calculation */ +int calcTime; /* the period of dirtyrate calculation */ +}; + +/** + * 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); + #endif /* LIBVIRT_DOMAIN_H */ diff --git a/src/driver-hypervisor.h b/src/driver-hypervisor.h index 9e8fe89921..5ad681997b 100644 --- a/src/driver-hypervisor.h +++ b/src/driver-hypervisor.h @@ -1400,6 +1400,12 @@ typedef int unsigned int nkeys, unsigned int flags); +typedef int +(*virDrvDomainGetDirtyRateInfo)(virDomainPtr domain, +virDomainDirtyRateInfoPtr info, +int sec, +unsigned int flags); + typedef struct _virHypervisorDriver virHypervisorDriver; typedef virHypervisorDriver *virHypervisorDriverPtr; @@ -1665,4 +1671,5 @@ struct _virHypervisorDriver { virDrvDomainBackupGetXMLDesc domainBackupGetXMLDesc; virDrvDomainAuthorizedSSHKeysGet domainAuthorizedSSHKeysGet; virDrvDomainAuthorizedSSHKeysSet domainAuthorizedSSHKeysSet; +virDrvDomainGetDirtyRateInfo domainGetDirtyRateInfo; }; diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index c9f8ffdb56..777028c499 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -13102,3 +13102,59 @@ virDomainAuthorizedSSHKeysSet(virDomainPtr domain, virDispatchError(conn); return -1; } + + +/** + * virDomainGetDirtyRateInfo: + * @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