* [PATCH] perf inject corrupts file by deleting event [not found] <83633eb2-04dc-4a13-3ad7-abd3a7459ac1@foss.arm.com> @ 2020-11-13 20:38 ` Al Grant 2020-11-16 16:25 ` Namhyung Kim 0 siblings, 1 reply; 7+ messages in thread From: Al Grant @ 2020-11-13 20:38 UTC (permalink / raw) To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, linux-perf-users "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> --- 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; ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] perf inject corrupts file by deleting event 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 0 siblings, 1 reply; 7+ messages in thread From: Namhyung Kim @ 2020-11-16 16:25 UTC (permalink / raw) To: Al Grant Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, linux-perf-users 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, 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; ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] perf inject corrupts file by deleting event 2020-11-16 16:25 ` Namhyung Kim @ 2020-11-16 16:45 ` Arnaldo Carvalho de Melo 2020-11-16 16:48 ` Arnaldo Carvalho de Melo 0 siblings, 1 reply; 7+ messages in thread From: Arnaldo Carvalho de Melo @ 2020-11-16 16:45 UTC (permalink / raw) To: Namhyung Kim Cc: Al Grant, Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin, Jiri Olsa, linux-perf-users 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] perf inject corrupts file by deleting event 2020-11-16 16:45 ` Arnaldo Carvalho de Melo @ 2020-11-16 16:48 ` Arnaldo Carvalho de Melo 2020-11-16 16:59 ` Arnaldo Carvalho de Melo 0 siblings, 1 reply; 7+ messages in thread From: Arnaldo Carvalho de Melo @ 2020-11-16 16:48 UTC (permalink / raw) To: Al Grant Cc: Namhyung Kim, Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin, Jiri Olsa, linux-perf-users Em Mon, Nov 16, 2020 at 01:45:33PM -0300, Arnaldo Carvalho de Melo escreveu: > Em Tue, Nov 17, 2020 at 01:25:09AM +0900, Namhyung Kim escreveu: > > On Sat, Nov 14, 2020 at 5:38 AM Al Grant <al.grant@foss.arm.com> wrote: > > > 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> > > 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? But I'll have to apply by hand: [acme@five perf]$ am /wb/1.patch warning: Patch sent with format=flowed; space at the end of lines might be lost. Applying: perf inject corrupts file by deleting event error: patch failed: tools/perf/builtin-inject.c:791 error: tools/perf/builtin-inject.c: patch does not apply Patch failed at 0001 perf inject corrupts file by deleting event hint: Use 'git am --show-current-patch=diff' to see the failed patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". [acme@five perf]$ ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] perf inject corrupts file by deleting event 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 0 siblings, 1 reply; 7+ messages in thread From: Arnaldo Carvalho de Melo @ 2020-11-16 16:59 UTC (permalink / raw) To: Al Grant Cc: Namhyung Kim, Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin, Jiri Olsa, linux-perf-users Em Mon, Nov 16, 2020 at 01:48:39PM -0300, Arnaldo Carvalho de Melo escreveu: > Em Mon, Nov 16, 2020 at 01:45:33PM -0300, Arnaldo Carvalho de Melo escreveu: > > Em Tue, Nov 17, 2020 at 01:25:09AM +0900, Namhyung Kim escreveu: > > > On Sat, Nov 14, 2020 at 5:38 AM Al Grant <al.grant@foss.arm.com> wrote: > > > > 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> > > > > 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? > > But I'll have to apply by hand: > > [acme@five perf]$ am /wb/1.patch > warning: Patch sent with format=flowed; space at the end of lines might be lost. > Applying: perf inject corrupts file by deleting event > error: patch failed: tools/perf/builtin-inject.c:791 > error: tools/perf/builtin-inject.c: patch does not apply > Patch failed at 0001 perf inject corrupts file by deleting event > hint: Use 'git am --show-current-patch=diff' to see the failed patch > When you have resolved this problem, run "git am --continue". > If you prefer to skip this patch, run "git am --skip" instead. > To restore the original branch and stop patching, run "git am --abort". > [acme@five perf]$ And you also forgot to update the comment and to remove the now unused 'evsel' variable, find the updated patch below, please check. Thanks, - Arnaldo commit 2dd954d9ec9059f330799a32d1cfa507beb18e1a Author: Al Grant <al.grant@foss.arm.com> Date: Fri Nov 13 20:38:26 2020 +0000 perf inject: Fix file corruption due to event deletion "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. Committer testing Removed the now unused 'evsel' variable, update the comment about the evsel removal not being performed anymore, and apply the patch manually as it failed with this warning: warning: Patch sent with format=flowed; space at the end of lines might be lost. Testing it with: $ perf bench internals inject-build-id # Running 'internals/inject-build-id' benchmark: Average build-id injection took: 8.543 msec (+- 0.130 msec) Average time per event: 0.838 usec (+- 0.013 usec) Average memory usage: 12717 KB (+- 9 KB) Average build-id-all injection took: 5.710 msec (+- 0.058 msec) Average time per event: 0.560 usec (+- 0.006 usec) Average memory usage: 12079 KB (+- 7 KB) $ Signed-off-by: Al Grant <al.grant@arm.com> Acked-by: Adrian Hunter <adrian.hunter@intel.com> Acked-by: Namhyung Kim <namhyung@kernel.org> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com> Cc: Jiri Olsa <jolsa@redhat.com> Cc: Mark Rutland <mark.rutland@arm.com> Cc: Peter Zijlstra <peterz@infradead.org> LPU-Reference: b9cf5611-daae-2390-3439-6617f8f0a34b@foss.arm.com Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com> diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c index 14d6c88fed76f249..43937f4b399ad2ca 100644 --- a/tools/perf/builtin-inject.c +++ b/tools/perf/builtin-inject.c @@ -779,25 +779,15 @@ static int __cmd_inject(struct perf_inject *inject) dsos__hit_all(session); /* * The AUX areas have been removed and replaced with - * synthesized hardware events, so clear the feature flag and - * remove the evsel. + * synthesized hardware events, so clear the feature flag. */ if (inject->itrace_synth_opts.set) { - struct evsel *evsel; - perf_header__clear_feat(&session->header, HEADER_AUXTRACE); if (inject->itrace_synth_opts.last_branch || 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; ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] perf inject corrupts file by deleting event 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 0 siblings, 1 reply; 7+ messages in thread From: Al Grant @ 2020-12-15 9:48 UTC (permalink / raw) To: Arnaldo Carvalho de Melo Cc: Namhyung Kim, Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin, Jiri Olsa, linux-perf-users On 16/11/2020 16:59, Arnaldo Carvalho de Melo wrote: > Em Mon, Nov 16, 2020 at 01:48:39PM -0300, Arnaldo Carvalho de Melo escreveu: >> Em Mon, Nov 16, 2020 at 01:45:33PM -0300, Arnaldo Carvalho de Melo escreveu: >>> Em Tue, Nov 17, 2020 at 01:25:09AM +0900, Namhyung Kim escreveu: >>>> On Sat, Nov 14, 2020 at 5:38 AM Al Grant <al.grant@foss.arm.com> wrote: >>>>> 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> >> >>>> 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? >> >> But I'll have to apply by hand: >> >> [acme@five perf]$ am /wb/1.patch >> warning: Patch sent with format=flowed; space at the end of lines might be lost. >> Applying: perf inject corrupts file by deleting event >> error: patch failed: tools/perf/builtin-inject.c:791 >> error: tools/perf/builtin-inject.c: patch does not apply >> Patch failed at 0001 perf inject corrupts file by deleting event >> hint: Use 'git am --show-current-patch=diff' to see the failed patch >> When you have resolved this problem, run "git am --continue". >> If you prefer to skip this patch, run "git am --skip" instead. >> To restore the original branch and stop patching, run "git am --abort". >> [acme@five perf]$ > > And you also forgot to update the comment and to remove the now unused > 'evsel' variable, find the updated patch below, please check. Thanks for tidying it up, it looks good to me. Do you need me to resubmit or can you take it as it is now with your changes? Al > > Thanks, > > - Arnaldo > > commit 2dd954d9ec9059f330799a32d1cfa507beb18e1a > Author: Al Grant <al.grant@foss.arm.com> > Date: Fri Nov 13 20:38:26 2020 +0000 > > perf inject: Fix file corruption due to event deletion > > "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. > > Committer testing > > Removed the now unused 'evsel' variable, update the comment about the > evsel removal not being performed anymore, and apply the patch manually > as it failed with this warning: > > warning: Patch sent with format=flowed; space at the end of lines might be lost. > > Testing it with: > > $ perf bench internals inject-build-id > # Running 'internals/inject-build-id' benchmark: > Average build-id injection took: 8.543 msec (+- 0.130 msec) > Average time per event: 0.838 usec (+- 0.013 usec) > Average memory usage: 12717 KB (+- 9 KB) > Average build-id-all injection took: 5.710 msec (+- 0.058 msec) > Average time per event: 0.560 usec (+- 0.006 usec) > Average memory usage: 12079 KB (+- 7 KB) > $ > > Signed-off-by: Al Grant <al.grant@arm.com> > Acked-by: Adrian Hunter <adrian.hunter@intel.com> > Acked-by: Namhyung Kim <namhyung@kernel.org> > Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com> > Cc: Jiri Olsa <jolsa@redhat.com> > Cc: Mark Rutland <mark.rutland@arm.com> > Cc: Peter Zijlstra <peterz@infradead.org> > LPU-Reference: b9cf5611-daae-2390-3439-6617f8f0a34b@foss.arm.com > Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com> > > diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c > index 14d6c88fed76f249..43937f4b399ad2ca 100644 > --- a/tools/perf/builtin-inject.c > +++ b/tools/perf/builtin-inject.c > @@ -779,25 +779,15 @@ static int __cmd_inject(struct perf_inject *inject) > dsos__hit_all(session); > /* > * The AUX areas have been removed and replaced with > - * synthesized hardware events, so clear the feature flag and > - * remove the evsel. > + * synthesized hardware events, so clear the feature flag. > */ > if (inject->itrace_synth_opts.set) { > - struct evsel *evsel; > - > perf_header__clear_feat(&session->header, > HEADER_AUXTRACE); > if (inject->itrace_synth_opts.last_branch || > 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; > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] perf inject corrupts file by deleting event 2020-12-15 9:48 ` Al Grant @ 2020-12-15 13:33 ` Arnaldo Carvalho de Melo 0 siblings, 0 replies; 7+ messages in thread From: Arnaldo Carvalho de Melo @ 2020-12-15 13:33 UTC (permalink / raw) To: Al Grant Cc: Namhyung Kim, Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin, Jiri Olsa, linux-perf-users Em Tue, Dec 15, 2020 at 09:48:32AM +0000, Al Grant escreveu: > On 16/11/2020 16:59, Arnaldo Carvalho de Melo wrote: > > Em Mon, Nov 16, 2020 at 01:48:39PM -0300, Arnaldo Carvalho de Melo escreveu: > > > Em Mon, Nov 16, 2020 at 01:45:33PM -0300, Arnaldo Carvalho de Melo escreveu: > > > > Em Tue, Nov 17, 2020 at 01:25:09AM +0900, Namhyung Kim escreveu: > > > > > On Sat, Nov 14, 2020 at 5:38 AM Al Grant <al.grant@foss.arm.com> wrote: > > > > > > 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> > > > > > > > > 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? > > > > > > But I'll have to apply by hand: > > > > > > [acme@five perf]$ am /wb/1.patch > > > warning: Patch sent with format=flowed; space at the end of lines might be lost. > > > Applying: perf inject corrupts file by deleting event > > > error: patch failed: tools/perf/builtin-inject.c:791 > > > error: tools/perf/builtin-inject.c: patch does not apply > > > Patch failed at 0001 perf inject corrupts file by deleting event > > > hint: Use 'git am --show-current-patch=diff' to see the failed patch > > > When you have resolved this problem, run "git am --continue". > > > If you prefer to skip this patch, run "git am --skip" instead. > > > To restore the original branch and stop patching, run "git am --abort". > > > [acme@five perf]$ > > > > And you also forgot to update the comment and to remove the now unused > > 'evsel' variable, find the updated patch below, please check. > > Thanks for tidying it up, it looks good to me. Do you need me to resubmit > or can you take it as it is now with your changes? > > Al Its already in v5.10: [acme@five perf]$ git log --oneline -1 1c756cd429d8f3da33d31f2a970284b9d5260534 1c756cd429d8f3da perf inject: Fix file corruption due to event deletion [acme@five perf]$ git tag --contains 1c756cd429d8f3da33d31f2a970284b9d5260534 | grep ^v v5.10 v5.10-rc5 v5.10-rc6 v5.10-rc7 [acme@five perf]$ Thanks for checking, - Arnaldo ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-12-15 13:33 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [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 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
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).