Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <69656438-8b9a-000b-0702-02dc480639f9@linux.intel.com>
 <20220407085721.3289414-1-florian.fischer@muhq.space> <20220407085721.3289414-3-florian.fischer@muhq.space>
 <CAP-5=fWC5e9PTs9PVttVDdNbCzYQVeqyuf95q181Vkg4NqJxqg@mail.gmail.com>
 <20220410164136.bxqtpbrmfbqxdx4n@pasture> <CAP-5=fXuR2tK_4FDnRA88a9wQkndbqyeN6G2muppaMhV5mAQ=A@mail.gmail.com>
In-Reply-To: <CAP-5=fXuR2tK_4FDnRA88a9wQkndbqyeN6G2muppaMhV5mAQ=A@mail.gmail.com>
From:   Namhyung Kim <namhyung@gmail.com>
Date:   Tue, 12 Apr 2022 15:31:07 -0700
Message-ID: <CAM9d7ci2vKMXopBwnWxebzkqsz+ySvhqqFb6_rkk=XE1fd1RnQ@mail.gmail.com>
Subject: Re: [PATCH v3 2/3] perf stat: add rusage utime and stime events
To:     Ian Rogers <irogers@google.com>
Cc:     Florian Fischer <florian.fischer@muhq.space>,
        Xing Zhengjun <zhengjun.xing@linux.intel.com>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        linux-perf-users <linux-perf-users@vger.kernel.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        Florian Schmaus <flow@cs.fau.de>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hello,

On Tue, Apr 12, 2022 at 2:36 AM Ian Rogers <irogers@google.com> 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