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=-10.1 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham 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 DE8CDC6369E for ; Mon, 16 Nov 2020 16:45:39 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 8B2DD221F8 for ; Mon, 16 Nov 2020 16:45:39 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=kernel.org header.i=@kernel.org header.b="0qbMlUT4" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1732311AbgKPQpg (ORCPT ); Mon, 16 Nov 2020 11:45:36 -0500 Received: from mail.kernel.org ([198.145.29.99]:60534 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1731586AbgKPQpf (ORCPT ); Mon, 16 Nov 2020 11:45:35 -0500 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 588B02222C; Mon, 16 Nov 2020 16:45:35 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1605545135; bh=b/mM6FTBdrX4Atsw6yN1L4ICk1j/rBmyLx6ki04QbPc=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=0qbMlUT4109h7uSRSL0y3fxua/ueNOXhH3LbHvZFLm/mgjEmOqS6L7la1hpFxZhw5 GKnZLupceOT8BGqjdXdHbHen+ri9SZ33N0I5ptfgVYHBwdue82HRZjnF9fX97oFhef LrelcF8VDLekDXsZ9dFZUzF74mK+yvDUZpqWZbe4= Received: by quaco.ghostprotocols.net (Postfix, from userid 1000) id 4652F411D1; Mon, 16 Nov 2020 13:45:33 -0300 (-03) Date: Mon, 16 Nov 2020 13:45:33 -0300 From: Arnaldo Carvalho de Melo To: Namhyung Kim Cc: Al Grant , Peter Zijlstra , Ingo Molnar , Mark Rutland , Alexander Shishkin , Jiri Olsa , linux-perf-users Subject: Re: [PATCH] perf inject corrupts file by deleting event Message-ID: <20201116164533.GC509215@kernel.org> References: <83633eb2-04dc-4a13-3ad7-abd3a7459ac1@foss.arm.com> 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: linux-perf-users@vger.kernel.org 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 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 > > Acked-by: Adrian Hunter > > Cc: Peter Zijlstra > > Cc: Ingo Molnar > > Cc: Arnaldo Carvalho de Melo > > Cc: Mark Rutland > > Cc: Alexander Shishkin > > Cc: Jiri Olsa > > Cc: Namhyung Kim > > 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