Received-SPF: pass (google.com: domain of linux-kernel+bounces-193151-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240521173952.3397644-1-weilin.wang@intel.com>
 <20240521173952.3397644-5-weilin.wang@intel.com> <CAM9d7cjob_tfgN+rMRrh=0SV56+z32CmP34BRY1eoFv48RVocg@mail.gmail.com>
 <CO6PR11MB563589ED3FC7126A6B27EB22EEF52@CO6PR11MB5635.namprd11.prod.outlook.com>
 <CAM9d7cjz0P=vrSr8yU=xMYhQ5XFT9A+K-WG9E+LyNzYWC-JhwA@mail.gmail.com>
In-Reply-To: <CAM9d7cjz0P=vrSr8yU=xMYhQ5XFT9A+K-WG9E+LyNzYWC-JhwA@mail.gmail.com>
From: Ian Rogers <irogers@google.com>
Date: Tue, 28 May 2024 15:16:58 -0700
Message-ID: <CAP-5=fV0Xoe9HAcpvSFBF=vGhTaHAJdDCPEmwZSRk3VXL9S+1A@mail.gmail.com>
Subject: Re: [RFC PATCH v9 4/7] perf stat: Plugin retire_lat value from
 sampled data to evsel
To: Namhyung Kim <namhyung@kernel.org>
Cc: "Wang, Weilin" <weilin.wang@intel.com>, Arnaldo Carvalho de Melo <acme@kernel.org>, 
	Peter Zijlstra <peterz@infradead.org>, Ingo Molnar <mingo@redhat.com>, 
	Alexander Shishkin <alexander.shishkin@linux.intel.com>, Jiri Olsa <jolsa@kernel.org>, 
	"Hunter, Adrian" <adrian.hunter@intel.com>, Kan Liang <kan.liang@linux.intel.com>, 
	"linux-perf-users@vger.kernel.org" <linux-perf-users@vger.kernel.org>, 
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "Taylor, Perry" <perry.taylor@intel.com>, 
	"Alt, Samantha" <samantha.alt@intel.com>, "Biggers, Caleb" <caleb.biggers@intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Sun, May 26, 2024 at 11:01=E2=80=AFAM Namhyung Kim <namhyung@kernel.org>=
 wrote:
>
> On Fri, May 24, 2024 at 4:52=E2=80=AFPM Wang, Weilin <weilin.wang@intel.c=
om> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Namhyung Kim <namhyung@kernel.org>
> > > Sent: Friday, May 24, 2024 4:17 PM
> > > To: Wang, Weilin <weilin.wang@intel.com>
> > > Cc: Ian Rogers <irogers@google.com>; Arnaldo Carvalho de Melo
> > > <acme@kernel.org>; Peter Zijlstra <peterz@infradead.org>; Ingo Molnar
> > > <mingo@redhat.com>; Alexander Shishkin
> > > <alexander.shishkin@linux.intel.com>; Jiri Olsa <jolsa@kernel.org>; H=
unter,
> > > Adrian <adrian.hunter@intel.com>; Kan Liang <kan.liang@linux.intel.co=
m>;
> > > linux-perf-users@vger.kernel.org; linux-kernel@vger.kernel.org; Taylo=
r, Perry
> > > <perry.taylor@intel.com>; Alt, Samantha <samantha.alt@intel.com>; Big=
gers,
> > > Caleb <caleb.biggers@intel.com>
> > > Subject: Re: [RFC PATCH v9 4/7] perf stat: Plugin retire_lat value fr=
om sampled
> > > data to evsel
> > >
> > > On Tue, May 21, 2024 at 10:40=E2=80=AFAM <weilin.wang@intel.com> wrot=
e:
> > > >
> > > > From: Weilin Wang <weilin.wang@intel.com>
> > > >
> > > > In current :R parsing implementation, the parser would recognize ev=
ents
> > > with
> > > > retire_latency modifier and insert them into the evlist like a norm=
al event.
> > > > Ideally, we need to avoid counting these events.
> > > >
> > > > In this commit, at the time when a retire_latency evsel is read, se=
t the retire
> > > > latency value processed from the sampled data to count value. This =
sampled
> > > > retire latency value will be used for metric calculation and final =
event count
> > > > print out.
> > >
> > > I'm confused.  Do you mean you don't count the event with 'R' modifie=
r
> > > (w/ perf stat) and just print the (average) retire latency (from perf=
 record)?
> >
> > In metric formulas, event without 'R' modifier is included as a normal =
event already.
> > So we don't need to count the event that with 'R' modifier. They only n=
eed to be
> > sampled.
>
> Oh, you have the event in the metric expression twice.  I thought of one.
> Then IIUC the metric looks something like this.
>
>   myevent1 + (myevent2 * myevent1:R)
>
> I think you'll have 2 myevent1 in perf stat and 1 in perf record, right?
> But the second one in perf stat is never used and the value is updated
> from perf record.
>
> Then we can simply remove the event from the evlist (or replace it with
> a dummy) to reduce the overheads (of open and read).
>
> >
> > >
> > > >
> > > > Signed-off-by: Weilin Wang <weilin.wang@intel.com>
> > > > ---
> > > >  tools/perf/arch/x86/util/evlist.c |  6 +++++
> > > >  tools/perf/util/evsel.c           | 44 +++++++++++++++++++++++++++=
++++
> > > >  tools/perf/util/evsel.h           |  5 ++++
> > > >  3 files changed, 55 insertions(+)
> > > >
> > > > diff --git a/tools/perf/arch/x86/util/evlist.c
> > > b/tools/perf/arch/x86/util/evlist.c
> > > > index b1ce0c52d88d..cebdd483149e 100644
> > > > --- a/tools/perf/arch/x86/util/evlist.c
> > > > +++ b/tools/perf/arch/x86/util/evlist.c
> > > > @@ -89,6 +89,12 @@ int arch_evlist__cmp(const struct evsel *lhs, co=
nst
> > > struct evsel *rhs)
> > > >                         return 1;
> > > >         }
> > > >
> > > > +       /* Retire latency event should not be group leader*/
> > >
> > > Hmm.. why?
> > Because we don't want to count them. Make them the group leader would n=
ot work.
>
> I don't understand.  You'll read the event regardless of being a
> leader or not.
>
> >
> > >
> > > > +       if (lhs->retire_lat && !rhs->retire_lat)
> > > > +               return 1;
> > > > +       if (!lhs->retire_lat && rhs->retire_lat)
> > > > +               return -1;
> > > > +
> > > >         /* Default ordering by insertion index. */
> > > >         return lhs->core.idx - rhs->core.idx;
> > > >  }
> > > > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> > > > index a0a8aee7d6b9..4d700338fc99 100644
> > > > --- a/tools/perf/util/evsel.c
> > > > +++ b/tools/perf/util/evsel.c
> > > > @@ -58,6 +58,7 @@
> > > >  #include <internal/xyarray.h>
> > > >  #include <internal/lib.h>
> > > >  #include <internal/threadmap.h>
> > > > +#include "util/intel-tpebs.h"
> > > >
> > > >  #include <linux/ctype.h>
> > > >
> > > > @@ -1523,6 +1524,40 @@ static int evsel__read_one(struct evsel *evs=
el,
> > > int cpu_map_idx, int thread)
> > > >         return perf_evsel__read(&evsel->core, cpu_map_idx, thread, =
count);
> > > >  }
> > > >
> > > > +static int evsel__set_retire_lat(struct evsel *evsel, int cpu_map_=
idx, int
> > > thread)
> > > > +{
> > > > +       struct perf_counts_values *count;
> > > > +       struct tpebs_retire_lat *t;
> > > > +       bool found =3D false;
> > > > +       __u64 val;
> > > > +
> > > > +       count =3D perf_counts(evsel->counts, cpu_map_idx, thread);
> > > > +
> > > > +       list_for_each_entry(t, &tpebs_results, nd) {
> > > > +               if (!strcmp(t->tpebs_name, evsel->name)) {
> > > > +                       found =3D true;
> > > > +                       break;
> > > > +               }
> > > > +       }
> > > > +
> > > > +       if (!found)
> > > > +               return -1;
> > > > +
> > > > +       /*
> > > > +        * Only set retire_latency value to the first CPU and threa=
d.
> > > > +        */
> > > > +       if (cpu_map_idx =3D=3D 0 && thread =3D=3D 0)
> > > > +               val =3D t->val;
> > > > +       else
> > > > +               val =3D 0;
> > > > +
> > > > +       count->val =3D val;
> > > > +       /* Set ena and run to non-zero */
> > > > +       count->ena =3D count->run =3D 1;
> > > > +       count->lost =3D 0;
> > >
> > > So here it seems you discard the actual count of the events
> > > and replace it with the retire latency.  That means you don't
> > > need to open the event in perf stat, and probably just have a
> > > placeholder, right?
> > >
> > > Btw, I think it's better to move this logic to intel-tpebs.c file and
> > > rename to tpebs_set_retire_lat().
> >
> > Ian wants this to be here and also suggested me to rename this function=
 to
> > evsel__read_retire_lat(). I'm ok with either way.
>
> I think it's better to have the tpebs logic together.

I think the tpebs functions can be in a tpebs file. I'd rather have
the retirement latency events (from the metric) be evsels for a few
reasons:

1) I'd rather everything in a metric be evsels, so things like
"num_cpus_online" should really be a tool event rather than a special
literal kind of thing. I'd like to reduce special cases over time, in
part as it should help with portability. For example, if only Intel
x86 can parse :R then someone trying to parse an Intel x86 metric on
ARM may get parser errors.

2) When we change from forking perf record to directly opening a
sampling ring buffer then it makes sense that we use/update the
evsel/evlist logic.

Thanks,
Ian

> Thanks,
> Namhyung
>
> >
> > >
> > >
> > > > +       return 0;
> > > > +}
> > > > +
> > > >  static void evsel__set_count(struct evsel *counter, int cpu_map_id=
x, int
> > > thread,
> > > >                              u64 val, u64 ena, u64 run, u64 lost)
> > > >  {
> > > > @@ -1530,6 +1565,12 @@ static void evsel__set_count(struct evsel
> > > *counter, int cpu_map_idx, int thread,
> > > >
> > > >         count =3D perf_counts(counter->counts, cpu_map_idx, thread)=
;
> > > >
> > > > +       if (counter->retire_lat) {
> > >
> > > if (evsel__is_retire_lat(counter)) ?
> > >
> > >
> > > > +               evsel__set_retire_lat(counter, cpu_map_idx, thread)=
;
> > > > +               perf_counts__set_loaded(counter->counts, cpu_map_id=
x, thread,
> > > true);
> > > > +               return;
> > > > +       }
> > > > +
> > > >         count->val    =3D val;
> > > >         count->ena    =3D ena;
> > > >         count->run    =3D run;
> > > > @@ -1778,6 +1819,9 @@ int evsel__read_counter(struct evsel *evsel, =
int
> > > cpu_map_idx, int thread)
> > > >         if (evsel__is_tool(evsel))
> > > >                 return evsel__read_tool(evsel, cpu_map_idx, thread)=
;
> > > >
> > > > +       if (evsel__is_retire_lat(evsel))
> > > > +               return evsel__set_retire_lat(evsel, cpu_map_idx, th=
read);
> > > > +
> > >
> > > I'm not sure if it works well with group event.  Probably that's
> > > why you wanted to prevent group leaders.  But I guess you
> > > can just check this after the PERF_FORMAT_GROUP, no?
> > >
> > > Thanks,
> > > Namhyung
> > >
> > >
> > > >         if (evsel->core.attr.read_format & PERF_FORMAT_GROUP)
> > > >                 return evsel__read_group(evsel, cpu_map_idx, thread=
);
> > > >
> > > > diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
> > > > index bd8e84954e34..aaf572317e92 100644
> > > > --- a/tools/perf/util/evsel.h
> > > > +++ b/tools/perf/util/evsel.h
> > > > @@ -303,6 +303,11 @@ static inline bool evsel__is_tool(const struct=
 evsel
> > > *evsel)
> > > >         return evsel->tool_event !=3D PERF_TOOL_NONE;
> > > >  }
> > > >
> > > > +static inline bool evsel__is_retire_lat(const struct evsel *evsel)
> > > > +{
> > > > +       return evsel->retire_lat;
> > > > +}
> > > > +
> > > >  const char *evsel__group_name(struct evsel *evsel);
> > > >  int evsel__group_desc(struct evsel *evsel, char *buf, size_t size)=
;
> > > >
> > > > --
> > > > 2.43.0
> > > >