Re: [PATCH v3 2/3] perf stat: add rusage utime and stime events

[Namhyung Kim]

Hello,

On Tue, Apr 12, 2022 at 2:36 AM Ian Rogers <irogers@xxxxxxxxxx> wrote:
>
> On Sun, Apr 10, 2022 at 9:41 AM Florian Fischer
> <florian.fischer@muhq.space> wrote:
> >
> > > > This patch adds two new tool internal events 'rusage_user_time'
> > > > and 'rusage_system_time' as well as their aliases 'ru_utime' and
> > > > 'ru_stime', similarly to the already present 'duration_time' event.
> > > >
> > > > Both events use the already collected rusage information obtained by wait4
> > > > and tracked in the global ru_stats.
> > > >
> > > > Examples presenting cache-misses and rusage information in both human and
> > > > machine-readable form:
> > > >
> > > > $ ./perf stat -e duration_time,ru_utime,ru_stime,cache-misses -- grep -q -r duration_time .
> > > >
> > > >  Performance counter stats for 'grep -q -r duration_time .':
> > > >
> > > >         67,422,542 ns   duration_time:u
> > > >         50,517,000 ns   ru_utime:u
> > > >         16,839,000 ns   ru_stime:u
> > > >             30,937      cache-misses:u
> > > >
> > > >        0.067422542 seconds time elapsed
> > > >
> > > >        0.050517000 seconds user
> > > >        0.016839000 seconds sys
> > > >
> > > > $ ./perf stat -x, -e duration_time,ru_utime,ru_stime,cache-misses -- grep -q -r duration_time .
> > > > 72134524,ns,duration_time:u,72134524,100.00,,
> > > > 65225000,ns,ru_utime:u,65225000,100.00,,
> > > > 6865000,ns,ru_stime:u,6865000,100.00,,
> > > > 38705,,cache-misses:u,71189328,100.00,,
> > >
> > > This is really nice. For metric code we currently handle duration_time
> > > in a special way, for example:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/util/metricgroup.c?h=perf/core#n745
> > > https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/util/metricgroup.c?h=perf/core#n1131
> > > We will need to do something similar with these tool events, but I'm
> > > happy that it can be follow-up work.
> > >
> > > I'm not a huge fan of the names ru_utime and ru_stime, two thoughts
> > > here we could do duration_time:u and duration_time:k but I don't think
> > > that really makes sense. My preference would be to just call ru_utime
> > > user_time and ru_stime system_time.
> >
> > I considered ru_{u,s}_time only as aliases because those are the field names in
> > the rusage struct filled by wait4 and are probably known by perf users.
> > The "official" names are currently rusage_{user,system}_time.
> > I could change them to only {user,system}_time because those names are more in line
> > with the already present duration_time and are independent of the rusage
> > implementation detail.
> >
> > What do you think of?
>
> I like user_time and system_time, short and to the point [1] while
> satisfying being intention-revealing. The aliases, ru_utime and
> ru_stime, are fine but a bit of an acronym soup. They mean we need to
> special case more names in the metric code. I don't know how others
> feel but I'd stick to just user_time and system_time.

Maybe I missed something but could we just use cpu/task-clock?
I'm not sure the rusage-base events work for cpu events and for
existing tasks.

Thanks,
Namhyung