Add helper function virGetCpuHaltPollTime to obtain halt polling
time. If the kernel support halt polling time statistic, and mount
debugfs. This function will take effect on KVM VMs.
Signed-off-by: Yang Fei <yangfei85@huawei.com>
---
src/libvirt_private.syms | 1 +
src/util/virutil.c | 43 ++++++++++++++++++++++++++++++++++++++++
src/util/virutil.h | 4 ++++
3 files changed, 48 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 68e4b6aab8..64aff4eca4 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -3479,6 +3479,7 @@ virDoesUserExist;
virDoubleToStr;
virFormatIntDecimal;
virFormatIntPretty;
+virGetCpuHaltPollTime;
virGetDeviceID;
virGetDeviceUnprivSGIO;
virGetGroupID;
diff --git a/src/util/virutil.c b/src/util/virutil.c
index 311cbbf93a..f5304644c0 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -1936,3 +1936,46 @@ virPipeNonBlock(int fds[2])
{
return virPipeImpl(fds, true, true);
}
+
+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;
+ g_autoptr(DIR) dir = NULL;
+ int ret = -1;
+ bool found = false;
+
+ if (!(debugFsPath = virFileFindMountPoint("debugfs")))
+ return ret;
+
+ completePath = g_strdup_printf("%s/%s", debugFsPath, "kvm");
+ if (virDirOpenIfExists(&dir, completePath) != 1)
+ return ret;
+
+ pidToStr = g_strdup_printf("%d%c", pid, '-');
+ while (virDirRead(dir, &ent, NULL) > 0) {
+ if (STRPREFIX(ent->d_name, pidToStr)) {
+ found = true;
+ break;
+ }
+ }
+
+ if (!found)
+ return ret;
+
+ if (virFileReadValueUllong(haltPollSuccess, "%s/%s/%s", completePath,
+ ent->d_name, "halt_poll_success_ns") < 0
+ || virFileReadValueUllong(haltPollFail, "%s/%s/%s", completePath,
+ ent->d_name, "halt_poll_fail_ns") < 0) {
+ return ret;
+ }
+
+ ret = 0;
+
+ return ret;
+}
diff --git a/src/util/virutil.h b/src/util/virutil.h
index 854b494890..03b225185f 100644
--- a/src/util/virutil.h
+++ b/src/util/virutil.h
@@ -225,3 +225,7 @@ int virPipeQuiet(int fds[2]);
* Returns: -1 on error, 0 on success
*/
int virPipeNonBlock(int fds[2]);
+
+int virGetCpuHaltPollTime(pid_t pid,
+ unsigned long long *haltPollSuccess,
+ unsigned long long *haltPollFail);
--
2.23.0
On Fri, Jul 16, 2021 at 18:42:22 +0800, Yang Fei wrote:
> Add helper function virGetCpuHaltPollTime to obtain halt polling
> time. If the kernel support halt polling time statistic, and mount
> debugfs. This function will take effect on KVM VMs.
>
> Signed-off-by: Yang Fei <yangfei85@huawei.com>
> ---
> src/libvirt_private.syms | 1 +
> src/util/virutil.c | 43 ++++++++++++++++++++++++++++++++++++++++
> src/util/virutil.h | 4 ++++
> 3 files changed, 48 insertions(+)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 68e4b6aab8..64aff4eca4 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -3479,6 +3479,7 @@ virDoesUserExist;
> virDoubleToStr;
> virFormatIntDecimal;
> virFormatIntPretty;
> +virGetCpuHaltPollTime;
> virGetDeviceID;
> virGetDeviceUnprivSGIO;
> virGetGroupID;
> diff --git a/src/util/virutil.c b/src/util/virutil.c
> index 311cbbf93a..f5304644c0 100644
> --- a/src/util/virutil.c
> +++ b/src/util/virutil.c
> @@ -1936,3 +1936,46 @@ virPipeNonBlock(int fds[2])
> {
> return virPipeImpl(fds, true, true);
> }
> +
> +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;
> + g_autoptr(DIR) dir = NULL;
> + int ret = -1;
> + bool found = false;
> +
> + if (!(debugFsPath = virFileFindMountPoint("debugfs")))
> + return ret;
Here '-1' is returned, but no libvirt error is reported ...
> +
> + completePath = g_strdup_printf("%s/%s", debugFsPath, "kvm");
> + if (virDirOpenIfExists(&dir, completePath) != 1)
> + return ret;
While here -1 is reported and an error may be reported (e.g on EACCESS).
We generally avoid this pattern because the caller cant properly handle
that.
> +
> + pidToStr = g_strdup_printf("%d%c", pid, '-');
I'm fairly certain this will fail to compile on mingw since %d isn't the
proper formatting modifier for pid_t.
> + while (virDirRead(dir, &ent, NULL) > 0) {
> + if (STRPREFIX(ent->d_name, pidToStr)) {
Any idea what the suffix after '-' in the debugfs entry is? If we can
figure it out we won't have to look up the correct file.
> + found = true;
> + break;
> + }
> + }
> +
> + if (!found)
> + return ret;
> +
> + if (virFileReadValueUllong(haltPollSuccess, "%s/%s/%s", completePath,
> + ent->d_name, "halt_poll_success_ns") < 0
> + || virFileReadValueUllong(haltPollFail, "%s/%s/%s", completePath,
> + ent->d_name, "halt_poll_fail_ns") < 0) {
As you can see here, this is one of the functions having terrible
semantics. It sometimes does set the libvirt error object and sometimes
doesn't, thats why we try to avoid those.
> + return ret;
> + }
> +
> + ret = 0;
> +
> + return ret;
> +}
On 2021/7/19 17:05, Peter Krempa wrote:
> On Fri, Jul 16, 2021 at 18:42:22 +0800, Yang Fei wrote:
>> Add helper function virGetCpuHaltPollTime to obtain halt polling
>> time. If the kernel support halt polling time statistic, and mount
>> debugfs. This function will take effect on KVM VMs.
>>
>> Signed-off-by: Yang Fei <yangfei85@huawei.com>
>> ---
>> src/libvirt_private.syms | 1 +
>> src/util/virutil.c | 43 ++++++++++++++++++++++++++++++++++++++++
>> src/util/virutil.h | 4 ++++
>> 3 files changed, 48 insertions(+)
>>
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index 68e4b6aab8..64aff4eca4 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -3479,6 +3479,7 @@ virDoesUserExist;
>> virDoubleToStr;
>> virFormatIntDecimal;
>> virFormatIntPretty;
>> +virGetCpuHaltPollTime;
>> virGetDeviceID;
>> virGetDeviceUnprivSGIO;
>> virGetGroupID;
>> diff --git a/src/util/virutil.c b/src/util/virutil.c
>> index 311cbbf93a..f5304644c0 100644
>> --- a/src/util/virutil.c
>> +++ b/src/util/virutil.c
>> @@ -1936,3 +1936,46 @@ virPipeNonBlock(int fds[2])
>> {
>> return virPipeImpl(fds, true, true);
>> }
>> +
>> +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;
>> + g_autoptr(DIR) dir = NULL;
>> + int ret = -1;
>> + bool found = false;
>> +
>> + if (!(debugFsPath = virFileFindMountPoint("debugfs")))
>> + return ret;
>
> Here '-1' is returned, but no libvirt error is reported ...
>
>> +
>> + completePath = g_strdup_printf("%s/%s", debugFsPath, "kvm");
>> + if (virDirOpenIfExists(&dir, completePath) != 1)
>> + return ret;
>
> While here -1 is reported and an error may be reported (e.g on EACCESS).
> We generally avoid this pattern because the caller cant properly handle
> that.
>
>> +
>> + pidToStr = g_strdup_printf("%d%c", pid, '-');
>
> I'm fairly certain this will fail to compile on mingw since %d isn't the
> proper formatting modifier for pid_t.
>
In the last version, I try to use %lld but failed in the ninja test.
How about forcibly converts the variable pid to long long?
Like this:
g_strdup_printf("%lld%c", (long long)pid, '-')
I find that this is used elsewhere in the code.
>> + while (virDirRead(dir, &ent, NULL) > 0) {
>> + if (STRPREFIX(ent->d_name, pidToStr)) {
>
> Any idea what the suffix after '-' in the debugfs entry is? If we can
> figure it out we won't have to look up the correct file.
>
The full directory name format is "pid-fd", The fd is assigned during VM creation.
As I know, we can find it in /proc/(qemu-kvm pid)/fd, but also need to look up which
one points to "anon_inode:kvm-vm". That may not be any more efficient than look up
it in the kvm directory.
>> + found = true;
>> + break;
>> + }
>> + }
>> +
>> + if (!found)
>> + return ret;
>> +
>> + if (virFileReadValueUllong(haltPollSuccess, "%s/%s/%s", completePath,
>> + ent->d_name, "halt_poll_success_ns") < 0
>> + || virFileReadValueUllong(haltPollFail, "%s/%s/%s", completePath,
>> + ent->d_name, "halt_poll_fail_ns") < 0) {
>
> As you can see here, this is one of the functions having terrible
> semantics. It sometimes does set the libvirt error object and sometimes
> doesn't, thats why we try to avoid those.
>
>> + return ret;
>> + }
>> +
>> + ret = 0;
>> +
>> + return ret;
>> +}
>
> .
>
Thanks,
Fei.
On 7/20/21 4:55 AM, Yang Fei wrote:
>
>
> On 2021/7/19 17:05, Peter Krempa wrote:
>> On Fri, Jul 16, 2021 at 18:42:22 +0800, Yang Fei wrote:
>>> Add helper function virGetCpuHaltPollTime to obtain halt polling
>>> time. If the kernel support halt polling time statistic, and mount
>>> debugfs. This function will take effect on KVM VMs.
>>>
>>> Signed-off-by: Yang Fei <yangfei85@huawei.com>
>>> ---
>>> src/libvirt_private.syms | 1 +
>>> src/util/virutil.c | 43 ++++++++++++++++++++++++++++++++++++++++
>>> src/util/virutil.h | 4 ++++
>>> 3 files changed, 48 insertions(+)
>>>
>>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>>> index 68e4b6aab8..64aff4eca4 100644
>>> --- a/src/libvirt_private.syms
>>> +++ b/src/libvirt_private.syms
>>> @@ -3479,6 +3479,7 @@ virDoesUserExist;
>>> virDoubleToStr;
>>> virFormatIntDecimal;
>>> virFormatIntPretty;
>>> +virGetCpuHaltPollTime;
>>> virGetDeviceID;
>>> virGetDeviceUnprivSGIO;
>>> virGetGroupID;
>>> diff --git a/src/util/virutil.c b/src/util/virutil.c
>>> index 311cbbf93a..f5304644c0 100644
>>> --- a/src/util/virutil.c
>>> +++ b/src/util/virutil.c
>>> @@ -1936,3 +1936,46 @@ virPipeNonBlock(int fds[2])
>>> {
>>> return virPipeImpl(fds, true, true);
>>> }
>>> +
>>> +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;
>>> + g_autoptr(DIR) dir = NULL;
>>> + int ret = -1;
>>> + bool found = false;
>>> +
>>> + if (!(debugFsPath = virFileFindMountPoint("debugfs")))
>>> + return ret;
>>
>> Here '-1' is returned, but no libvirt error is reported ...
>>
>>> +
>>> + completePath = g_strdup_printf("%s/%s", debugFsPath, "kvm");
>>> + if (virDirOpenIfExists(&dir, completePath) != 1)
>>> + return ret;
>>
>> While here -1 is reported and an error may be reported (e.g on EACCESS).
>> We generally avoid this pattern because the caller cant properly handle
>> that.
>>
>>> +
>>> + pidToStr = g_strdup_printf("%d%c", pid, '-');
>>
>> I'm fairly certain this will fail to compile on mingw since %d isn't the
>> proper formatting modifier for pid_t.
>>
>
> In the last version, I try to use %lld but failed in the ninja test.
> How about forcibly converts the variable pid to long long?
> Like this:
> g_strdup_printf("%lld%c", (long long)pid, '-')
> I find that this is used elsewhere in the code.
Yes, that works.
>
>>> + while (virDirRead(dir, &ent, NULL) > 0) {
>>> + if (STRPREFIX(ent->d_name, pidToStr)) {
>>
>> Any idea what the suffix after '-' in the debugfs entry is? If we can
>> figure it out we won't have to look up the correct file.
>>
>
> The full directory name format is "pid-fd", The fd is assigned during VM creation.
> As I know, we can find it in /proc/(qemu-kvm pid)/fd, but also need to look up which
> one points to "anon_inode:kvm-vm". That may not be any more efficient than look up
> it in the kvm directory.
Right. I tried to find that in the kernel sources, but wasn't
successful. In that case I think you can do what you're doing.
Michal
On 7/16/21 12:42 PM, Yang Fei wrote:
> Add helper function virGetCpuHaltPollTime to obtain halt polling
> time. If the kernel support halt polling time statistic, and mount
> debugfs. This function will take effect on KVM VMs.
>
> Signed-off-by: Yang Fei <yangfei85@huawei.com>
> ---
> src/libvirt_private.syms | 1 +
> src/util/virutil.c | 43 ++++++++++++++++++++++++++++++++++++++++
> src/util/virutil.h | 4 ++++
> 3 files changed, 48 insertions(+)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 68e4b6aab8..64aff4eca4 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -3479,6 +3479,7 @@ virDoesUserExist;
> virDoubleToStr;
> virFormatIntDecimal;
> virFormatIntPretty;
> +virGetCpuHaltPollTime;
> virGetDeviceID;
> virGetDeviceUnprivSGIO;
> virGetGroupID;
> diff --git a/src/util/virutil.c b/src/util/virutil.c
> index 311cbbf93a..f5304644c0 100644
> --- a/src/util/virutil.c
> +++ b/src/util/virutil.c
> @@ -1936,3 +1936,46 @@ virPipeNonBlock(int fds[2])
> {
> return virPipeImpl(fds, true, true);
> }
> +
Apart from Peter's findings:
I'm not sure virutil.c is the best placement for this function. I mean,
virutil.c became dump of functions we did not find a better place for.
How about src/util/virhostcpu.c ? That seems like a better fit, doesn't it?
> +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;
> + g_autoptr(DIR) dir = NULL;
> + int ret = -1;
This variable is pretty much useless. The reason we have it in other
functions is that they have the following pattern:
int ret = -1;
if ()
goto clenaup;
if ()
goto cleanup;
ret = 0;
cleanup:
something();
return ret;
But this is not really the case for this function. You can do 'return
-1' instead of 'return ret' and then plain 'return 0' at the end.
> + bool found = false;
> +
> + if (!(debugFsPath = virFileFindMountPoint("debugfs")))
> + return ret;
> +
> + completePath = g_strdup_printf("%s/%s", debugFsPath, "kvm");
> + if (virDirOpenIfExists(&dir, completePath) != 1)
> + return ret;
> +
> + pidToStr = g_strdup_printf("%d%c", pid, '-');
> + while (virDirRead(dir, &ent, NULL) > 0) {
> + if (STRPREFIX(ent->d_name, pidToStr)) {
> + found = true;
> + break;
> + }
> + }
> +
> + if (!found)
> + return ret;
> +
> + if (virFileReadValueUllong(haltPollSuccess, "%s/%s/%s", completePath,
> + ent->d_name, "halt_poll_success_ns") < 0
> + || virFileReadValueUllong(haltPollFail, "%s/%s/%s", completePath,
> + ent->d_name, "halt_poll_fail_ns") < 0) {
We like to format this as:
if (condition1 ||
condition2)
> + return ret;
> + }
> +
> + ret = 0;
> +
> + return ret;
> +}
Michal
On 2021/7/19 18:20, Michal Prívozník wrote:
> On 7/16/21 12:42 PM, Yang Fei wrote:
>> Add helper function virGetCpuHaltPollTime to obtain halt polling
>> time. If the kernel support halt polling time statistic, and mount
>> debugfs. This function will take effect on KVM VMs.
>>
>> Signed-off-by: Yang Fei <yangfei85@huawei.com>
>> ---
>> src/libvirt_private.syms | 1 +
>> src/util/virutil.c | 43 ++++++++++++++++++++++++++++++++++++++++
>> src/util/virutil.h | 4 ++++
>> 3 files changed, 48 insertions(+)
>>
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index 68e4b6aab8..64aff4eca4 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -3479,6 +3479,7 @@ virDoesUserExist;
>> virDoubleToStr;
>> virFormatIntDecimal;
>> virFormatIntPretty;
>> +virGetCpuHaltPollTime;
>> virGetDeviceID;
>> virGetDeviceUnprivSGIO;
>> virGetGroupID;
>> diff --git a/src/util/virutil.c b/src/util/virutil.c
>> index 311cbbf93a..f5304644c0 100644
>> --- a/src/util/virutil.c
>> +++ b/src/util/virutil.c
>> @@ -1936,3 +1936,46 @@ virPipeNonBlock(int fds[2])
>> {
>> return virPipeImpl(fds, true, true);
>> }
>> +
>
> Apart from Peter's findings:
>
> I'm not sure virutil.c is the best placement for this function. I mean,
> virutil.c became dump of functions we did not find a better place for.
> How about src/util/virhostcpu.c ? That seems like a better fit, doesn't it?
>
Fuction in src/util/virhostcpu.c looks more like getting the physical
properties of the host CPU. Such as the number of CPUs, the frequency, etc.
But this is probably the best placement for this function. I'll move it to
virhostcpu.c 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;
>> + g_autoptr(DIR) dir = NULL;
>> + int ret = -1;
>
> This variable is pretty much useless. The reason we have it in other
> functions is that they have the following pattern:
>
> int ret = -1;
>
> if ()
> goto clenaup;
>
> if ()
> goto cleanup;
>
> ret = 0;
> cleanup:
> something();
> return ret;
>
> But this is not really the case for this function. You can do 'return
> -1' instead of 'return ret' and then plain 'return 0' at the end.
>
OK, I'll remove the ret in next version.
>> + bool found = false;
>> +
>> + if (!(debugFsPath = virFileFindMountPoint("debugfs")))
>> + return ret;
>> +
>> + completePath = g_strdup_printf("%s/%s", debugFsPath, "kvm");
>> + if (virDirOpenIfExists(&dir, completePath) != 1)
>> + return ret;
>> +
>> + pidToStr = g_strdup_printf("%d%c", pid, '-');
>> + while (virDirRead(dir, &ent, NULL) > 0) {
>> + if (STRPREFIX(ent->d_name, pidToStr)) {
>> + found = true;
>> + break;
>> + }
>> + }
>> +
>> + if (!found)
>> + return ret;
>> +
>> + if (virFileReadValueUllong(haltPollSuccess, "%s/%s/%s", completePath,
>> + ent->d_name, "halt_poll_success_ns") < 0
>> + || virFileReadValueUllong(haltPollFail, "%s/%s/%s", completePath,
>> + ent->d_name, "halt_poll_fail_ns") < 0) {
>
> We like to format this as:
>
> if (condition1 ||
> condition2)
>
OK, I'll do it.
>> + return ret;
>> + }
>> +
>> + ret = 0;
>> +
>> + return ret;
>> +}
>
> Michal
>
> .
>
Thanks,
Fei.
© 2016 - 2026 Red Hat, Inc.