From: Arnaldo Carvalho de Melo <acme@kernel.org>
To: Namhyung Kim <namhyung@kernel.org>
Cc: Al Grant <al.grant@foss.arm.com>,
Peter Zijlstra <peterz@infradead.org>,
Ingo Molnar <mingo@redhat.com>,
Mark Rutland <mark.rutland@arm.com>,
Alexander Shishkin <alexander.shishkin@linux.intel.com>,
Jiri Olsa <jolsa@redhat.com>,
linux-perf-users <linux-perf-users@vger.kernel.org>
Subject: Re: [PATCH] perf inject corrupts file by deleting event
Date: Mon, 16 Nov 2020 13:45:33 -0300 [thread overview]
Message-ID: <20201116164533.GC509215@kernel.org> (raw)
In-Reply-To: <CAM9d7cgmQ552DFpPo6b8kCwq4B9faSPxGE3tTBW6jfJd1xMVwA@mail.gmail.com>
Em Tue, Nov 17, 2020 at 01:25:09AM +0900, Namhyung Kim escreveu:
> Hello,
>
> On Sat, Nov 14, 2020 at 5:38 AM Al Grant <al.grant@foss.arm.com> wrote:
> >
> > "perf inject" can create corrupt files when synthesizing sample
> > events from AUX data. This happens when in the input file, the
> > first event (for the AUX data) has a different sample_type from
> > the second event (generally dummy). Specifically, they differ in
> > the bits that indicate the standard fields appended to perf
> > records in the mmap buffer. "perf inject" deletes the first event
> > and moves up the second event to first position. The problem is
> > with the synthetic PERF_RECORD_MMAP (etc.) events created by
> > "perf record". Since these are synthetic versions of events which
> > are normally produced by the kernel, they have to have the
> > standard fields appended as described by sample_type. "perf record"
> > fills these in with zeroes, including the IDENTIFIER field;
> > perf readers interpret records with zero IDENTIFIER using the
> > descriptor for the first event in the file. Since "perf inject"
> > changes the first event, these synthetic records are then
> > processed with the wrong value of sample_type, and the perf
> > reader reads bad data, reports on incorrect length records etc.
> >
> > Mismatching sample_types are seen with "perf record -e cs_etm//",
> > where the AUX event has TID|TIME|CPU|IDENTIFIER and the dummy
> > event has TID|TIME|IDENTIFIER. Perhaps they could be the same,
> > but it isn't normally a problem if they aren't - perf has
> > no problems reading the file. The sample_types have to agree on
> > the position of IDENTIFIER, because that's how perf finds the
> > right event descriptor in the first place, but they don't normally
> > have to agree on other fields, and perf doesn't check that they do.
> > The problem is specific to the way "perf inject" reorganizes the
> > events and the way synthetic MMAP events are recorded with a zero
> > identifier. A simple solution is to stop "perf inject" deleting
> > the tracing event.
> >
> > Signed-off-by: Al Grant <al.grant@arm.com>
> > Acked-by: Adrian Hunter <adrian.hunter@intel.com>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Ingo Molnar <mingo@redhat.com>
> > Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> > Cc: Jiri Olsa <jolsa@redhat.com>
> > Cc: Namhyung Kim <namhyung@kernel.org>
>
> While I'm ok with this change,
So, to make progress, I'll take your phrase as an Acked-by, as described
in Documentation/process/submitting-patches.rst, ok?
The rest of your comments below can be done as some follow up work,
Thanks,
- Arnaldo
> I think we can put the dummy events
> to the front of the evlist (during record) so that we can make sure that
> tracking records would refer to them in order to parse the data.
>
> And I also think that we should omit the dummy events from the
> perf report output.
>
> Thanks,
> Namhyung
>
>
> > ---
> > tools/perf/builtin-inject.c | 7 -------
> > 1 file changed, 7 deletions(-)
> >
> > diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
> > index 452a75fe68e5..f4968ebf5f3a 100644
> > --- a/tools/perf/builtin-inject.c
> > +++ b/tools/perf/builtin-inject.c
> > @@ -791,13 +791,6 @@ static int __cmd_inject(struct perf_inject *inject)
> > inject->itrace_synth_opts.add_last_branch)
> > perf_header__set_feat(&session->header,
> > HEADER_BRANCH_STACK);
> > - evsel = perf_evlist__id2evsel_strict(session->evlist,
> > - inject->aux_id);
> > - if (evsel) {
> > - pr_debug("Deleting %s\n", evsel__name(evsel));
> > - evlist__remove(session->evlist, evsel);
> > - evsel__delete(evsel);
> > - }
> > }
> > session->header.data_offset = output_data_offset;
> > session->header.data_size = inject->bytes_written;
--
- Arnaldo
next prev parent reply other threads:[~2020-11-16 16:45 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <83633eb2-04dc-4a13-3ad7-abd3a7459ac1@foss.arm.com>
2020-11-13 20:38 ` [PATCH] perf inject corrupts file by deleting event Al Grant
2020-11-16 16:25 ` Namhyung Kim
2020-11-16 16:45 ` Arnaldo Carvalho de Melo [this message]
2020-11-16 16:48 ` Arnaldo Carvalho de Melo
2020-11-16 16:59 ` Arnaldo Carvalho de Melo
2020-12-15 9:48 ` Al Grant
2020-12-15 13:33 ` 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=20201116164533.GC509215@kernel.org \
--to=acme@kernel.org \
--cc=al.grant@foss.arm.com \
--cc=alexander.shishkin@linux.intel.com \
--cc=jolsa@redhat.com \
--cc=linux-perf-users@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=mingo@redhat.com \
--cc=namhyung@kernel.org \
--cc=peterz@infradead.org \
/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).