[PATCH 0/2] Implement qemuDomainGetStatsCpu fallback for qemu:///session
*** BLURB HERE *** Michal Prívozník (2): util: Extend virProcessGetStatInfo() for sysTime and userTime qemu: Implement qemuDomainGetStatsCpu fallback for qemu:///session src/ch/ch_driver.c | 1 + src/qemu/qemu_driver.c | 39 ++++++++++++++++++++++++++++++++++++--- src/util/virprocess.c | 30 +++++++++++++++++++++++------- src/util/virprocess.h | 2 ++ 4 files changed, 62 insertions(+), 10 deletions(-) -- 2.35.1
The virProcessGetStatInfo() helper parses /proc stat file for given PID and/or TID and reports cumulative cpuTime which is just a sum of user and sys times. But in near future, we'll need those times separately, so make the function return them too (if caller desires). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/ch/ch_driver.c | 1 + src/qemu/qemu_driver.c | 4 +++- src/util/virprocess.c | 30 +++++++++++++++++++++++------- src/util/virprocess.h | 2 ++ 4 files changed, 29 insertions(+), 8 deletions(-) diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c index e7c172c894..bde148075d 100644 --- a/src/ch/ch_driver.c +++ b/src/ch/ch_driver.c @@ -1075,6 +1075,7 @@ chDomainHelperGetVcpus(virDomainObj *vm, vcpuinfo->number = i; vcpuinfo->state = VIR_VCPU_RUNNING; if (virProcessGetStatInfo(&vcpuinfo->cpuTime, + NULL, NULL, &vcpuinfo->cpu, NULL, vm->pid, vcpupid) < 0) { virReportSystemError(errno, "%s", diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 707f4cc1bb..d7283a6e47 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1360,6 +1360,7 @@ qemuDomainHelperGetVcpus(virDomainObj *vm, vcpuinfo->state = VIR_VCPU_RUNNING; if (virProcessGetStatInfo(&vcpuinfo->cpuTime, + NULL, NULL, &vcpuinfo->cpu, NULL, vm->pid, vcpupid) < 0) { virReportSystemError(errno, "%s", @@ -2516,6 +2517,7 @@ 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")); @@ -10676,7 +10678,7 @@ qemuDomainMemoryStatsInternal(virDomainObj *vm, ret = 0; } - if (virProcessGetStatInfo(NULL, NULL, &rss, vm->pid, 0) < 0) { + if (virProcessGetStatInfo(NULL, NULL, NULL, NULL, &rss, vm->pid, 0) < 0) { virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("cannot get RSS for domain")); } else { diff --git a/src/util/virprocess.c b/src/util/virprocess.c index 013afd91b4..4cc75b8b45 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -1737,19 +1737,24 @@ virProcessGetStat(pid_t pid, #ifdef __linux__ int virProcessGetStatInfo(unsigned long long *cpuTime, + unsigned long long *sysTime, + unsigned long long *userTime, int *lastCpu, long *vm_rss, pid_t pid, pid_t tid) { g_auto(GStrv) proc_stat = virProcessGetStat(pid, tid); - unsigned long long usertime = 0, systime = 0; + unsigned long long utime = 0; + unsigned long long stime = 0; + const unsigned long long jiff2sec = 1000ull * 1000ull * 1000ull / + (unsigned long long) sysconf(_SC_CLK_TCK); long rss = 0; int cpu = 0; if (!proc_stat || - virStrToLong_ullp(proc_stat[VIR_PROCESS_STAT_UTIME], NULL, 10, &usertime) < 0 || - virStrToLong_ullp(proc_stat[VIR_PROCESS_STAT_STIME], NULL, 10, &systime) < 0 || + virStrToLong_ullp(proc_stat[VIR_PROCESS_STAT_UTIME], NULL, 10, &utime) < 0 || + 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"); @@ -1758,11 +1763,16 @@ virProcessGetStatInfo(unsigned long long *cpuTime, /* We got jiffies * We want nanoseconds * _SC_CLK_TCK is jiffies per second - * So calculate thus.... + * So calculate this.... */ + utime *= jiff2sec; + stime *= jiff2sec; if (cpuTime) - *cpuTime = 1000ull * 1000ull * 1000ull * (usertime + systime) - / (unsigned long long) sysconf(_SC_CLK_TCK); + *cpuTime = utime + stime; + if (sysTime) + *sysTime = stime; + if (userTime) + *userTime = utime; if (lastCpu) *lastCpu = cpu; @@ -1771,7 +1781,7 @@ virProcessGetStatInfo(unsigned long long *cpuTime, VIR_DEBUG("Got status for %d/%d user=%llu sys=%llu cpu=%d rss=%ld", - (int) pid, tid, usertime, systime, cpu, rss); + (int) pid, tid, utime, stime, cpu, rss); return 0; } @@ -1844,6 +1854,8 @@ virProcessGetSchedInfo(unsigned long long *cpuWait, #else int virProcessGetStatInfo(unsigned long long *cpuTime, + unsigned long long *sysTime, + unsigned long long *userTime, int *lastCpu, long *vm_rss, pid_t pid G_GNUC_UNUSED, @@ -1853,6 +1865,10 @@ virProcessGetStatInfo(unsigned long long *cpuTime, * platforms, so just report neutral values */ if (cpuTime) *cpuTime = 0; + if (sysTime) + *sysTime = 0; + if (userTime) + *userTime = 0; if (lastCpu) *lastCpu = 0; if (vm_rss) diff --git a/src/util/virprocess.h b/src/util/virprocess.h index 30b6981c73..b70a405d9f 100644 --- a/src/util/virprocess.h +++ b/src/util/virprocess.h @@ -195,6 +195,8 @@ typedef enum { int virProcessNamespaceAvailable(unsigned int ns); int virProcessGetStatInfo(unsigned long long *cpuTime, + unsigned long long *sysTime, + unsigned long long *userTime, int *lastCpu, long *vm_rss, pid_t pid, -- 2.35.1
On a Monday in 2022, Michal Privoznik wrote:
The virProcessGetStatInfo() helper parses /proc stat file for given PID and/or TID and reports cumulative cpuTime which is just a sum of user and sys times. But in near future, we'll need those times separately, so make the function return them too (if caller desires).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/ch/ch_driver.c | 1 + src/qemu/qemu_driver.c | 4 +++- src/util/virprocess.c | 30 +++++++++++++++++++++++------- src/util/virprocess.h | 2 ++ 4 files changed, 29 insertions(+), 8 deletions(-)
diff --git a/src/util/virprocess.c b/src/util/virprocess.c index 013afd91b4..4cc75b8b45 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -1737,19 +1737,24 @@ virProcessGetStat(pid_t pid, #ifdef __linux__ int virProcessGetStatInfo(unsigned long long *cpuTime, + unsigned long long *sysTime, + unsigned long long *userTime,
I would expect these to be swapped. time(1) reports user time before sys time, and they are in that order also in /proc/$PID/stat
int *lastCpu, long *vm_rss, pid_t pid, pid_t tid) { g_auto(GStrv) proc_stat = virProcessGetStat(pid, tid); - unsigned long long usertime = 0, systime = 0; + unsigned long long utime = 0; + unsigned long long stime = 0; + const unsigned long long jiff2sec = 1000ull * 1000ull * 1000ull / + (unsigned long long) sysconf(_SC_CLK_TCK); long rss = 0; int cpu = 0;
if (!proc_stat || - virStrToLong_ullp(proc_stat[VIR_PROCESS_STAT_UTIME], NULL, 10, &usertime) < 0 || - virStrToLong_ullp(proc_stat[VIR_PROCESS_STAT_STIME], NULL, 10, &systime) < 0 || + virStrToLong_ullp(proc_stat[VIR_PROCESS_STAT_UTIME], NULL, 10, &utime) < 0 || + 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"); @@ -1758,11 +1763,16 @@ virProcessGetStatInfo(unsigned long long *cpuTime, /* We got jiffies * We want nanoseconds * _SC_CLK_TCK is jiffies per second - * So calculate thus.... + * So calculate this.... */ + utime *= jiff2sec; + stime *= jiff2sec;
By using the descriptive constnant name, we no longer need the multi-line comment. But it converts to ns, so 'jiff2ns' or 'jiff2nsec' would be more appropriate. Jano
if (cpuTime) - *cpuTime = 1000ull * 1000ull * 1000ull * (usertime + systime) - / (unsigned long long) sysconf(_SC_CLK_TCK); + *cpuTime = utime + stime; + if (sysTime) + *sysTime = stime; + if (userTime) + *userTime = utime; if (lastCpu) *lastCpu = cpu;
For domains started under session URI, we don't set up CGroups (well, how could we since we're not running as root anyways). Nevertheless, fetching CPU statistics exits early because of lacking cpuacct controller. But with recent extension to virProcessGetStatInfo() we can get the values we need from the proc filesystem. Implement the fallback for the session URI as some of virt tools rely on cpu.* stats to be reported (virt-top, virt-manager). Resolves: https://gitlab.com/libvirt/libvirt/-/issues/353 Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1693707 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 35 +++++++++++++++++++++++++++++++++-- 1 file changed, 33 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d7283a6e47..c7cca64001 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17765,6 +17765,30 @@ qemuDomainGetStatsCpuCgroup(virDomainObj *dom, return 0; } + +static int +qemuDomainGetStatsCpuProc(virDomainObj *vm, + virTypedParamList *params) +{ + unsigned long long cpuTime = 0; + unsigned long long sysTime = 0; + unsigned long long userTime = 0; + + if (virProcessGetStatInfo(&cpuTime, &sysTime, &userTime, + NULL, NULL, vm->pid, 0) < 0) { + /* ignore error */ + return 0; + } + + if (virTypedParamListAddULLong(params, cpuTime, "cpu.time") < 0 || + virTypedParamListAddULLong(params, userTime, "cpu.user") < 0 || + virTypedParamListAddULLong(params, sysTime, "cpu.system") < 0) + return -1; + + return 0; +} + + static int qemuDomainGetStatsCpuHaltPollTimeFromStats(virDomainObj *dom, unsigned int privflags, @@ -17860,8 +17884,15 @@ qemuDomainGetStatsCpu(virQEMUDriver *driver, virTypedParamList *params, unsigned int privflags) { - if (qemuDomainGetStatsCpuCgroup(dom, params) < 0) - return -1; + qemuDomainObjPrivate *priv = dom->privateData; + + if (priv->cgroup) { + if (qemuDomainGetStatsCpuCgroup(dom, params) < 0) + return -1; + } else { + if (qemuDomainGetStatsCpuProc(dom, params) < 0) + return -1; + } if (qemuDomainGetStatsCpuCache(driver, dom, params) < 0) return -1; -- 2.35.1
On Mon, Sep 05, 2022 at 09:40:57AM +0200, Michal Privoznik wrote:
> For domains started under session URI, we don't set up CGroups
> (well, how could we since we're not running as root anyways).
> Nevertheless, fetching CPU statistics exits early because of
> lacking cpuacct controller. But with recent extension to
> virProcessGetStatInfo() we can get the values we need from the
> proc filesystem. Implement the fallback for the session URI as
> some of virt tools rely on cpu.* stats to be reported (virt-top,
> virt-manager).
>
> Resolves: https://gitlab.com/libvirt/libvirt/-/issues/353
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1693707
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
> src/qemu/qemu_driver.c | 35 +++++++++++++++++++++++++++++++++--
> 1 file changed, 33 insertions(+), 2 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index d7283a6e47..c7cca64001 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -17765,6 +17765,30 @@ qemuDomainGetStatsCpuCgroup(virDomainObj *dom,
> return 0;
> }
>
> +
> +static int
> +qemuDomainGetStatsCpuProc(virDomainObj *vm,
> + virTypedParamList *params)
> +{
> + unsigned long long cpuTime = 0;
> + unsigned long long sysTime = 0;
> + unsigned long long userTime = 0;
> +
> + if (virProcessGetStatInfo(&cpuTime, &sysTime, &userTime,
> + NULL, NULL, vm->pid, 0) < 0) {
> + /* ignore error */
> + return 0;
> + }
This has upset the static analysis in coverity:
*** CID 398935: API usage errors (SWAPPED_ARGUMENTS)
/src/qemu/qemu_driver.c: 17777 in qemuDomainGetStatsCpuProc()
17771 virTypedParamList *params)
17772 {
17773 unsigned long long cpuTime = 0;
17774 unsigned long long sysTime = 0;
17775 unsigned long long userTime = 0;
17776
>>> CID 398935: API usage errors (SWAPPED_ARGUMENTS)
>>> The positions of arguments in the call to "virProcessGetStatInfo" do not match the ordering of the parameters:
* "&sysTime" is passed to "userTime".
* "&userTime" is passed to "sysTime".
17777 if (virProcessGetStatInfo(&cpuTime, &sysTime, &userTime,
17778 NULL, NULL, vm->pid, 0) < 0) {
17779 /* ignore error */
17780 return 0;
17781 }
17782
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On a Monday in 2022, Michal Privoznik wrote:
*** BLURB HERE ***
Michal Prívozník (2): util: Extend virProcessGetStatInfo() for sysTime and userTime qemu: Implement qemuDomainGetStatsCpu fallback for qemu:///session
src/ch/ch_driver.c | 1 + src/qemu/qemu_driver.c | 39 ++++++++++++++++++++++++++++++++++++--- src/util/virprocess.c | 30 +++++++++++++++++++++++------- src/util/virprocess.h | 2 ++ 4 files changed, 62 insertions(+), 10 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (3)
-
Daniel P. Berrangé -
Ján Tomko -
Michal Privoznik