Re: [PATCH 1/2] util: Extend virProcessGetStatInfo() for sysTime and userTime

[Ján Tomko <jtomko@xxxxxxxxxx>]

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@xxxxxxxxxx>
> ---
> 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;