[PATCH] domstats:add haltpolling time statistic interface

Yang Fei posted 1 patch 2 years, 10 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20210702045329.32288-1-yangfei85@huawei.com
There is a newer version of this series
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(+)
[PATCH] domstats:add haltpolling time statistic interface
Posted by Yang Fei 2 years, 10 months ago
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
 <cb953129bfe5c0f2da835a0469930873fb7e71df>. 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 <yangfei85@huawei.com>
---
 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, &haltPollSuccess, &haltPollFail) != 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, &valToStr)) < 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(&dir, completePath) < 0) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       "%s %s", "Can not open directory", completePath);
+        return ret;
+    }
+
+    pidToStr = g_strdup_printf("%d", pid);
+    while (virDirRead(dir, &ent, NULL) > 0) {
+        if (strncmp(ent->d_name, pidToStr, strlen(pidToStr)) == 0 &&
+            ent->d_name[strlen(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 854b494890..b3c1e8a0bc 100644
--- a/src/util/virutil.h
+++ b/src/util/virutil.h
@@ -225,3 +225,12 @@ int virPipeQuiet(int fds[2]);
  * Returns: -1 on error, 0 on success
  */
 int virPipeNonBlock(int fds[2]);
+
+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


Re: [PATCH] domstats:add haltpolling time statistic interface
Posted by Peter Krempa 2 years, 10 months ago
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
>  <cb953129bfe5c0f2da835a0469930873fb7e71df>. 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 <yangfei85@huawei.com>
> ---
>  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, &haltPollSuccess, &haltPollFail) != 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, &valToStr)) < 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, value) < 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Unable to parse '%s' as an integer"), valToStr);
> +        goto cleanup;
> +    }
> +
> +    ret = 0;
> +cleanup:

No need for cleanup label since it's not cleaning up anything.

> +    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(&dir, completePath) < 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       "%s %s", "Can not open directory", completePath);
> +        return ret;
> +    }
> +
> +    pidToStr = g_strdup_printf("%d", pid);

%d for pid_t is invalid.

> +    while (virDirRead(dir, &ent, NULL) > 0) {
> +        if (strncmp(ent->d_name, pidToStr, strlen(pidToStr)) == 0 &&

We mandate use of our wrappers, in this case STRPREFIX

> +            ent->d_name[strlen(pidToStr)] == '-') {

Formatting the pidToStr including the '-' will simplify the code since
you can match the full prefix.

> +            flag = 1;
> +            break;
> +        }
> +    }
> +
> +    if (flag == 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Could not find VM(Pid %s) in '%s'"), pidToStr, completePath);

Reporting errors in optional stats code is not acceptable. The domstats
code is best-effort and should return what it can.

> +        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;
> +}

Re: [PATCH] domstats:add haltpolling time statistic interface
Posted by Yang Fei 2 years, 10 months ago

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
>>  <cb953129bfe5c0f2da835a0469930873fb7e71df>. 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 <yangfei85@huawei.com>
>> ---
>>  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, &haltPollSuccess, &haltPollFail) != 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, &valToStr)) < 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.

>> +        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.
> 

The code under this comment is copied from virCgroupGetValueRaw
which can only be used to obtain Cgroup data.
I plan extract this function to virutil.c in an independent patch. So that
we can call this function directly.

>> +    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?
> 

Since 10 here is a magic number, I've given an comment to explain it.

>> +    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:
> 
> No need for cleanup label since it's not cleaning up anything.
> 

OK, I will remove it in the next version.

>> +    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(&dir, completePath) < 0) {
>> +        virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                       "%s %s", "Can not open directory", completePath);
>> +        return ret;
>> +    }
>> +
>> +    pidToStr = g_strdup_printf("%d", pid);
> 
> %d for pid_t is invalid.
> 

Should I use %lld?

>> +    while (virDirRead(dir, &ent, NULL) > 0) {
>> +        if (strncmp(ent->d_name, pidToStr, strlen(pidToStr)) == 0 &&
> 
> We mandate use of our wrappers, in this case STRPREFIX
> 
>> +            ent->d_name[strlen(pidToStr)] == '-') {
> 
> Formatting the pidToStr including the '-' will simplify the code since
> you can match the full prefix.
> 

OK, I'll use the STRPREFIX in next version.

>> +            flag = 1;
>> +            break;
>> +        }
>> +    }
>> +
>> +    if (flag == 0) {
>> +        virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                       _("Could not find VM(Pid %s) in '%s'"), pidToStr, completePath);
> 
> Reporting errors in optional stats code is not acceptable. The domstats
> code is best-effort and should return what it can.
> 

So, the function should not report any errors or return any error number in any case.  If something
happends, e.g. debugfs isn't mounted, the caller should not add halt_poll_success_ns and
halt_poll_fail_ns to domstats.  Have I got that right?

>> +        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;
>> +}
> 
> .
> 

Thanks,
Fei.