From: Mathieu Poirier <mathieu.poirier@linaro.org>
To: Wei Li <liwei391@huawei.com>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>,
Mike Leach <mike.leach@linaro.org>,
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>,
Kim Phillips <kim.phillips@arm.com>,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org,
Peter Zijlstra <peterz@infradead.org>,
Ingo Molnar <mingo@redhat.com>,
leo.yan@linaro.org
Subject: Re: [PATCH 2/2] perf tools: Fix record failure when mixed with ARM SPE event
Date: Thu, 2 Jul 2020 17:03:22 -0600 [thread overview]
Message-ID: <20200702230322.GB471976@xps15> (raw)
In-Reply-To: <20200623123141.27747-3-liwei391@huawei.com>
Hi Li,
On Tue, Jun 23, 2020 at 08:31:41PM +0800, Wei Li wrote:
> When recording with cache-misses and arm_spe_x event, i found that
> it will just fail without showing any error info if i put cache-misses
> after arm_spe_x event.
>
> [root@localhost 0620]# perf record -e cache-misses -e \
> arm_spe_0/ts_enable=1,pct_enable=1,pa_enable=1,load_filter=1,\
> jitter=1,store_filter=1,min_latency=0/ sleep 1
> [ perf record: Woken up 1 times to write data ]
> [ perf record: Captured and wrote 0.067 MB perf.data ]
> [root@localhost 0620]# perf record -e \
> arm_spe_0/ts_enable=1,pct_enable=1,pa_enable=1,load_filter=1,jitter=1,\
> store_filter=1,min_latency=0/ -e cache-misses sleep 1
> [root@localhost 0620]#
>
> Finally, i found the reason is that the parameter 'arm_spe_pmu' passed to
> arm_spe_recording_init() in auxtrace_record__init() is wrong. When the
> arm_spe_x event is not the last event, 'arm_spe_pmus[i]' will be out of
> bounds.
Yes, this indeed broken.
The current code can only work if the only event to be
traced is an arm_spe_X, or if it is the last event to be specified.
Otherwise the last event type will be checked against all the
arm_spe_pmus[i]->types, none will match and an out of bound i index will be
used in arm_spc_recording_init().
Since this problem is not easy to figure out please include the above
explanation in the changelog.
>
> It seems that the code can't support concurrent multiple different
> arm_spe_x events currently. So add the code to check and record the
> found 'arm_spe_pmu' to fix this issue.
>
> In fact, we don't support concurrent multiple same arm_spe_x events either,
> that is checked in arm_spe_recording_options(), and it will show the
> relevant info.
>
> Fixes: ffd3d18c20b8d ("perf tools: Add ARM Statistical Profiling Extensions (SPE) support")
> Signed-off-by: Wei Li <liwei391@huawei.com>
> ---
> tools/perf/arch/arm/util/auxtrace.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/tools/perf/arch/arm/util/auxtrace.c b/tools/perf/arch/arm/util/auxtrace.c
> index 62b7b03d691a..7bb6f29e766c 100644
> --- a/tools/perf/arch/arm/util/auxtrace.c
> +++ b/tools/perf/arch/arm/util/auxtrace.c
> @@ -58,6 +58,7 @@ struct auxtrace_record
> bool found_etm = false;
> bool found_spe = false;
> static struct perf_pmu **arm_spe_pmus;
> + static struct perf_pmu *arm_spe_pmu;
As far as I can tell the "static" doesn't do anything.
> static int nr_spes = 0;
> int i = 0;
>
> @@ -77,6 +78,13 @@ struct auxtrace_record
>
> for (i = 0; i < nr_spes; i++) {
> if (evsel->core.attr.type == arm_spe_pmus[i]->type) {
> + if (found_spe && (arm_spe_pmu != arm_spe_pmus[i])) {
> + pr_err("Concurrent multiple SPE operation not currently supported\n");
> + *err = -EOPNOTSUPP;
> + return NULL;
> + }
Instead of the above, which as you rightly pointed out, is also done in
arm_spe_recording_options() it might be best to just fix the "if (!nr_spes)"
condition:
if (!nr_spes || arm_spe_pmu)
continue
Furthermore, instead of having a new arm_spe_pmu variable you could simply make
found_spe a struct perf_pmu. That would be one less variable to take care of.
> +
> + arm_spe_pmu = arm_spe_pmus[i];
> found_spe = true;
Last but not least do you know where the memory allocated for array arm_spe_pmus
is released? If you can't find it either then we have a memory leak and it
would be nice to have that fixed.
Regards
Mathieu
PS: Leo Yan has spent a fair amount of time in the SPE code - please CC him on
your next revision.
> break;
> }
> @@ -94,7 +102,7 @@ struct auxtrace_record
>
> #if defined(__aarch64__)
> if (found_spe)
> - return arm_spe_recording_init(err, arm_spe_pmus[i]);
> + return arm_spe_recording_init(err, arm_spe_pmu);
> #endif
>
> /*
> --
> 2.17.1
>
next prev parent reply other threads:[~2020-07-02 23:03 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-23 12:31 [PATCH 0/2] perf tools: Fix record failure when mixed with ARM SPE event Wei Li
2020-06-23 12:31 ` [PATCH 1/2] perf tools: ARM SPE code cleanup Wei Li
2020-07-02 22:39 ` Mathieu Poirier
2020-07-05 5:05 ` Leo Yan
2020-06-23 12:31 ` [PATCH 2/2] perf tools: Fix record failure when mixed with ARM SPE event Wei Li
2020-07-02 23:03 ` Mathieu Poirier [this message]
2020-07-03 4:06 ` liwei (GF)
2020-07-05 5:24 ` Leo Yan
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=20200702230322.GB471976@xps15 \
--to=mathieu.poirier@linaro.org \
--cc=acme@kernel.org \
--cc=alexander.shishkin@linux.intel.com \
--cc=jolsa@redhat.com \
--cc=kim.phillips@arm.com \
--cc=leo.yan@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=liwei391@huawei.com \
--cc=mark.rutland@arm.com \
--cc=mike.leach@linaro.org \
--cc=mingo@redhat.com \
--cc=namhyung@kernel.org \
--cc=peterz@infradead.org \
--cc=suzuki.poulose@arm.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).