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 > <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(a)huawei.com&gt; > --- > 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; > +} .