BPF Archive on lore.kernel.org
 help / color / Atom feed
From: Ian Rogers <irogers@google.com>
To: Jiri Olsa <jolsa@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>,
	Ingo Molnar <mingo@redhat.com>,
	Arnaldo Carvalho de Melo <acme@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Alexander Shishkin <alexander.shishkin@linux.intel.com>,
	Namhyung Kim <namhyung@kernel.org>,
	Song Liu <songliubraving@fb.com>,
	Andrii Nakryiko <andriin@fb.com>,
	Kajol Jain <kjain@linux.ibm.com>, Andi Kleen <ak@linux.intel.com>,
	John Garry <john.garry@huawei.com>,
	Jin Yao <yao.jin@linux.intel.com>,
	Kan Liang <kan.liang@linux.intel.com>,
	Cong Wang <xiyou.wangcong@gmail.com>,
	Kim Phillips <kim.phillips@amd.com>, Paul Clarke <pc@us.ibm.com>,
	Srikar Dronamraju <srikar@linux.vnet.ibm.com>,
	LKML <linux-kernel@vger.kernel.org>,
	Networking <netdev@vger.kernel.org>, bpf <bpf@vger.kernel.org>,
	linux-perf-users <linux-perf-users@vger.kernel.org>,
	Vince Weaver <vincent.weaver@maine.edu>,
	Stephane Eranian <eranian@google.com>
Subject: Re: [PATCH 5/7] perf metricgroup: Remove duped metric group events
Date: Wed, 20 May 2020 09:50:13 -0700
Message-ID: <CAP-5=fVGf9i7hvQcht_8mnMMjzhQYdFqPzZFraE-iMR7Vcr1tw@mail.gmail.com> (raw)
In-Reply-To: <20200520134847.GM157452@krava>

On Wed, May 20, 2020 at 6:49 AM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Wed, May 20, 2020 at 12:28:12AM -0700, Ian Rogers wrote:
>
> SNIP
>
> >
> > @@ -157,7 +183,7 @@ static int metricgroup__setup_events(struct list_head *groups,
> >       int i = 0;
> >       int ret = 0;
> >       struct egroup *eg;
> > -     struct evsel *evsel;
> > +     struct evsel *evsel, *tmp;
> >       unsigned long *evlist_used;
> >
> >       evlist_used = bitmap_alloc(perf_evlist->core.nr_entries);
> > @@ -173,7 +199,8 @@ static int metricgroup__setup_events(struct list_head *groups,
> >                       ret = -ENOMEM;
> >                       break;
> >               }
> > -             evsel = find_evsel_group(perf_evlist, &eg->pctx, metric_events,
> > +             evsel = find_evsel_group(perf_evlist, &eg->pctx,
> > +                                     eg->has_constraint, metric_events,
> >                                       evlist_used);
> >               if (!evsel) {
> >                       pr_debug("Cannot resolve %s: %s\n",
> > @@ -200,6 +227,12 @@ static int metricgroup__setup_events(struct list_head *groups,
> >               list_add(&expr->nd, &me->head);
> >       }
> >
> > +     evlist__for_each_entry_safe(perf_evlist, tmp, evsel) {
> > +             if (!test_bit(evsel->idx, evlist_used)) {
> > +                     evlist__remove(perf_evlist, evsel);
> > +                     evsel__delete(evsel);
> > +             }
>
> is the groupping still enabled when we merge groups? could part
> of the metric (events) be now computed in different groups?

By default the change will take two metrics and allow the shorter
metric (in terms of number of events) to share the events of the
longer metric. If the events for the shorter metric aren't in the
longer metric then the shorter metric must use its own group of
events. If sharing has occurred then the bitmap is used to work out
which events and groups are no longer in use.

With --metric-no-group then any event can be used for a metric as
there is no grouping. This is why more events can be eliminated.

With --metric-no-merge then the logic to share events between
different metrics is disabled and every metric is in a group. This
allows the current behavior to be had.

There are some corner cases, such as metrics with constraints (that
don't group) and duration_time that is never placed into a group.

Partial sharing, with one event in 1 weak event group and 1 in another
is never performed. Using --metric-no-group allows something similar.
Given multiplexing, I'd be concerned about accuracy problems if events
between groups were shared - say for IPC, were you measuring
instructions and cycles at the same moment?

> I was wondering if we could merge all the hasmaps into single
> one before the parse the evlist.. this way we won't need removing
> later.. but I did not thought this through completely, so it
> might not work at some point

This could be done in the --metric-no-group case reasonably easily
like the current hashmap. For groups you'd want something like a set
of sets of events, but then you'd only be able to share events if the
sets were the same. A directed acyclic graph could capture the events
and the sharing relationships, it may be possible to optimize cases
like {A,B,C}, {A,B,D}, {A,B} so that the small group on the end shares
events with both the {A,B,C} and {A,B,D} group. This may be good
follow up work. We could also solve this in the json, for example
create a "phony" group of {A,B,C,D} that all three metrics share from.
You could also use --metric-no-group to achieve that sharing now.

Thanks,
Ian

> jirka
>

  reply index

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-20  7:28 [PATCH 0/7] Share events between metrics Ian Rogers
2020-05-20  7:28 ` [PATCH 1/7] perf metricgroup: Change evlist_used to a bitmap Ian Rogers
2020-05-20 13:14   ` Jiri Olsa
2020-05-20 13:35     ` Arnaldo Carvalho de Melo
2020-05-20  7:28 ` [PATCH 2/7] perf metricgroup: Always place duration_time last Ian Rogers
2020-05-20  7:28 ` [PATCH 3/7] perf metricgroup: Delay events string creation Ian Rogers
2020-05-20 13:14   ` Jiri Olsa
2020-05-20 18:22     ` Ian Rogers
2020-05-20 20:34       ` Arnaldo Carvalho de Melo
2020-05-20 22:10         ` Jiri Olsa
2020-05-20  7:28 ` [PATCH 4/7] perf metricgroup: Order event groups by size Ian Rogers
2020-05-20  7:28 ` [PATCH 5/7] perf metricgroup: Remove duped metric group events Ian Rogers
2020-05-20 13:48   ` Jiri Olsa
2020-05-20 16:50     ` Ian Rogers [this message]
2020-05-20 22:09       ` Jiri Olsa
2020-05-20 22:42         ` Ian Rogers
2020-05-21 10:54           ` Jiri Olsa
2020-05-21 17:26             ` Ian Rogers
2020-05-21 17:28               ` Arnaldo Carvalho de Melo
2020-05-20  7:28 ` [PATCH 6/7] perf metricgroup: Add options to not group or merge Ian Rogers
2020-05-20  7:28 ` [PATCH 7/7] perf metricgroup: Remove unnecessary ',' from events Ian Rogers
2020-05-20 13:13 ` [PATCH 0/7] Share events between metrics Jiri Olsa
2020-05-20 14:50   ` Ian Rogers

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAP-5=fVGf9i7hvQcht_8mnMMjzhQYdFqPzZFraE-iMR7Vcr1tw@mail.gmail.com' \
    --to=irogers@google.com \
    --cc=acme@kernel.org \
    --cc=ak@linux.intel.com \
    --cc=alexander.shishkin@linux.intel.com \
    --cc=andriin@fb.com \
    --cc=bpf@vger.kernel.org \
    --cc=eranian@google.com \
    --cc=john.garry@huawei.com \
    --cc=jolsa@redhat.com \
    --cc=kan.liang@linux.intel.com \
    --cc=kim.phillips@amd.com \
    --cc=kjain@linux.ibm.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-perf-users@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=mingo@redhat.com \
    --cc=namhyung@kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=pc@us.ibm.com \
    --cc=peterz@infradead.org \
    --cc=songliubraving@fb.com \
    --cc=srikar@linux.vnet.ibm.com \
    --cc=vincent.weaver@maine.edu \
    --cc=xiyou.wangcong@gmail.com \
    --cc=yao.jin@linux.intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

BPF Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/bpf/0 bpf/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 bpf bpf/ https://lore.kernel.org/bpf \
		bpf@vger.kernel.org
	public-inbox-index bpf

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.bpf


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git