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  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Sat, 16 May 2020 00:41:50 +0200
From:   Jiri Olsa <jolsa@...hat.com>
To:     Ian Rogers <irogers@...gle.com>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Namhyung Kim <namhyung@...nel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        Andrii Nakryiko <andriin@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...omium.org>,
        Kajol Jain <kjain@...ux.ibm.com>,
        Andi Kleen <ak@...ux.intel.com>,
        John Garry <john.garry@...wei.com>,
        Jin Yao <yao.jin@...ux.intel.com>,
        Kan Liang <kan.liang@...ux.intel.com>,
        Cong Wang <xiyou.wangcong@...il.com>,
        Kim Phillips <kim.phillips@....com>,
        Adrian Hunter <adrian.hunter@...el.com>,
        Leo Yan <leo.yan@...aro.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        Stephane Eranian <eranian@...gle.com>
Subject: Re: [PATCH v2 7/7] perf expr: Migrate expr ids table to a hashmap

On Fri, May 15, 2020 at 02:35:43PM -0700, Ian Rogers wrote:
> On Fri, May 15, 2020 at 12:41 PM Jiri Olsa <jolsa@...hat.com> wrote:
> >
> > On Fri, May 15, 2020 at 09:50:07AM -0700, Ian Rogers wrote:
> >
> > SNIP
> >
> > > diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> > > index b071df373f8b..37be5a368d6e 100644
> > > --- a/tools/perf/util/metricgroup.c
> > > +++ b/tools/perf/util/metricgroup.c
> > > @@ -85,8 +85,7 @@ static void metricgroup__rblist_init(struct rblist *metric_events)
> > >
> > >  struct egroup {
> > >       struct list_head nd;
> > > -     int idnum;
> > > -     const char **ids;
> > > +     struct expr_parse_ctx pctx;
> > >       const char *metric_name;
> > >       const char *metric_expr;
> > >       const char *metric_unit;
> > > @@ -94,19 +93,21 @@ struct egroup {
> > >  };
> > >
> > >  static struct evsel *find_evsel_group(struct evlist *perf_evlist,
> > > -                                   const char **ids,
> > > -                                   int idnum,
> > > +                                   struct expr_parse_ctx *pctx,
> > >                                     struct evsel **metric_events,
> > >                                     bool *evlist_used)
> > >  {
> > >       struct evsel *ev;
> > > -     int i = 0, j = 0;
> > >       bool leader_found;
> > > +     const size_t idnum = hashmap__size(&pctx->ids);
> > > +     size_t i = 0;
> > > +     int j = 0;
> > > +     double *val_ptr;
> > >
> > >       evlist__for_each_entry (perf_evlist, ev) {
> > >               if (evlist_used[j++])
> > >                       continue;
> > > -             if (!strcmp(ev->name, ids[i])) {
> > > +             if (hashmap__find(&pctx->ids, ev->name, (void **)&val_ptr)) {
> >
> > hum, you sure it's doing the same thing as before?
> >
> > hashmap__find will succede all the time in here, while the
> > previous code was looking for the start of the group ...
> > the logic in here is little convoluted, so maybe I'm
> > missing some point in here ;-)
> 
> If we have a metric like "A + B" and another like "C / D" then by
> we'll generate a string (the extra_events strbuf in the code) like
> "{A,B}:W,{C,D}:W" from __metricgroup__add_metric. This will turn into
> an evlist in metricgroup__parse_groups of A,B,C,D. The code is trying
> to associate the events A,B with the first metric and C,D with the
> second. The code doesn't support sharing of events and events are
> marked as used and can't be part of other metrics. The evlist order is
> also reflective of the order of metrics, so if there were metrics "A +
> B + C" and "A + B", as the first metric is first in the evlist we
> don't run the risk of C being placed with A and B in a different
> group.
> 
> The old code used the order of events to match within a metric and say
> for metric "A+B+C" we want to match A then B, and so on. The new code
> acts more like a set, so "A + B + C" becomes a set containing A, B and
> C, we check A is in the set then B and then C. For both pieces of code
> they are only working because of the evlist_used "bitmap" and that the
> order in the evlists and metrics matches.
> 
> The current code could just use ordering to match first n1 events with
> the first metric, the next n2 events with the second and so on. So
> both the find now, and the strcmp before always return true in this
> branch.
> 
> In the RFC patch set I want to share events and so I do checks related
> to the group leader so that I know when moving from one group to
> another in the evlist. The find/strcmp becomes load bearing as I will
> re-use events as long as they match.
> https://lore.kernel.org/lkml/20200508053629.210324-14-irogers@google.com/
> 
> > jirka
> >
> > >                       if (!metric_events[i])
> > >                               metric_events[i] = ev;
> > >                       i++;
> > > @@ -118,7 +119,8 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
> > >                       memset(metric_events, 0,
> > >                               sizeof(struct evsel *) * idnum);
> 
> This re-check was unnecessary in the old code and unnecessary even
> more so now as the hashmap_find is given exactly the same arguments.
> I'll remove it in v3 while addressing Andrii's memory leak fixes.

ok, that was my other confusion, because it's useless ;-)

thanks for the explanation,
jirka

> 
> Thanks,
> Ian
> 
> > > -                     if (!strcmp(ev->name, ids[i])) {
> > > +                     if (hashmap__find(&pctx->ids, ev->name,
> > > +                                       (void **)&val_ptr)) {
> > >                               if (!metric_events[i])
> > >                                       metric_events[i] = ev;
> >
> > SNIP
> >
> 

Powered by blists - more mailing lists