On Wed, Jan 18, 2023 at 10:58:17AM +0100, Michal Privoznik wrote:

Firstly, the virProcessGetStatInfo() does not fail really. But even if it did, it sets correct errno only sometimes (and even that is done in a helper it's calling - virProcessGetStat() and even there it's the case only in very few error paths).

Therefore, using virReportSystemError() to report errors is very misleading. Use plain virReportError() instead. Luckily, there are only two places where the former was used: chDomainHelperGetVcpus() and qemuDomainHelperGetVcpus() (not a big surprise since CH driver is heavily inspired by QEMU driver).

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>

Reviewed-by: Martin Kletzander <mkletzan@redhat.com>

--- src/ch/ch_driver.c | 4 ++-- src/qemu/qemu_driver.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c index db2a66d131..12fbe31c24 100644 --- a/src/ch/ch_driver.c +++ b/src/ch/ch_driver.c @@ -1079,8 +1079,8 @@ chDomainHelperGetVcpus(virDomainObj *vm, NULL, NULL, &vcpuinfo->cpu, NULL, vm->pid, vcpupid) < 0) { - virReportSystemError(errno, "%s", - _("cannot get vCPU placement & pCPU time")); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot get vCPU placement & pCPU time")); return -1; } } diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d6879175fe..c576c601ad 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1355,8 +1355,8 @@ qemuDomainHelperGetVcpus(virDomainObj *vm, NULL, NULL, &vcpuinfo->cpu, NULL, vm->pid, vcpupid) < 0) { - virReportSystemError(errno, "%s", - _("cannot get vCPU placement & pCPU time")); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot get vCPU placement & pCPU time")); return -1; } } -- 2.38.2

On Wed, Jan 18, 2023 at 10:58:18AM +0100, Michal Privoznik wrote:

Yeah, we've already seen this commit (v8.0.0-rc2~4) and also its revert (v8.1.0-rc1~345). While the original idea was sound, the implementation was less so and it changed behaviour of some public APIs (e.g. whilst getting stats for a running guest was best effort it started to return errors).

With this patch virsh dominfo will fail for all running qemu and ch domains on non-Linux. Also virDomainGetVcpus in some cases, although that is (maybe) not used that much? The question is do we want it to fail if the strings cannot be parsed or something more sinister than just the system not being supported? Maybe just ignoring the error is fine since that is how it used to work before.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 11 ++++------- src/util/virprocess.c | 28 +++++++--------------------- 2 files changed, 11 insertions(+), 28 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c576c601ad..25c681bfd2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2508,13 +2508,10 @@ qemuDomainGetInfo(virDomainPtr dom, }

if (virDomainObjIsActive(vm)) { - if (virProcessGetStatInfo(&(info->cpuTime), NULL, NULL, - NULL, NULL, - vm->pid, 0) < 0) { - virReportError(VIR_ERR_OPERATION_FAILED, "%s", - _("cannot read cputime for domain")); - goto cleanup; - } + /* Specifically ignore the error here. QEMU's PID might be already gone + * even though the check above says it's still active. */ + virProcessGetStatInfo(&(info->cpuTime), NULL, + NULL, NULL, NULL, vm->pid, 0); }

if (VIR_ASSIGN_IS_OVERFLOW(info->nrVirtCpu, virDomainDefGetVcpus(vm->def))) { diff --git a/src/util/virprocess.c b/src/util/virprocess.c index 39ca5de811..800af29537 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -1761,7 +1761,7 @@ virProcessGetStatInfo(unsigned long long *cpuTime, virStrToLong_ullp(proc_stat[VIR_PROCESS_STAT_STIME], NULL, 10, &stime) < 0 || virStrToLong_l(proc_stat[VIR_PROCESS_STAT_RSS], NULL, 10, &rss) < 0 || virStrToLong_i(proc_stat[VIR_PROCESS_STAT_PROCESSOR], NULL, 10, &cpu) < 0) { - VIR_WARN("cannot parse process status data"); + return -1; }

utime *= jiff2nsec; @@ -1778,7 +1778,6 @@ virProcessGetStatInfo(unsigned long long *cpuTime, if (vm_rss) *vm_rss = rss * virGetSystemPageSizeKB();

- VIR_DEBUG("Got status for %d/%d user=%llu sys=%llu cpu=%d rss=%ld", (int) pid, tid, utime, stime, cpu, rss);

@@ -1852,28 +1851,15 @@ virProcessGetSchedInfo(unsigned long long *cpuWait,

#else int -virProcessGetStatInfo(unsigned long long *cpuTime, - unsigned long long *userTime, - unsigned long long *sysTime, - int *lastCpu, - long *vm_rss, +virProcessGetStatInfo(unsigned long long *cpuTime G_GNUC_UNUSED, + unsigned long long *userTime G_GNUC_UNUSED, + unsigned long long *sysTime G_GNUC_UNUSED, + int *lastCpu G_GNUC_UNUSED, + long *vm_rss G_GNUC_UNUSED, pid_t pid G_GNUC_UNUSED, pid_t tid G_GNUC_UNUSED) { - /* We don't have a way to collect this information on non-Linux - * platforms, so just report neutral values */ - if (cpuTime) - *cpuTime = 0; - if (userTime) - *userTime = 0; - if (sysTime) - *sysTime = 0; - if (lastCpu) - *lastCpu = 0; - if (vm_rss) - *vm_rss = 0; - - return 0; + return -1; }

int -- 2.38.2

On 1/19/23 15:18, Martin Kletzander wrote:

On Wed, Jan 18, 2023 at 10:58:19AM +0100, Michal Privoznik wrote:

...

We have virDomainGetCPUStats() API which offers querying statistics on host CPU usage by given guest. And it works in two modes: getting overall stats (@start_cpu == -1, @ncpus == 1) or getting per host CPU usage.

For the QEMU driver it is implemented by looking into values stored in corresponding cpuacct CGroup controller. Well, this works for system instances, where libvirt has permissions to create CGroups and place QEMU process into them. But it does not fly for session connection, where no CGroups are set up.

Fortunately, we can do something similar to v8.8.0-rc1~95 and use virProcessGetStatInfo() to fill the overall stats. Unfortunately, I haven't found any source of per host CPU usage, so we just continue throwing an error in that case.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 52 ++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 50 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 25c681bfd2..cc10dd87e2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16006,6 +16006,50 @@ qemuDomainGetMetadata(virDomainPtr dom,     return ret; }

+#define QEMU_CPU_STATS_PROC_TOTAL 3 + +static int +qemuDomainGetCPUStatsProc(virDomainObj *vm, +                          virTypedParameterPtr params, +                          unsigned int nparams)

The only thing I do not like about this patch is that we would now have both:

    static int     qemuDomainGetCPUStatsProc(virDomainObj *vm,                               virTypedParameterPtr params,                               unsigned int nparams)

and:

    static int     qemuDomainGetStatsCpuProc(virDomainObj *vm,                               virTypedParamList *params)

doing almost, but not completely, the same thing.  And there is no nice way to avoid it.  I just haven't noticed in v1, sorry.  Or is there a way to converge at least some of these?

Yeah, I realized this too. But, there's a difference between virTypedParameter and virTypedParamList. The former is allocated by user and we fill it out, while the latter is allocated by us. For instance, users interested in just the very first stat, might allocate space for it and call us like: qemuDomainGetCPUStatsProc(vm, ¶ms, 1); (obviously, they would call the public API, but in the end this is how the function would be called). That is not possible with virTypedParamList. Another difference is in the semantics: when the function is called with nparams == 0, then it's supposed to return the number of supported stats. We could invent some glue function that would cover these differences, but I worry that it would end up being longer than this new function I'm inventing (and less readable too). Michal