Date: Fri, 26 Apr 2024 10:34:39 -0700
From: Ian Rogers <irogers@...gle.com>
To: "Liang, Kan" <kan.liang@...ux.intel.com>
Cc: weilin.wang@...el.com, Peter Zijlstra <peterz@...radead.org>, 
	Ingo Molnar <mingo@...hat.com>, Arnaldo Carvalho de Melo <acme@...nel.org>, Namhyung Kim <namhyung@...nel.org>, 
	Mark Rutland <mark.rutland@....com>, 
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>, Jiri Olsa <jolsa@...nel.org>, 
	Adrian Hunter <adrian.hunter@...el.com>, Ze Gao <zegao2021@...il.com>, 
	Leo Yan <leo.yan@...ux.dev>, Ravi Bangoria <ravi.bangoria@....com>, 
	Dmitrii Dolgov <9erthalion6@...il.com>, Song Liu <song@...nel.org>, 
	James Clark <james.clark@....com>, linux-perf-users@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH v1 0/3] Retirement latency perf stat support

On Fri, Apr 26, 2024 at 10:22 AM Liang, Kan <kan.liang@...ux.intel.com> wrote:
>
> On 2024-04-25 6:34 p.m., Ian Rogers wrote:
> > Support 'R' as a retirement latency modifier on events. When present
> > the evsel will fork perf record and perf report commands, parsing the
> > perf report output as the count value. The intent is to do something
> > similar to Weilin's series:
> > https://lore.kernel.org/lkml/20240402214436.1409476-1-weilin.wang@intelcom/
> >
> > While the 'R' and the retirement latency are Intel specific, in the
> > future I can imagine more evsel like commands that require child
> > processes. We can make the logic more generic at that point.
> >
>
> I think in generic what we want is the weight/latency information of the
> event. 'W' is already occupied by the weak group. Maybe 'L' is a more
> generic name than 'R'. With the event modifier, perf collects and report
> the weight/latency information of the event in a perf stat command.
>
> Not just changing the evsel, I think a proper output is still required.
> It's possible that an end user can use it without metrics. E.g.,
> perf stat -e cycles,instructions:L
> A possible generic output maybe
>
> 1,931,099,931   cycles
>   801,826,458   instructions    # Avg Weight1 1000
>                                 # Avg Weight2 800
>                                 # Avg Weight3 500

I think this is good but we need to work toward it. This change is
opening a separate perf record per CPU, we should really open one perf
record and then read each counter separately in the perf report
output. We shouldn't really fork a perf record, we should gather
multiple weights, and so on.. There isn't a notion in the current
counts abstraction that you have multiple counts, and that will need
feeding through into all the aggregation code.

Thanks,
Ian

> Thanks,
> Kan
>
> > The code is untested on hardware that supports retirement latency, and
> > with metrics with retirement latency in them. The record is also of
> > sleep and various things need tweaking but I think v1 is good enough
> > for people to give input.
> >
> > The first patch stops opening a dummy event for tool events. I came
> > across this while looking into the issue and we can likely just pick
> > it first. I kept it in the series for cleanliness sake.
> >
> > The code has benefitted greatly from Weilin's work and Namhyung's
> > great review input.
> >
> > Ian Rogers (3):
> >   perf evsel: Don't open tool events
> >   perf parse-events: Add a retirement latency modifier
> >   perf evsel: Add retirement latency event support
> >
> >  tools/perf/util/evsel.c        | 186 ++++++++++++++++++++++++++++++++-
> >  tools/perf/util/evsel.h        |   4 +
> >  tools/perf/util/parse-events.c |   2 +
> >  tools/perf/util/parse-events.h |   1 +
> >  tools/perf/util/parse-events.l |   3 +-
> >  5 files changed, 192 insertions(+), 4 deletions(-)
> >

Powered by blists - more mailing lists