From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-16.3 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,INCLUDES_PATCH,MAILING_LIST_MULTI, MENTIONS_GIT_HOSTING,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 1800DC2D0A8 for ; Wed, 23 Sep 2020 20:25:19 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id BFA46206DB for ; Wed, 23 Sep 2020 20:25:18 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1600892718; bh=ZOVsM5eMpLXXum2JcEHZfF1tPbVCWbObTLlQYheEv6Y=; h=Date:From:To:Cc:Subject:References:In-Reply-To:List-ID:From; b=hwZCzzfr5BNzaMEkDuUxVBViMETRF7H9E/H1x9BRY5o9SERO8Kal+6Fcw7JaPrtd3 CWd6LtNYSCPRVwQp6QVDBIXvTbYuS5oYez7K/nkSZ2OIZMswd4BgZBb+ytHwLBEU1R +g1mdio8CntXeJNHo727/vIZBo9nWFtz53n0s8EQ= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726709AbgIWUZP (ORCPT ); Wed, 23 Sep 2020 16:25:15 -0400 Received: from mail.kernel.org ([198.145.29.99]:58446 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726681AbgIWUZN (ORCPT ); Wed, 23 Sep 2020 16:25:13 -0400 Received: from quaco.ghostprotocols.net (unknown [179.97.37.151]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 3611E20725; Wed, 23 Sep 2020 20:25:11 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1600892711; bh=ZOVsM5eMpLXXum2JcEHZfF1tPbVCWbObTLlQYheEv6Y=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=1NbFYn5SF3eKcDGbhiZD4lken0+npLZp8jkT1C+IAWfBhbmb8RaELJ27wD5Oi6RwY xu4KwKNqHlhZrrhzoZR5A3Irtl1DFu/XCUa5JUDs43aKY9nWomwmC/jaiTxAcLrkbH 76KEQnBpEOPmSvuMxWW0tnlVY2qecCrlxbTU/Ivk= Received: by quaco.ghostprotocols.net (Postfix, from userid 1000) id B6E1F400E9; Wed, 23 Sep 2020 17:25:08 -0300 (-03) Date: Wed, 23 Sep 2020 17:25:08 -0300 From: Arnaldo Carvalho de Melo To: Ian Rogers Cc: Namhyung Kim , Peter Zijlstra , Ingo Molnar , Mark Rutland , Alexander Shishkin , Jiri Olsa , Alexei Starovoitov , Daniel Borkmann , Martin KaFai Lau , Song Liu , Yonghong Song , Andrii Nakryiko , John Fastabend , KP Singh , Adrian Hunter , Andi Kleen , Athira Rajeev , linux-kernel , Network Development , bpf , Stephane Eranian , Jin Yao Subject: Re: [PATCH v4] perf metricgroup: Fix uncore metric expressions Message-ID: <20200923202508.GG2517482@kernel.org> References: <20200917190026.GB1426933@kernel.org> <20200917201807.4090224-1-irogers@google.com> <20200917203953.GA1525630@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Url: http://acmel.wordpress.com Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org Em Wed, Sep 23, 2020 at 12:59:28PM -0700, Ian Rogers escreveu: > On Thu, Sep 17, 2020 at 6:47 PM Namhyung Kim wrote: > > > > Hi Arnaldo, > > > > On Fri, Sep 18, 2020 at 5:39 AM Arnaldo Carvalho de Melo > > wrote: > > > > > > Em Thu, Sep 17, 2020 at 01:18:07PM -0700, Ian Rogers escreveu: > > > > A metric like DRAM_BW_Use has on SkylakeX events uncore_imc/cas_count_read/ > > > > and uncore_imc/case_count_write/. These events open 6 events per socket > > > > with pmu names of uncore_imc_[0-5]. The current metric setup code in > > > > find_evsel_group assumes one ID will map to 1 event to be recorded in > > > > metric_events. For events with multiple matches, the first event is > > > > recorded in metric_events (avoiding matching >1 event with the same > > > > name) and the evlist_used updated so that duplicate events aren't > > > > removed when the evlist has unused events removed. > > > > > > Namhyung, please check if you still Acks this as you provided it for v3. > > > > Sure, > > > > Acked-by: Namhyung Kim > > Thanks Namhyung and Arnaldo, could we merge this? [acme@five perf]$ git log --oneline perf/urgent dcc81be0fc4e6694 (quaco/perf/urgent, perf/urgent) perf metricgroup: Fix uncore metric expressions 0f1b550e29c1f952 perf parse-event: Release cpu_map refcount if evsel alloc failed 5d680be3b014d583 perf parse-event: Fix cpu map refcounting 5925fa68fe824465 (torvalds/master) Merge tag 'perf-tools-fixes-for-v5.9-2020-09-16' of git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux I have it already locally, need to finish a round of testing so that I can push it publicly, - Arnaldo > Ian > > > Thanks > > Namhyung > > > > > > > > > Before this change: > > > > $ /tmp/perf/perf stat -M DRAM_BW_Use -a -- sleep 1 > > > > > > > > Performance counter stats for 'system wide': > > > > > > > > 41.14 MiB uncore_imc/cas_count_read/ > > > > 1,002,614,251 ns duration_time > > > > > > > > 1.002614251 seconds time elapsed > > > > > > > > After this change: > > > > $ /tmp/perf/perf stat -M DRAM_BW_Use -a -- sleep 1 > > > > > > > > Performance counter stats for 'system wide': > > > > > > > > 157.47 MiB uncore_imc/cas_count_read/ # 0.00 DRAM_BW_Use > > > > 126.97 MiB uncore_imc/cas_count_write/ > > > > 1,003,019,728 ns duration_time > > > > > > > > Erroneous duplication introduced in: > > > > commit 2440689d62e9 ("perf metricgroup: Remove duped metric group events"). > > > > > > > > Fixes: ded80bda8bc9 ("perf expr: Migrate expr ids table to a hashmap"). > > > > Reported-by: Jin Yao > > > > Signed-off-by: Ian Rogers > > > > --- > > > > tools/perf/util/metricgroup.c | 75 ++++++++++++++++++++++++++--------- > > > > 1 file changed, 56 insertions(+), 19 deletions(-) > > > > > > > > diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c > > > > index ab5030fcfed4..d948a7f910cf 100644 > > > > --- a/tools/perf/util/metricgroup.c > > > > +++ b/tools/perf/util/metricgroup.c > > > > @@ -150,6 +150,18 @@ static void expr_ids__exit(struct expr_ids *ids) > > > > free(ids->id[i].id); > > > > } > > > > > > > > +static bool contains_event(struct evsel **metric_events, int num_events, > > > > + const char *event_name) > > > > +{ > > > > + int i; > > > > + > > > > + for (i = 0; i < num_events; i++) { > > > > + if (!strcmp(metric_events[i]->name, event_name)) > > > > + return true; > > > > + } > > > > + return false; > > > > +} > > > > + > > > > /** > > > > * Find a group of events in perf_evlist that correpond to those from a parsed > > > > * metric expression. Note, as find_evsel_group is called in the same order as > > > > @@ -180,7 +192,11 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist, > > > > int i = 0, matched_events = 0, events_to_match; > > > > const int idnum = (int)hashmap__size(&pctx->ids); > > > > > > > > - /* duration_time is grouped separately. */ > > > > + /* > > > > + * duration_time is always grouped separately, when events are grouped > > > > + * (ie has_constraint is false) then ignore it in the matching loop and > > > > + * add it to metric_events at the end. > > > > + */ > > > > if (!has_constraint && > > > > hashmap__find(&pctx->ids, "duration_time", (void **)&val_ptr)) > > > > events_to_match = idnum - 1; > > > > @@ -207,23 +223,20 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist, > > > > sizeof(struct evsel *) * idnum); > > > > current_leader = ev->leader; > > > > } > > > > - if (hashmap__find(&pctx->ids, ev->name, (void **)&val_ptr)) { > > > > - if (has_constraint) { > > > > - /* > > > > - * Events aren't grouped, ensure the same event > > > > - * isn't matched from two groups. > > > > - */ > > > > - for (i = 0; i < matched_events; i++) { > > > > - if (!strcmp(ev->name, > > > > - metric_events[i]->name)) { > > > > - break; > > > > - } > > > > - } > > > > - if (i != matched_events) > > > > - continue; > > > > - } > > > > + /* > > > > + * Check for duplicate events with the same name. For example, > > > > + * uncore_imc/cas_count_read/ will turn into 6 events per socket > > > > + * on skylakex. Only the first such event is placed in > > > > + * metric_events. If events aren't grouped then this also > > > > + * ensures that the same event in different sibling groups > > > > + * aren't both added to metric_events. > > > > + */ > > > > + if (contains_event(metric_events, matched_events, ev->name)) > > > > + continue; > > > > + /* Does this event belong to the parse context? */ > > > > + if (hashmap__find(&pctx->ids, ev->name, (void **)&val_ptr)) > > > > metric_events[matched_events++] = ev; > > > > - } > > > > + > > > > if (matched_events == events_to_match) > > > > break; > > > > } > > > > @@ -239,7 +252,7 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist, > > > > } > > > > > > > > if (matched_events != idnum) { > > > > - /* Not whole match */ > > > > + /* Not a whole match */ > > > > return NULL; > > > > } > > > > > > > > @@ -247,8 +260,32 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist, > > > > > > > > for (i = 0; i < idnum; i++) { > > > > ev = metric_events[i]; > > > > - ev->metric_leader = ev; > > > > + /* Don't free the used events. */ > > > > set_bit(ev->idx, evlist_used); > > > > + /* > > > > + * The metric leader points to the identically named event in > > > > + * metric_events. > > > > + */ > > > > + ev->metric_leader = ev; > > > > + /* > > > > + * Mark two events with identical names in the same group (or > > > > + * globally) as being in use as uncore events may be duplicated > > > > + * for each pmu. Set the metric leader of such events to be the > > > > + * event that appears in metric_events. > > > > + */ > > > > + evlist__for_each_entry_continue(perf_evlist, ev) { > > > > + /* > > > > + * If events are grouped then the search can terminate > > > > + * when then group is left. > > > > + */ > > > > + if (!has_constraint && > > > > + ev->leader != metric_events[i]->leader) > > > > + break; > > > > + if (!strcmp(metric_events[i]->name, ev->name)) { > > > > + set_bit(ev->idx, evlist_used); > > > > + ev->metric_leader = metric_events[i]; > > > > + } > > > > + } > > > > } > > > > > > > > return metric_events[0]; > > > > -- > > > > 2.28.0.618.gf4bc123cb7-goog > > > > > > > > > > -- > > > > > > - Arnaldo -- - Arnaldo