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