[PATCH v2 0/3] domstats:add haltpolling time statistic interface
This series add the ability to statistic the halt polling time when VM execute HLT(arm is WFI). v1: https://listman.redhat.com/archives/libvir-list/2021-July/msg00029.html changes from v1: - Move virGetCgroupValueRaw to utils.c and rename it virGetValueRaw. So that we can call it to obtain halt polling time. - Helper function virGetCpuHaltPollTime and virGetDebugFsKvmValue are added in a separate patch - Use STRPREFIX to match the path prefix. - Fix the logic that domstats will break when platform is non-linux, debugfs isn't mounted and so on. Yang Fei (3): util: Move virGetCgroupValueRaw to vircgroup.c and rename it virGetValueRaw util: Add virGetCpuHaltPollTime and virGetDebugFsKvmValue qemu: Introduce qemuDomainGetStatsCpuHaltPollTime src/libvirt_private.syms | 2 + src/qemu/qemu_driver.c | 28 +++++++++++ src/util/vircgroup.c | 29 +---------- src/util/vircgrouppriv.h | 3 -- src/util/vircgroupv1.c | 5 +- src/util/vircgroupv2.c | 5 +- src/util/virutil.c | 101 +++++++++++++++++++++++++++++++++++++++ src/util/virutil.h | 12 +++++ 8 files changed, 151 insertions(+), 34 deletions(-) -- 2.23.0
Move virGetCgroupValueRaw from vircgroup.c, so that we can call it more appropriately at any where. And change it to a more generic name virGetValueRaw. Replace virGetCgroupValueRaw by virGetValueRaw in cgroup data obtain. Signed-off-by: Yang Fei <yangfei85@huawei.com> --- src/util/vircgroup.c | 29 ++--------------------------- src/util/vircgrouppriv.h | 3 --- src/util/vircgroupv1.c | 5 +++-- src/util/vircgroupv2.c | 5 +++-- src/util/virutil.c | 23 +++++++++++++++++++++++ src/util/virutil.h | 3 +++ 6 files changed, 34 insertions(+), 34 deletions(-) diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 65a81e8d6f..80a06c78b9 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -48,6 +48,7 @@ #include "virtypedparam.h" #include "virhostcpu.h" #include "virthread.h" +#include "virutil.h" VIR_LOG_INIT("util.cgroup"); @@ -539,31 +540,6 @@ virCgroupSetValueRaw(const char *path, return 0; } - -int -virCgroupGetValueRaw(const char *path, - char **value) -{ - int rc; - - *value = NULL; - - VIR_DEBUG("Get value %s", path); - - if ((rc = virFileReadAll(path, 1024*1024, value)) < 0) { - virReportSystemError(errno, - _("Unable to read from '%s'"), path); - return -1; - } - - /* Terminated with '\n' has sometimes harmful effects to the caller */ - if (rc > 0 && (*value)[rc - 1] == '\n') - (*value)[rc - 1] = '\0'; - - return 0; -} - - int virCgroupSetValueStr(virCgroup *group, int controller, @@ -590,10 +566,9 @@ virCgroupGetValueStr(virCgroup *group, if (virCgroupPathOfController(group, controller, key, &keypath) < 0) return -1; - return virCgroupGetValueRaw(keypath, value); + return virGetValueRaw(keypath, value); } - int virCgroupGetValueForBlkDev(const char *str, const char *path, diff --git a/src/util/vircgrouppriv.h b/src/util/vircgrouppriv.h index caf7ed84db..627308d995 100644 --- a/src/util/vircgrouppriv.h +++ b/src/util/vircgrouppriv.h @@ -76,9 +76,6 @@ int virCgroupSetValueDBus(const char *unitName, int virCgroupSetValueRaw(const char *path, const char *value); -int virCgroupGetValueRaw(const char *path, - char **value); - int virCgroupSetValueStr(virCgroup *group, int controller, const char *key, diff --git a/src/util/vircgroupv1.c b/src/util/vircgroupv1.c index 8a04bb2e4a..2a40de51a0 100644 --- a/src/util/vircgroupv1.c +++ b/src/util/vircgroupv1.c @@ -42,6 +42,7 @@ #include "virerror.h" #include "viralloc.h" #include "virthread.h" +#include "virutil.h" VIR_LOG_INIT("util.cgroup"); @@ -1042,7 +1043,7 @@ virCgroupV1GetBlkioWeight(virCgroup *group, return -1; } - if (virCgroupGetValueRaw(path, &value) < 0) + if (virGetValueRaw(path, &value) < 0) return -1; if (virStrToLong_ui(value, NULL, 10, weight) < 0) { @@ -1297,7 +1298,7 @@ virCgroupV1GetBlkioDeviceWeight(virCgroup *group, return -1; } - if (virCgroupGetValueRaw(path, &value) < 0) + if (virGetValueRaw(path, &value) < 0) return -1; if (virCgroupGetValueForBlkDev(value, devPath, &str) < 0) diff --git a/src/util/vircgroupv2.c b/src/util/vircgroupv2.c index 8881d3a88a..1c444d58f2 100644 --- a/src/util/vircgroupv2.c +++ b/src/util/vircgroupv2.c @@ -41,6 +41,7 @@ #include "virlog.h" #include "virstring.h" #include "virsystemd.h" +#include "virutil.h" VIR_LOG_INIT("util.cgroup"); @@ -701,7 +702,7 @@ virCgroupV2GetBlkioWeight(virCgroup *group, return -1; } - if (virCgroupGetValueRaw(path, &value) < 0) + if (virGetValueRaw(path, &value) < 0) return -1; if ((tmp = strstr(value, "default "))) { @@ -907,7 +908,7 @@ virCgroupV2GetBlkioDeviceWeight(virCgroup *group, return -1; } - if (virCgroupGetValueRaw(path, &value) < 0) + if (virGetValueRaw(path, &value) < 0) return -1; if (virCgroupGetValueForBlkDev(value, devPath, &str) < 0) diff --git a/src/util/virutil.c b/src/util/virutil.c index 311cbbf93a..c0d25fe247 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -1936,3 +1936,26 @@ virPipeNonBlock(int fds[2]) { return virPipeImpl(fds, true, true); } + +int +virGetValueRaw(const char *path, + char **value) +{ + int rc; + + *value = NULL; + + VIR_DEBUG("Get value %s", path); + + if ((rc = virFileReadAll(path, 1024*1024, value)) < 0) { + virReportSystemError(errno, + _("Unable to read from '%s'"), path); + return -1; + } + + /* Terminated with '\n' has sometimes harmful effects to the caller */ + if (rc > 0 && (*value)[rc - 1] == '\n') + (*value)[rc - 1] = '\0'; + + return 0; +} diff --git a/src/util/virutil.h b/src/util/virutil.h index 854b494890..851b421476 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -225,3 +225,6 @@ int virPipeQuiet(int fds[2]); * Returns: -1 on error, 0 on success */ int virPipeNonBlock(int fds[2]); + +int virGetValueRaw(const char *path, + char **value); -- 2.23.0
On 7/14/21 2:28 PM, Yang Fei wrote:
Move virGetCgroupValueRaw from vircgroup.c, so that we can call it more appropriately at any where. And change it to a more generic name virGetValueRaw. Replace virGetCgroupValueRaw by virGetValueRaw in cgroup data obtain.
Signed-off-by: Yang Fei <yangfei85@huawei.com> --- src/util/vircgroup.c | 29 ++--------------------------- src/util/vircgrouppriv.h | 3 --- src/util/vircgroupv1.c | 5 +++-- src/util/vircgroupv2.c | 5 +++-- src/util/virutil.c | 23 +++++++++++++++++++++++ src/util/virutil.h | 3 +++ 6 files changed, 34 insertions(+), 34 deletions(-)
I don't think this is necessary. We have whole family of functions that read a file and translate its contents into an integer. For instance virFileReadValueUllong(). BTW: make sure that 'ninja test' passes after each commit. Michal
On 2021/7/14 22:17, Michal Prívozník wrote:
On 7/14/21 2:28 PM, Yang Fei wrote:
Move virGetCgroupValueRaw from vircgroup.c, so that we can call it more appropriately at any where. And change it to a more generic name virGetValueRaw. Replace virGetCgroupValueRaw by virGetValueRaw in cgroup data obtain.
Signed-off-by: Yang Fei <yangfei85@huawei.com> --- src/util/vircgroup.c | 29 ++--------------------------- src/util/vircgrouppriv.h | 3 --- src/util/vircgroupv1.c | 5 +++-- src/util/vircgroupv2.c | 5 +++-- src/util/virutil.c | 23 +++++++++++++++++++++++ src/util/virutil.h | 3 +++ 6 files changed, 34 insertions(+), 34 deletions(-)
I don't think this is necessary. We have whole family of functions that read a file and translate its contents into an integer. For instance virFileReadValueUllong().
BTW: make sure that 'ninja test' passes after each commit.
Michal
.
Yes, virFileReadValueUllong() is also meets the requirement. I will drop this patch in next version. Thanks for reminding me and I will be more careful afterwards.
Add helper function virGetCpuHaltPollTime and virGetDebugFsKvmValue to obtain the halt polling time. If system mount debugfs, and the kernel support halt polling time statistic, it will work. Signed-off-by: Yang Fei <yangfei85@huawei.com> --- src/libvirt_private.syms | 2 ++ src/util/virutil.c | 78 ++++++++++++++++++++++++++++++++++++++++ src/util/virutil.h | 9 +++++ 3 files changed, 89 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 68e4b6aab8..f92213b8c2 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3479,6 +3479,8 @@ virDoesUserExist; virDoubleToStr; virFormatIntDecimal; virFormatIntPretty; +virGetCpuHaltPollTime; +virGetDebugFsKvmValue; virGetDeviceID; virGetDeviceUnprivSGIO; virGetGroupID; diff --git a/src/util/virutil.c b/src/util/virutil.c index c0d25fe247..7e4531b4b4 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -1959,3 +1959,81 @@ virGetValueRaw(const char *path, return 0; } + +int +virGetDebugFsKvmValue(struct dirent *ent, + const char *path, + const char *filename, + unsigned long long *value) +{ + g_autofree char *valToStr = NULL; + g_autofree char *valPath = NULL; + + valPath = g_strdup_printf("%s/%s/%s", path, ent->d_name, filename); + + if (virGetValueRaw(valPath, &valToStr) < 0) { + return -1; + } + + /* 10 is a Cardinality, must be between 2 and 36 inclusive, + * or special value 0. Used in fuction strtoull() + */ + if (virStrToLong_ull(valToStr, NULL, 10, value) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to parse '%s' as an integer"), valToStr); + return -1; + } + + return 0; +} + +int +virGetCpuHaltPollTime(pid_t pid, + unsigned long long *haltPollSuccess, + unsigned long long *haltPollFail) +{ + g_autofree char *pidToStr = NULL; + g_autofree char *debugFsPath = NULL; + g_autofree char *completePath = NULL; + struct dirent *ent = NULL; + DIR *dir = NULL; + int ret = -1; + int flag = 0; + + if (!(debugFsPath = virFileFindMountPoint("debugfs"))) { + virReportSystemError(errno, "%s", + _("unable to find debugfs mountpoint")); + return ret; + } + + completePath = g_strdup_printf("%s/%s", debugFsPath, "kvm"); + if (virDirOpen(&dir, completePath) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s %s", "Can not open directory", completePath); + return ret; + } + + pidToStr = g_strdup_printf("%lld%c", (unsigned long long)pid, '-'); + while (virDirRead(dir, &ent, NULL) > 0) { + if (STRPREFIX(ent->d_name, pidToStr)) { + flag = 1; + break; + } + } + + if (flag == 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not find VM(Pid %s) in '%s'"), pidToStr, completePath); + goto cleanup; + } + + if (virGetDebugFsKvmValue(ent, completePath, "halt_poll_success_ns", haltPollSuccess) < 0 || + virGetDebugFsKvmValue(ent, completePath, "halt_poll_fail_ns", haltPollFail) < 0) { + goto cleanup; + } + + ret = 0; +cleanup: + closedir(dir); + return ret; +} diff --git a/src/util/virutil.h b/src/util/virutil.h index 851b421476..00cef47208 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -228,3 +228,12 @@ int virPipeNonBlock(int fds[2]); int virGetValueRaw(const char *path, char **value); + +int virGetDebugFsKvmValue(struct dirent *ent, + const char *path, + const char *filename, + unsigned long long *value); + +int virGetCpuHaltPollTime(pid_t pid, + unsigned long long *haltPollSuccess, + unsigned long long *haltPollFail); -- 2.23.0
On 7/14/21 2:28 PM, Yang Fei wrote:
Add helper function virGetCpuHaltPollTime and virGetDebugFsKvmValue to obtain the halt polling time. If system mount debugfs, and the kernel support halt polling time statistic, it will work.
Signed-off-by: Yang Fei <yangfei85@huawei.com> --- src/libvirt_private.syms | 2 ++ src/util/virutil.c | 78 ++++++++++++++++++++++++++++++++++++++++ src/util/virutil.h | 9 +++++ 3 files changed, 89 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 68e4b6aab8..f92213b8c2 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3479,6 +3479,8 @@ virDoesUserExist; virDoubleToStr; virFormatIntDecimal; virFormatIntPretty; +virGetCpuHaltPollTime; +virGetDebugFsKvmValue; virGetDeviceID; virGetDeviceUnprivSGIO; virGetGroupID; diff --git a/src/util/virutil.c b/src/util/virutil.c index c0d25fe247..7e4531b4b4 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -1959,3 +1959,81 @@ virGetValueRaw(const char *path,
return 0; } + +int +virGetDebugFsKvmValue(struct dirent *ent, + const char *path, + const char *filename, + unsigned long long *value) +{ + g_autofree char *valToStr = NULL; + g_autofree char *valPath = NULL; + + valPath = g_strdup_printf("%s/%s/%s", path, ent->d_name, filename); + + if (virGetValueRaw(valPath, &valToStr) < 0) { + return -1; + } + + /* 10 is a Cardinality, must be between 2 and 36 inclusive, + * or special value 0. Used in fuction strtoull() + */ + if (virStrToLong_ull(valToStr, NULL, 10, value) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to parse '%s' as an integer"), valToStr); + return -1; + } + + return 0; +}
This looks very close to what virFileReadValueUllong() does.
+ +int +virGetCpuHaltPollTime(pid_t pid, + unsigned long long *haltPollSuccess, + unsigned long long *haltPollFail) +{ + g_autofree char *pidToStr = NULL; + g_autofree char *debugFsPath = NULL; + g_autofree char *completePath = NULL; + struct dirent *ent = NULL; + DIR *dir = NULL; + int ret = -1; + int flag = 0; + + if (!(debugFsPath = virFileFindMountPoint("debugfs"))) { + virReportSystemError(errno, "%s", + _("unable to find debugfs mountpoint")); + return ret; + }
Is this something worth polluting logs with error? What if we run under kernel that doesn't have debugfs?
+ + completePath = g_strdup_printf("%s/%s", debugFsPath, "kvm"); + if (virDirOpen(&dir, completePath) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s %s", "Can not open directory", completePath); + return ret; + }
virDirOpen() reports an error on failure. No need to rewrite it. And in the light of previous comment maybe virDirOpenIfExists() instead?
+ + pidToStr = g_strdup_printf("%lld%c", (unsigned long long)pid, '-'); + while (virDirRead(dir, &ent, NULL) > 0) { + if (STRPREFIX(ent->d_name, pidToStr)) { + flag = 1; + break; + } + } + + if (flag == 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not find VM(Pid %s) in '%s'"), pidToStr, completePath); + goto cleanup; + }
Again, not big fan. What it my domain runs TCG? Also, please rename 'flag' to something more specific, e.g. "found".
+ + if (virGetDebugFsKvmValue(ent, completePath, "halt_poll_success_ns", haltPollSuccess) < 0 || + virGetDebugFsKvmValue(ent, completePath, "halt_poll_fail_ns", haltPollFail) < 0) { + goto cleanup; + } + + ret = 0; +cleanup: + closedir(dir);
Again, please make sure 'ninja test' passes. We have a syntax-check rule that forbids direct call of closedir(). The proper way is to define dir variable like this: g_autoptr(DIR) dir = NULL;
+ return ret; +} diff --git a/src/util/virutil.h b/src/util/virutil.h index 851b421476..00cef47208 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -228,3 +228,12 @@ int virPipeNonBlock(int fds[2]);
int virGetValueRaw(const char *path, char **value); + +int virGetDebugFsKvmValue(struct dirent *ent, + const char *path, + const char *filename, + unsigned long long *value); + +int virGetCpuHaltPollTime(pid_t pid, + unsigned long long *haltPollSuccess, + unsigned long long *haltPollFail);
Michal
On 2021/7/14 22:17, Michal Prívozník wrote:
On 7/14/21 2:28 PM, Yang Fei wrote:
Add helper function virGetCpuHaltPollTime and virGetDebugFsKvmValue to obtain the halt polling time. If system mount debugfs, and the kernel support halt polling time statistic, it will work.
Signed-off-by: Yang Fei <yangfei85@huawei.com> --- src/libvirt_private.syms | 2 ++ src/util/virutil.c | 78 ++++++++++++++++++++++++++++++++++++++++ src/util/virutil.h | 9 +++++ 3 files changed, 89 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 68e4b6aab8..f92213b8c2 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3479,6 +3479,8 @@ virDoesUserExist; virDoubleToStr; virFormatIntDecimal; virFormatIntPretty; +virGetCpuHaltPollTime; +virGetDebugFsKvmValue; virGetDeviceID; virGetDeviceUnprivSGIO; virGetGroupID; diff --git a/src/util/virutil.c b/src/util/virutil.c index c0d25fe247..7e4531b4b4 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -1959,3 +1959,81 @@ virGetValueRaw(const char *path,
return 0; } + +int +virGetDebugFsKvmValue(struct dirent *ent, + const char *path, + const char *filename, + unsigned long long *value) +{ + g_autofree char *valToStr = NULL; + g_autofree char *valPath = NULL; + + valPath = g_strdup_printf("%s/%s/%s", path, ent->d_name, filename); + + if (virGetValueRaw(valPath, &valToStr) < 0) { + return -1; + } + + /* 10 is a Cardinality, must be between 2 and 36 inclusive, + * or special value 0. Used in fuction strtoull() + */ + if (virStrToLong_ull(valToStr, NULL, 10, value) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to parse '%s' as an integer"), valToStr); + return -1; + } + + return 0; +}
This looks very close to what virFileReadValueUllong() does.
I will directly use the function virFileReadValueUllong() in the next version.
+ +int +virGetCpuHaltPollTime(pid_t pid, + unsigned long long *haltPollSuccess, + unsigned long long *haltPollFail) +{ + g_autofree char *pidToStr = NULL; + g_autofree char *debugFsPath = NULL; + g_autofree char *completePath = NULL; + struct dirent *ent = NULL; + DIR *dir = NULL; + int ret = -1; + int flag = 0; + + if (!(debugFsPath = virFileFindMountPoint("debugfs"))) { + virReportSystemError(errno, "%s", + _("unable to find debugfs mountpoint")); + return ret; + }
Is this something worth polluting logs with error? What if we run under kernel that doesn't have debugfs?
If kernel doesn't have debugfs, halt polling time will not be presented in domstats, and return what it can obtain. Could I use VIR_WARN() or VIR_INFO() to give the fail messege?
+ + completePath = g_strdup_printf("%s/%s", debugFsPath, "kvm"); + if (virDirOpen(&dir, completePath) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s %s", "Can not open directory", completePath); + return ret; + }
virDirOpen() reports an error on failure. No need to rewrite it. And in the light of previous comment maybe virDirOpenIfExists() instead?
Yes, virDirOpenIfExists() is indeed more appropriate. I will fix it next version.
+ + pidToStr = g_strdup_printf("%lld%c", (unsigned long long)pid, '-'); + while (virDirRead(dir, &ent, NULL) > 0) { + if (STRPREFIX(ent->d_name, pidToStr)) { + flag = 1; + break; + } + } + + if (flag == 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not find VM(Pid %s) in '%s'"), pidToStr, completePath); + goto cleanup; + }
Again, not big fan. What it my domain runs TCG? Also, please rename 'flag' to something more specific, e.g. "found".
If domain runs TCG, the pid-fd directory will not be generated in the path. Domstats just return the data it can obtain. That's really not appropriate to report a error here. And I'll give the variable a more appropriate name in next version.
+ + if (virGetDebugFsKvmValue(ent, completePath, "halt_poll_success_ns", haltPollSuccess) < 0 || + virGetDebugFsKvmValue(ent, completePath, "halt_poll_fail_ns", haltPollFail) < 0) { + goto cleanup; + } + + ret = 0; +cleanup: + closedir(dir);
Again, please make sure 'ninja test' passes. We have a syntax-check rule that forbids direct call of closedir(). The proper way is to define dir variable like this:
g_autoptr(DIR) dir = NULL;
I'll pay more attention about it and modify the definition method in next version.
+ return ret; +} diff --git a/src/util/virutil.h b/src/util/virutil.h index 851b421476..00cef47208 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -228,3 +228,12 @@ int virPipeNonBlock(int fds[2]);
int virGetValueRaw(const char *path, char **value); + +int virGetDebugFsKvmValue(struct dirent *ent, + const char *path, + const char *filename, + unsigned long long *value); + +int virGetCpuHaltPollTime(pid_t pid, + unsigned long long *haltPollSuccess, + unsigned long long *haltPollFail);
Michal
.
This function is used to obtain the halt polling time. The kernel provides statistics about haltpollsuccess.time and haltpollfail.time. We add it in domstats, so that we can use command 'virsh domstats VM' to get it if system support. Signed-off-by: Yang Fei <yangfei85@huawei.com> --- src/qemu/qemu_driver.c | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 235f575901..6163037ec3 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17839,6 +17839,31 @@ qemuDomainGetStatsCpuCgroup(virDomainObj *dom, return 0; } +#ifdef __linux__ +static int +qemuDomainGetStatsCpuHaltPollTime(virDomainObj *dom, + virTypedParamList *params) +{ + unsigned long long haltPollSuccess = 0; + unsigned long long haltPollFail = 0; + pid_t pid = dom->pid; + int err = 0; + + err = virGetCpuHaltPollTime(pid, &haltPollSuccess, &haltPollFail); + if (!err && virTypedParamListAddULLong(params, haltPollSuccess, "haltpollsuccess.time") < 0) + return -1; + if (!err && virTypedParamListAddULLong(params, haltPollFail, "haltpollfail.time") < 0) + return -1; + return 0; +} +#else +static int +qemuDomainGetStatsCpuHaltPollTime(virDomainObj *dom, + virTypedParamList *params) +{ + return 0; +} +#endif static int qemuDomainGetStatsCpu(virQEMUDriver *driver, @@ -17852,6 +17877,9 @@ qemuDomainGetStatsCpu(virQEMUDriver *driver, if (qemuDomainGetStatsCpuCache(driver, dom, params) < 0) return -1; + if (qemuDomainGetStatsCpuHaltPollTime(dom, params) < 0) + return -1; + return 0; } -- 2.23.0
On 7/14/21 2:28 PM, Yang Fei wrote:
This function is used to obtain the halt polling time. The kernel provides statistics about haltpollsuccess.time and haltpollfail.time. We add it in domstats, so that we can use command 'virsh domstats VM' to get it if system support.
Signed-off-by: Yang Fei <yangfei85@huawei.com> --- src/qemu/qemu_driver.c | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 235f575901..6163037ec3 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17839,6 +17839,31 @@ qemuDomainGetStatsCpuCgroup(virDomainObj *dom, return 0; }
+#ifdef __linux__ +static int +qemuDomainGetStatsCpuHaltPollTime(virDomainObj *dom, + virTypedParamList *params) +{ + unsigned long long haltPollSuccess = 0; + unsigned long long haltPollFail = 0; + pid_t pid = dom->pid; + int err = 0; + + err = virGetCpuHaltPollTime(pid, &haltPollSuccess, &haltPollFail); + if (!err && virTypedParamListAddULLong(params, haltPollSuccess, "haltpollsuccess.time") < 0) + return -1; + if (!err && virTypedParamListAddULLong(params, haltPollFail, "haltpollfail.time") < 0) + return -1;
I know you copied this pattern from qemuDomainGetStatsCpuCgroup() but it's not as appealing is it could be. How about: if (virGetCpuHaltPollTime() < 0) return 0; if (virTypedParamListAddULLong() < 0 || virTypedParamListAddULLong() < 0) return -1; return 0; Also, please document these new parameters in src/libvirt-domain.c just like every other parameter is (look around line 11626).
+ return 0; +} +#else +static int +qemuDomainGetStatsCpuHaltPollTime(virDomainObj *dom, + virTypedParamList *params) +{ + return 0; +} +#endif
Once my suggestions from previous patches are worked in, this non-Linux variant won't be necessary.
static int qemuDomainGetStatsCpu(virQEMUDriver *driver, @@ -17852,6 +17877,9 @@ qemuDomainGetStatsCpu(virQEMUDriver *driver, if (qemuDomainGetStatsCpuCache(driver, dom, params) < 0) return -1;
+ if (qemuDomainGetStatsCpuHaltPollTime(dom, params) < 0) + return -1; + return 0; }
Michal
On 2021/7/14 22:17, Michal Prívozník wrote:
On 7/14/21 2:28 PM, Yang Fei wrote:
This function is used to obtain the halt polling time. The kernel provides statistics about haltpollsuccess.time and haltpollfail.time. We add it in domstats, so that we can use command 'virsh domstats VM' to get it if system support.
Signed-off-by: Yang Fei <yangfei85@huawei.com> --- src/qemu/qemu_driver.c | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 235f575901..6163037ec3 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17839,6 +17839,31 @@ qemuDomainGetStatsCpuCgroup(virDomainObj *dom, return 0; }
+#ifdef __linux__ +static int +qemuDomainGetStatsCpuHaltPollTime(virDomainObj *dom, + virTypedParamList *params) +{ + unsigned long long haltPollSuccess = 0; + unsigned long long haltPollFail = 0; + pid_t pid = dom->pid; + int err = 0; + + err = virGetCpuHaltPollTime(pid, &haltPollSuccess, &haltPollFail); + if (!err && virTypedParamListAddULLong(params, haltPollSuccess, "haltpollsuccess.time") < 0) + return -1; + if (!err && virTypedParamListAddULLong(params, haltPollFail, "haltpollfail.time") < 0) + return -1;
I know you copied this pattern from qemuDomainGetStatsCpuCgroup() but it's not as appealing is it could be. How about:
if (virGetCpuHaltPollTime() < 0) return 0;
if (virTypedParamListAddULLong() < 0 || virTypedParamListAddULLong() < 0) return -1;
return 0;
Also, please document these new parameters in src/libvirt-domain.c just like every other parameter is (look around line 11626).
OK, I'll add it in next version.
+ return 0; +} +#else +static int +qemuDomainGetStatsCpuHaltPollTime(virDomainObj *dom, + virTypedParamList *params) +{ + return 0; +} +#endif
Once my suggestions from previous patches are worked in, this non-Linux variant won't be necessary.
static int qemuDomainGetStatsCpu(virQEMUDriver *driver, @@ -17852,6 +17877,9 @@ qemuDomainGetStatsCpu(virQEMUDriver *driver, if (qemuDomainGetStatsCpuCache(driver, dom, params) < 0) return -1;
+ if (qemuDomainGetStatsCpuHaltPollTime(dom, params) < 0) + return -1; + return 0; }
Michal
.
participants (2)
-
Michal Prívozník -
Yang Fei