From: John Garry <john.garry@huawei.com>
To: Ian Rogers <irogers@google.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>,
Jiri Olsa <jolsa@redhat.com>,
"Namhyung Kim" <namhyung@kernel.org>,
kajoljain <kjain@linux.ibm.com>,
LKML <linux-kernel@vger.kernel.org>, <linuxarm@openeuler.org>,
Joakim Zhang <qiangqing.zhang@nxp.com>
Subject: Re: [PATCH] perf metricgroup: Fix for metrics containing duration_time
Date: Wed, 20 Jan 2021 17:08:36 +0000 [thread overview]
Message-ID: <26a9e447-b2ef-c459-ebf1-9992ee5c5cd0@huawei.com> (raw)
In-Reply-To: <CAP-5=fVr0pFpqpev0DW6MMYB1VouH4rL0_wY3_OsbQLS=deJag@mail.gmail.com>
On 20/01/2021 16:40, Ian Rogers wrote:
> On Wed, Jan 20, 2021 at 8:23 AM John Garry <john.garry@huawei.com
> <mailto:john.garry@huawei.com>> wrote:
>
> Metrics containing duration_time cause a segfault:
>
> $./perf stat -v -M L1D_Cache_Fill_BW sleep 1
> Using CPUID GenuineIntel-6-3D-4
> metric expr 64 * l1d.replacement / 1000000000 / duration_time for
> L1D_Cache_Fill_BW
> found event duration_time
> found event l1d.replacement
> adding {l1d.replacement}:W,duration_time
> l1d.replacement -> cpu/umask=0x1,(null)=0x1e8483,event=0x51/
> Segmentation fault
>
> In commit c2337d67199a ("perf metricgroup: Fix metrics using aliases
> covering multiple PMUs"), the logic in find_evsel_group() when iter'ing
> events was changed to not only select events in same group, but also for
> aliased PMUs.
>
> Checking whether events were for aliased PMUs was done by comparing the
> event PMU name. This was not safe for duration_time event, which has no
> associated PMU (and no PMU name), so fix by checking if the event
> PMU name
> is set also.
>
>
> Thanks for this, it should be fairly easy to add a test. Could we do this?
I don't mind following up with that.
>
> Fixes: c2337d67199a ("perf metricgroup: Fix metrics using aliases
> covering multiple PMUs")
> Reported-by: Joakim Zhang <qiangqing.zhang@nxp.com
> <mailto:qiangqing.zhang@nxp.com>>
> Signed-off-by: John Garry <john.garry@huawei.com
> <mailto:john.garry@huawei.com>>
>
> diff --git a/tools/perf/util/metricgroup.c
> b/tools/perf/util/metricgroup.c
> index 2e60ee170abc..e6d3452031e5 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -162,6 +162,14 @@ static bool contains_event(struct evsel
> **metric_events, int num_events,
> return false;
> }
>
> +static bool evsel_same_pmu(struct evsel *ev1, struct evsel *ev2)
> +{
> + if (!ev1->pmu_name || !ev2->pmu_name)
> + return false;
>
>
> What about the case of "!ev1->pmu_name && !ev2->pmu_name" ?
As far as I know, it should not happen, since duration_time is a special
event. More below.
>
> Thanks,
> Ian
>
> +
> + return !strcmp(ev1->pmu_name, ev2->pmu_name);
> +}
> +
> /**
> * Find a group of events in perf_evlist that correspond to those
> from a parsed
> * metric expression. Note, as find_evsel_group is called in the
> same order as
> @@ -280,8 +288,7 @@ static struct evsel *find_evsel_group(struct
> evlist *perf_evlist,
> */
> if (!has_constraint &&
> ev->leader != metric_events[i]->leader &&
> - !strcmp(ev->leader->pmu_name,
> - metric_events[i]->leader->pmu_name))
> + evsel_same_pmu(ev->leader,
> metric_events[i]->leader))
ev->leader->pmu_name == NULL for only duration_time event. And we don't
get here for ev == metric_events[i] == duration_time event (as we use
evlist__for_each_entry_continue() and duration_time is always last in
metric_events[]), so both event arguments should not have pmu_name ==
NULL. Indeed, I could just check metric_events[i]->leader->pmu_name !=
NULL, but thought it better to check both for safety.
Cheers,
John
> break;
> if (!strcmp(metric_events[i]->name,
> ev->name)) {
> set_bit(ev->idx, evlist_used);
> --
> 2.26.2
>
next prev parent reply other threads:[~2021-01-20 17:15 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-20 16:18 [PATCH] perf metricgroup: Fix for metrics containing duration_time John Garry
[not found] ` <CAP-5=fVr0pFpqpev0DW6MMYB1VouH4rL0_wY3_OsbQLS=deJag@mail.gmail.com>
2021-01-20 17:08 ` John Garry [this message]
[not found] ` <CAP-5=fX3C8v02ZbXPGWs1eKrbO5YAddegJuig2B755=Ubd1w1Q@mail.gmail.com>
2021-01-21 17:14 ` John Garry
2021-01-20 21:35 ` Jiri Olsa
2021-01-21 20:02 ` Arnaldo Carvalho de Melo
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=26a9e447-b2ef-c459-ebf1-9992ee5c5cd0@huawei.com \
--to=john.garry@huawei.com \
--cc=acme@kernel.org \
--cc=alexander.shishkin@linux.intel.com \
--cc=irogers@google.com \
--cc=jolsa@redhat.com \
--cc=kjain@linux.ibm.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linuxarm@openeuler.org \
--cc=mark.rutland@arm.com \
--cc=mingo@redhat.com \
--cc=namhyung@kernel.org \
--cc=peterz@infradead.org \
--cc=qiangqing.zhang@nxp.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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).