Re: [RFC PATCH v11 3/8] perf stat: Fork and launch perf record when perf stat needs to get retire latency value for a metric.

[Namhyung Kim]

On Sun, Jun 09, 2024 at 03:02:21AM +0000, Wang, Weilin wrote:
> 
> 
> > -----Original Message-----
> > From: Namhyung Kim <namhyung@xxxxxxxxxx>
> > Sent: Saturday, June 8, 2024 7:28 PM
> > To: Wang, Weilin <weilin.wang@xxxxxxxxx>
> > Cc: Ian Rogers <irogers@xxxxxxxxxx>; Arnaldo Carvalho de Melo
> > <acme@xxxxxxxxxx>; Peter Zijlstra <peterz@xxxxxxxxxxxxx>; Ingo Molnar
> > <mingo@xxxxxxxxxx>; Alexander Shishkin
> > <alexander.shishkin@xxxxxxxxxxxxxxx>; Jiri Olsa <jolsa@xxxxxxxxxx>; Hunter,
> > Adrian <adrian.hunter@xxxxxxxxx>; Kan Liang <kan.liang@xxxxxxxxxxxxxxx>;
> > linux-perf-users@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Taylor, Perry
> > <perry.taylor@xxxxxxxxx>; Alt, Samantha <samantha.alt@xxxxxxxxx>; Biggers,
> > Caleb <caleb.biggers@xxxxxxxxx>
> > Subject: Re: [RFC PATCH v11 3/8] perf stat: Fork and launch perf record when
> > perf stat needs to get retire latency value for a metric.
> > 
> > On Fri, Jun 07, 2024 at 08:45:13PM +0000, Wang, Weilin wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Namhyung Kim <namhyung@xxxxxxxxxx>
> > > > Sent: Friday, June 7, 2024 12:20 PM
> > > > To: Wang, Weilin <weilin.wang@xxxxxxxxx>
> > > > Cc: Ian Rogers <irogers@xxxxxxxxxx>; Arnaldo Carvalho de Melo
> > > > <acme@xxxxxxxxxx>; Peter Zijlstra <peterz@xxxxxxxxxxxxx>; Ingo Molnar
> > > > <mingo@xxxxxxxxxx>; Alexander Shishkin
> > > > <alexander.shishkin@xxxxxxxxxxxxxxx>; Jiri Olsa <jolsa@xxxxxxxxxx>; Hunter,
> > > > Adrian <adrian.hunter@xxxxxxxxx>; Kan Liang <kan.liang@xxxxxxxxxxxxxxx>;
> > > > linux-perf-users@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Taylor,
> > Perry
> > > > <perry.taylor@xxxxxxxxx>; Alt, Samantha <samantha.alt@xxxxxxxxx>;
> > Biggers,
> > > > Caleb <caleb.biggers@xxxxxxxxx>
> > > > Subject: Re: [RFC PATCH v11 3/8] perf stat: Fork and launch perf record
> > when
> > > > perf stat needs to get retire latency value for a metric.
> > > >
> > > > On Fri, Jun 07, 2024 at 01:07:12AM +0000, Wang, Weilin wrote:
> > [SNIP]
> > > > > > > @@ -2186,6 +2240,9 @@ static int evsel__open_cpu(struct evsel
> > *evsel,
> > > > > > struct perf_cpu_map *cpus,
> > > > > > >  		return 0;
> > > > > > >  	}
> > > > > > >
> > > > > > > +	if (evsel__is_retire_lat(evsel))
> > > > > > > +		return tpebs_start(evsel->evlist, cpus);
> > > > > >
> > > > > > As it works with evlist, I think it's better to put this code there.
> > > > > > But it seems perf stat doesn't call the evlist API for open, then we
> > > > > > can add this to somewhere in __run_perf_stat() directly.
> > > > > >
> > > > > > > +
> > > > > > >  	err = __evsel__prepare_open(evsel, cpus, threads);
> > > > > > >  	if (err)
> > > > > > >  		return err;
> > > > > > > @@ -2376,6 +2433,8 @@ int evsel__open(struct evsel *evsel, struct
> > > > > > perf_cpu_map *cpus,
> > > > > > >
> > > > > > >  void evsel__close(struct evsel *evsel)
> > > > > > >  {
> > > > > > > +	if (evsel__is_retire_lat(evsel))
> > > > > > > +		tpebs_delete();
> > > > > >
> > > > > > Ditto.
> > > > >
> > > > > Hi Namhyung,
> > > > >
> > > > > I hope both this and the one above on open could stay in evsel level
> > because
> > > > > these are operations on retire_latency evsel.
> > > >
> > > > Then I think you need to remove the specific evsel not the all tpebs
> > > > events.
> > > >
> > > > > At the same time, a lot of the
> > > > > previous several versions of work was to move TPEBS code out from perf
> > > > stat to
> > > > > evsel to make it more generic. I think move these back to
> > __run_perf_stat()
> > > > are
> > > > > opposite to that goal.
> > > >
> > > > Oh, I meant you can have the logic in utils/intel-tpebs.c but add a call
> > > > to tpebs_delete() in __run_perf_stat().  I think it'd better to keep it
> > > > in evlist__close() but we don't use evlist__open() for perf stat so it's
> > > > not symmetric. :(
> > > >
> > > > Anyway, all I want to say is that tpebs APIs work on evlist level.  So I
> > > > think it's natural that they are called for the whole list, not for an
> > > > event/evsel.
> > >
> > > I think we're trying to work at evsel level and open(remove) or close one
> > > retire_latency evsel at a time. In addition to that, we put all the required
> > retire_latency
> > > together in one perf record launch in order to reduce overhead to fork
> > multiple perf
> > > record. I hope this makes sense.
> > 
> > Well.. I think we can do something like this in the current code.
> > 
> > __run_perf_stat():
> >   ...
> > 
> >   tpebs__start(evlist, target);
> > 
> >   evlist__for_each_cpu(...) {
> >       if (create_perf_steat_counter() < 0) {
> >           ....
> > 
> > instead of doing it in the evsel__open().  What's the issue with this
> > approach?
> 
> This is basically how tpebs__start() was invoked in v9 (https://lore.kernel.org/all/CAM9d7ci7tgjR8LVNx+ZrFKMGo+OZn=eFSksPL56MeP_Q84PkMw@xxxxxxxxxxxxxx/)
> 
> I changed it in v10 so that it works at evsel level. 
> 
> Ian, could you please let me know what do you think about this? 

Ok, we sync-ed offline and agreed to have it in evsel level.  I still
think it's better to handle it in evlist level (at least for TPEBS) but
unfortunately we don't use evlist__open() consistently and there are
places it's not called.  Probably we need to convert the all call sites
to open evsel to be from evlist__open() then move tpebs__start() there.

Thanks,
Namhyung