| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CO6PR11MB563589ED3FC7126A6B27EB22EEF52@CO6PR11MB5635.namprd11.prod.outlook.com>
Date: Fri, 24 May 2024 23:51:58 +0000
From: "Wang, Weilin" <weilin.wang@...el.com>
To: Namhyung Kim <namhyung@...nel.org>
CC: Ian Rogers <irogers@...gle.com>, Arnaldo Carvalho de Melo
<acme@...nel.org>, Peter Zijlstra <peterz@...radead.org>, Ingo Molnar
<mingo@...hat.com>, Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
Jiri Olsa <jolsa@...nel.org>, "Hunter, Adrian" <adrian.hunter@...el.com>, Kan
Liang <kan.liang@...ux.intel.com>, "linux-perf-users@...r.kernel.org"
<linux-perf-users@...r.kernel.org>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>, "Taylor, Perry" <perry.taylor@...el.com>,
"Alt, Samantha" <samantha.alt@...el.com>, "Biggers, Caleb"
<caleb.biggers@...el.com>
Subject: RE: [RFC PATCH v9 4/7] perf stat: Plugin retire_lat value from
sampled data to evsel
> -----Original Message-----
> From: Namhyung Kim <namhyung@...nel.org>
> Sent: Friday, May 24, 2024 4:17 PM
> To: Wang, Weilin <weilin.wang@...el.com>
> Cc: Ian Rogers <irogers@...gle.com>; Arnaldo Carvalho de Melo
> <acme@...nel.org>; Peter Zijlstra <peterz@...radead.org>; Ingo Molnar
> <mingo@...hat.com>; Alexander Shishkin
> <alexander.shishkin@...ux.intel.com>; Jiri Olsa <jolsa@...nel.org>; Hunter,
> Adrian <adrian.hunter@...el.com>; Kan Liang <kan.liang@...ux.intel.com>;
> linux-perf-users@...r.kernel.org; linux-kernel@...r.kernel.org; Taylor, Perry
> <perry.taylor@...el.com>; Alt, Samantha <samantha.alt@...el.com>; Biggers,
> Caleb <caleb.biggers@...el.com>
> Subject: Re: [RFC PATCH v9 4/7] perf stat: Plugin retire_lat value from sampled
> data to evsel
>
> On Tue, May 21, 2024 at 10:40 AM <weilin.wang@...el.com> wrote:
> >
> > From: Weilin Wang <weilin.wang@...el.com>
> >
> > In current :R parsing implementation, the parser would recognize events
> with
> > retire_latency modifier and insert them into the evlist like a normal event.
> > Ideally, we need to avoid counting these events.
> >
> > In this commit, at the time when a retire_latency evsel is read, set 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' modifier
> (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 need to be
sampled.
>
> >
> > Signed-off-by: Weilin Wang <weilin.wang@...el.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, const
> 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 not work.
>
> > + 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 *evsel,
> 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 = false;
> > + __u64 val;
> > +
> > + count = perf_counts(evsel->counts, cpu_map_idx, thread);
> > +
> > + list_for_each_entry(t, &tpebs_results, nd) {
> > + if (!strcmp(t->tpebs_name, evsel->name)) {
> > + found = true;
> > + break;
> > + }
> > + }
> > +
> > + if (!found)
> > + return -1;
> > +
> > + /*
> > + * Only set retire_latency value to the first CPU and thread.
> > + */
> > + if (cpu_map_idx == 0 && thread == 0)
> > + val = t->val;
> > + else
> > + val = 0;
> > +
> > + count->val = val;
> > + /* Set ena and run to non-zero */
> > + count->ena = count->run = 1;
> > + count->lost = 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.
Thanks,
Weilin
>
>
> > + return 0;
> > +}
> > +
> > static void evsel__set_count(struct evsel *counter, int cpu_map_idx, 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 = 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_idx, thread,
> true);
> > + return;
> > + }
> > +
> > count->val = val;
> > count->ena = ena;
> > count->run = 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, thread);
> > +
>
> 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 != 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
> >
Powered by blists - more mailing lists