Re: [PATCH] domstats:add haltpolling time statistic interface
On 2021/7/2 15:45, Peter Krempa wrote:
> On Fri, Jul 02, 2021 at 12:53:29 +0800, Yang Fei wrote:
>> This patch add the ability to statistic the halt polling time when
>> VM execute HLT(arm is WFI).
>>
>> In actual services, the execution of the HLT instruction by the
>> guest is an important cause of virtualization overhead. The halt
>> polling feature is introduced to solve this problem. When a guest
>> idle VM exit occurs, the host continues polling for a period of
>> time to reduce the guest service delay. This mechanism may cause
>> the CPU usage to be 100% when the physical CPU is idle. If the
>> guest service model is woken up at an interval to process a small
>> amount of traffic, and the interval is shorter than kvm halt_poll_ns.
>> The host polls the block time of the entire VM and the CPU usage
>> increases to 100%.
>>
>> The kernel provides the capability of collecting statistics on the
>> halt polling time after v5.8, Introduced by commit
>> . It is rendered in debugfs.
>> Therefore, we can use this kernel feature to provide the halt poll
>> time to the user to obtain a more accurate CPU usage as required.
>
> Note that I'm reviewing just the code side, not the justification or
> usefullnes of the data reported.
>
>>
>> Signed-off-by: Yang Fei
>> ---
>> src/libvirt_private.syms | 2 +
>> src/qemu/qemu_driver.c | 29 +
>> src/util/virutil.c | 89
>> src/util/virutil.h | 9
>> 4 files changed, 129 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/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 235f575901..3a2b530ecf 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -17812,6 +17812,32 @@ qemuDomainGetStatsCpuCache(virQEMUDriver *driver,
>> return ret;
>> }
>>
>> +#ifdef __linux__
>> +static int
>> +qemuDomainGetStatsCpuHaltPollTime(virDomainObj *dom,
>> + virTypedParamList *params)
>> +{
>> +unsigned long long haltPollSuccess = 0;
>> +unsigned long long haltPollFail = 0;
>> +pid_t pid = dom->pid;
>> +
>> +if (virGetCpuHaltPollTime(pid, , ) != 0)
>> +return -1;
>> +if (virTypedParamListAddULLong(params, haltPollSuccess,
>> "haltpollsuccess.time") < 0)
>> +return -1;
>> +if (virTypedParamListAddULLong(params, haltPollFail,
>> "haltpollfail.time") < 0)
>> +return -1;
>> +
>> +return 0;
>> +}
>> +#else
>> +static int
>> +qemuDomainGetStatsCpuHaltPollTime(virDomainObj *dom,
>> + virTypedParamList *params)
>> +{
>> +return -1;
>
> This breaks domstats on non-linux ...
>
>> +}
>> +#endif
>>
>> static int
>> qemuDomainGetStatsCpuCgroup(virDomainObj *dom,
>> @@ -17852,6 +17878,9 @@ qemuDomainGetStatsCpu(virQEMUDriver *driver,
>> if (qemuDomainGetStatsCpuCache(driver, dom, params) < 0)
>> return -1;
>>
>> +if (qemuDomainGetStatsCpuHaltPollTime(dom, params) < 0)
>> +return -1;
>
> since you'd return -1 here.
>
> Additionally the qemuDomainGetStatsCpuHaltPollTime function IMO should
> not be conditionally compiled on linux. The helpers below should and the
> qemu code should just conditionally fill in the data if it's available.
>
My consideration is not to compile this feature on non-Linux platforms,
since the function of get halt polling time is based on the capabilities of
Linux kernel.
How about return 0 in non-linux platform?
>> +
>> return 0;
>> }
>>
>> diff --git a/src/util/virutil.c b/src/util/virutil.c
>> index 311cbbf93a..8715deaca5 100644
>> --- a/src/util/virutil.c
>> +++ b/src/util/virutil.c
>> @@ -1936,3 +1936,92 @@ virPipeNonBlock(int fds[2])
>> {
>> return virPipeImpl(fds, true, true);
>> }
>> +
>> +int
>> +virGetDebugFsKvmValue(struct dirent *ent,
>> +const char *path,
>> +const char *filename,
>> +unsigned long long *value)
>
> This helper should be added in a separate patch, to separate it from the
> other changes, especially the qemu driver change.
>
I will remove it to a separate patch in next version.
>> +{
>> +g_autofree char *valToStr = NULL;
>> +g_autofree char *valPath = NULL;
>> +int rc = -1;
>> +int ret = -1;
>> +
>> +valPath = g_strdup_printf("%s/%s/%s", path, ent->d_name, filename);
>> +
>> +if ((rc = virFileReadAll(valPath, 16, )) < 0) {
>
> Why just 16 bytes?
>
Cause the data type of halt_poll_success_ns and halt_poll_fail_ns is u64,
it only requires 8 bytes of memory.
>> +
Re: [PATCH] domstats:add haltpolling time statistic interface
On Fri, Jul 02, 2021 at 12:53:29 +0800, Yang Fei wrote:
> This patch add the ability to statistic the halt polling time when
> VM execute HLT(arm is WFI).
>
> In actual services, the execution of the HLT instruction by the
> guest is an important cause of virtualization overhead. The halt
> polling feature is introduced to solve this problem. When a guest
> idle VM exit occurs, the host continues polling for a period of
> time to reduce the guest service delay. This mechanism may cause
> the CPU usage to be 100% when the physical CPU is idle. If the
> guest service model is woken up at an interval to process a small
> amount of traffic, and the interval is shorter than kvm halt_poll_ns.
> The host polls the block time of the entire VM and the CPU usage
> increases to 100%.
>
> The kernel provides the capability of collecting statistics on the
> halt polling time after v5.8, Introduced by commit
> . It is rendered in debugfs.
> Therefore, we can use this kernel feature to provide the halt poll
> time to the user to obtain a more accurate CPU usage as required.
Note that I'm reviewing just the code side, not the justification or
usefullnes of the data reported.
>
> Signed-off-by: Yang Fei
> ---
> src/libvirt_private.syms | 2 +
> src/qemu/qemu_driver.c | 29 +
> src/util/virutil.c | 89
> src/util/virutil.h | 9
> 4 files changed, 129 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/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 235f575901..3a2b530ecf 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -17812,6 +17812,32 @@ qemuDomainGetStatsCpuCache(virQEMUDriver *driver,
> return ret;
> }
>
> +#ifdef __linux__
> +static int
> +qemuDomainGetStatsCpuHaltPollTime(virDomainObj *dom,
> + virTypedParamList *params)
> +{
> +unsigned long long haltPollSuccess = 0;
> +unsigned long long haltPollFail = 0;
> +pid_t pid = dom->pid;
> +
> +if (virGetCpuHaltPollTime(pid, , ) != 0)
> +return -1;
> +if (virTypedParamListAddULLong(params, haltPollSuccess,
> "haltpollsuccess.time") < 0)
> +return -1;
> +if (virTypedParamListAddULLong(params, haltPollFail,
> "haltpollfail.time") < 0)
> +return -1;
> +
> +return 0;
> +}
> +#else
> +static int
> +qemuDomainGetStatsCpuHaltPollTime(virDomainObj *dom,
> + virTypedParamList *params)
> +{
> +return -1;
This breaks domstats on non-linux ...
> +}
> +#endif
>
> static int
> qemuDomainGetStatsCpuCgroup(virDomainObj *dom,
> @@ -17852,6 +17878,9 @@ qemuDomainGetStatsCpu(virQEMUDriver *driver,
> if (qemuDomainGetStatsCpuCache(driver, dom, params) < 0)
> return -1;
>
> +if (qemuDomainGetStatsCpuHaltPollTime(dom, params) < 0)
> +return -1;
since you'd return -1 here.
Additionally the qemuDomainGetStatsCpuHaltPollTime function IMO should
not be conditionally compiled on linux. The helpers below should and the
qemu code should just conditionally fill in the data if it's available.
> +
> return 0;
> }
>
> diff --git a/src/util/virutil.c b/src/util/virutil.c
> index 311cbbf93a..8715deaca5 100644
> --- a/src/util/virutil.c
> +++ b/src/util/virutil.c
> @@ -1936,3 +1936,92 @@ virPipeNonBlock(int fds[2])
> {
> return virPipeImpl(fds, true, true);
> }
> +
> +int
> +virGetDebugFsKvmValue(struct dirent *ent,
> +const char *path,
> +const char *filename,
> +unsigned long long *value)
This helper should be added in a separate patch, to separate it from the
other changes, especially the qemu driver change.
> +{
> +g_autofree char *valToStr = NULL;
> +g_autofree char *valPath = NULL;
> +int rc = -1;
> +int ret = -1;
> +
> +valPath = g_strdup_printf("%s/%s/%s", path, ent->d_name, filename);
> +
> +if ((rc = virFileReadAll(valPath, 16, )) < 0) {
Why just 16 bytes?
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unable to read from '%s'"), valPath);
> +goto cleanup;
> +}
> +
> +/* Terminated with '\n' has sometimes harmful effects to the caller */
To the caller? I don't quite understand what you mean here.
> +if (rc > 0 && (valToStr)[rc - 1] == '\n')
> +(valToStr)[rc - 1] = '\0';
> +
> +/* 10 is a Cardinality, must be between 2 and 36 inclusive,
> + * or special value 0. Used in fuction strtoull()
> + */
What's the point of this comment?
> +if (virStrToLong_ull(valToStr, NULL, 10,
[PATCH] domstats:add haltpolling time statistic interface
This patch add the ability to statistic the halt polling time when
VM execute HLT(arm is WFI).
In actual services, the execution of the HLT instruction by the
guest is an important cause of virtualization overhead. The halt
polling feature is introduced to solve this problem. When a guest
idle VM exit occurs, the host continues polling for a period of
time to reduce the guest service delay. This mechanism may cause
the CPU usage to be 100% when the physical CPU is idle. If the
guest service model is woken up at an interval to process a small
amount of traffic, and the interval is shorter than kvm halt_poll_ns.
The host polls the block time of the entire VM and the CPU usage
increases to 100%.
The kernel provides the capability of collecting statistics on the
halt polling time after v5.8, Introduced by commit
. It is rendered in debugfs.
Therefore, we can use this kernel feature to provide the halt poll
time to the user to obtain a more accurate CPU usage as required.
Signed-off-by: Yang Fei
---
src/libvirt_private.syms | 2 +
src/qemu/qemu_driver.c | 29 +
src/util/virutil.c | 89
src/util/virutil.h | 9
4 files changed, 129 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/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 235f575901..3a2b530ecf 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -17812,6 +17812,32 @@ qemuDomainGetStatsCpuCache(virQEMUDriver *driver,
return ret;
}
+#ifdef __linux__
+static int
+qemuDomainGetStatsCpuHaltPollTime(virDomainObj *dom,
+ virTypedParamList *params)
+{
+unsigned long long haltPollSuccess = 0;
+unsigned long long haltPollFail = 0;
+pid_t pid = dom->pid;
+
+if (virGetCpuHaltPollTime(pid, , ) != 0)
+return -1;
+if (virTypedParamListAddULLong(params, haltPollSuccess,
"haltpollsuccess.time") < 0)
+return -1;
+if (virTypedParamListAddULLong(params, haltPollFail, "haltpollfail.time")
< 0)
+return -1;
+
+return 0;
+}
+#else
+static int
+qemuDomainGetStatsCpuHaltPollTime(virDomainObj *dom,
+ virTypedParamList *params)
+{
+return -1;
+}
+#endif
static int
qemuDomainGetStatsCpuCgroup(virDomainObj *dom,
@@ -17852,6 +17878,9 @@ qemuDomainGetStatsCpu(virQEMUDriver *driver,
if (qemuDomainGetStatsCpuCache(driver, dom, params) < 0)
return -1;
+if (qemuDomainGetStatsCpuHaltPollTime(dom, params) < 0)
+return -1;
+
return 0;
}
diff --git a/src/util/virutil.c b/src/util/virutil.c
index 311cbbf93a..8715deaca5 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -1936,3 +1936,92 @@ virPipeNonBlock(int fds[2])
{
return virPipeImpl(fds, true, true);
}
+
+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;
+int rc = -1;
+int ret = -1;
+
+valPath = g_strdup_printf("%s/%s/%s", path, ent->d_name, filename);
+
+if ((rc = virFileReadAll(valPath, 16, )) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unable to read from '%s'"), valPath);
+goto cleanup;
+}
+
+/* Terminated with '\n' has sometimes harmful effects to the caller */
+if (rc > 0 && (valToStr)[rc - 1] == '\n')
+(valToStr)[rc - 1] = '\0';
+
+/* 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);
+goto cleanup;
+}
+
+ret = 0;
+cleanup:
+return ret;
+}
+
+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"));
+goto cleanup;
+}
+
+completePath = g_strdup_printf("%s/%s", debugFsPath, "kvm");
+if (virDirOpen(, completePath) < 0) {
+
