7:28 a.m.
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
7:28 a.m.
New subject: [PATCH v2 1/3] util: Move virGetCgroupValueRaw to vircgroup.c and rename it virGetValueRaw
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(a)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
9:17 a.m.
New subject: [PATCH v2 1/3] util: Move virGetCgroupValueRaw to vircgroup.c and rename it virGetValueRaw
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(a)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
8:41 a.m.
New subject: [PATCH v2 1/3] util: Move virGetCgroupValueRaw to vircgroup.c and rename it virGetValueRaw
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(a)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.
7:28 a.m.
New subject: [PATCH v2 2/3] util: Add virGetCpuHaltPollTime and virGetDebugFsKvmValue
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(a)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
9:17 a.m.
New subject: [PATCH v2 2/3] util: Add virGetCpuHaltPollTime and virGetDebugFsKvmValue
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(a)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
8:41 a.m.
New subject: [PATCH v2 2/3] util: Add virGetCpuHaltPollTime and virGetDebugFsKvmValue
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(a)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
.
7:28 a.m.
New subject: [PATCH v2 3/3] qemu: Introduce qemuDomainGetStatsCpuHaltPollTime
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(a)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
9:17 a.m.
New subject: [PATCH v2 3/3] qemu: Introduce qemuDomainGetStatsCpuHaltPollTime
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(a)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
8:41 a.m.
New subject: [PATCH v2 3/3] qemu: Introduce qemuDomainGetStatsCpuHaltPollTime
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(a)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
.
1228
days inactive
1229
days old
9 comments
2 participants
participants (2)
-
Michal Prívozník -
Yang Fei