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

On Fri, Jul 16, 2021 at 18:42:23 +0800, Yang Fei wrote:

This function add halt polling time interface in domstats. So that we can use command 'virsh domstats VM' to get the data if system support.

Signed-off-by: Yang Fei <yangfei85@huawei.com> --- src/libvirt-domain.c | 7 +++++++ src/qemu/qemu_driver.c | 20 ++++++++++++++++++++ 2 files changed, 27 insertions(+)

diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 750e32f0ca..8e58c1b43f 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -11625,6 +11625,13 @@ virConnectGetDomainCapabilities(virConnectPtr conn, * "cpu.user" - user cpu time spent in nanoseconds as unsigned long long. * "cpu.system" - system cpu time spent in nanoseconds as unsigned long * long. + * "haltpollsuccess.time" - halt-polling cpu usage about the VCPU polled + * until a virtual interrupt was delivered in + * nanoseconds as unsigned long long. + * "haltpollfail.time" - halt-polling cpu usage about the VCPU had to schedule + * out (either because the maximum poll time was reached + * or it needed to yield the CPU) in nanoseconds as + * unsigned long long.

These don't conform with the other fields as they don't have the 'cpu.' prefix. Without the prefix it makes them a group of their own which would have other implications and that's probably not desired. Additionally the format is weird. I'd suggest: cpu.haltpoll.success.time cpu.haltpoll.fail.time or something similar, but it must be hierarchical and must make sense. Additionally the same kind of docs is in virsh's man page, so don't forget to add it there too.

* "cpu.cache.monitor.count" - the number of cache monitors for this domain * "cpu.cache.monitor.<num>.name" - the name of cache monitor <num> * "cpu.cache.monitor.<num>.vcpus" - vcpu list of cache monitor <num> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 235f575901..adb4628558 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17839,6 +17839,23 @@ qemuDomainGetStatsCpuCgroup(virDomainObj *dom, return 0; }

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

This still can reportserrors which would be logged and then igored and thus would pollute the logs. This is not acceptable in the stats API as it's being called fairly often. You must ensure that in any failure case, this doesn't log anything and doesn't make the stats API return failure. Just silently skip the output.

+ return 0; + + if (virTypedParamListAddULLong(params, haltPollSuccess, "haltpollsuccess.time") < 0 || + virTypedParamListAddULLong(params, haltPollFail, "haltpollfail.time") < 0) + return -1; + + return 0; +}