Re: [PATCH v3 1/2] util: Add virGetCpuHaltPollTime
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(a)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.