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=-13.8 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable 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 25A52C433DB for ; Sat, 20 Feb 2021 11:53:04 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id CDBE464E43 for ; Sat, 20 Feb 2021 11:53:03 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org CDBE464E43 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linaro.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References:Message-ID: Subject:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=Qe0EIUs7zC0u2+O4kojcHRwwlFdXJJZHwTeDQgJw5cg=; b=D7yCmzblujaiULvAwY0fq5+1o ieEOGFL4eVVlAJi46sgq4eClUIgxH0a9vr2623ML+xqf29oy8uhJnPBuzAIXC8GSFiKE/UYfuFlyP BYoIIu2u/ZCKIQWDEeLWHmCj++qGc86BfxlMrlfjkL2KBiYtMb6gg43+ihth4VftWKm43JPRYoFxo WHFN2dPSdHGXGTxNLLmxMVTTkxqyCei21Is1trzdXS5wBWBhmw4DvLiNeEbQxRcoKP1agodQ+R2/6 4jEVFL/G5WZPmk0NFXp1KoLGbABtfIPaOxXWdV/Rad80k+z5BwiY8DxuwAj0g1HFkjblQI0eLI/2g T08Q49Oug==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1lDQmP-0003lD-OR; Sat, 20 Feb 2021 11:50:33 +0000 Received: from mail-pg1-x529.google.com ([2607:f8b0:4864:20::529]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1lDQmM-0003kX-OL for linux-arm-kernel@lists.infradead.org; Sat, 20 Feb 2021 11:50:32 +0000 Received: by mail-pg1-x529.google.com with SMTP id t25so7083740pga.2 for ; Sat, 20 Feb 2021 03:50:29 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=kg4ioz6bfAx/Lh/IDD2LjV5Zlw8cQV4X2x+CpujDirY=; b=W5jp7kOiLw8h3tIpkTtMIPz2rBIxw6YC4wN5n81mLHuRu/jVLT8Jmxa7PFdmvRYjPJ 04hR8gAgoojdVy/LtQ72AgVa4EDrvn3hIbafj16CXLvryYEZXT4wqc7MaDKrZg+vABK3 /oyMKPc23Mi6qOKly0BpZT7c5KKhdqhBo40Z7o4nB0eiNNDtaSfEW5IYvaFFuf8ebQdX m2VaUHU2iB2i74wcOoIMFhmqgyy5LKwBICUW+83N/CewApx78Hn6VZIWz/42FE6dxWdi YZw0uH0AS0i2Vsv/AuLiF2YMEUndF9c3QMBeWcDNYjXt6h1foCu+TczsSMJBu/mtKchN AyCA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=kg4ioz6bfAx/Lh/IDD2LjV5Zlw8cQV4X2x+CpujDirY=; b=kDCwiLeq4EX3gL44CyTpEq/WV3xRKeH1T5syrczEKUmX4l1DV9Y8Ed5bsdFlRv0ThI cnBY8D1RaNr9MmDjTKwwcDygbGgX+La9UZYg4wHTg0qFHr1xfmJkGnviERTRR3yJPMIC 3gpm1mxurK4qU6tZ3AN3Cr3IdA3T+ihjNwXa1EkkIBljutKbuKgHyYYL4UGqZT9jYTof DXzzikbEPtqdbUamjT/MhZGeUZ7pQYmK4Zd1xU52fCJvzvg7pFoxwZJAHE49ggP2cex7 rqPUeEUG+pshdMUdn9yXNbpxYnsh8FMUc/mHseILW4dJ2mP1nAJOPvq0FiQskrGy4wJr 5EXw== X-Gm-Message-State: AOAM530I8ueZb3aLYvW+ozhaHhRkQ2P9kiU6DpbEXt9GNY2gGE5N/r0J W7DCEj8ZTtCsjgNsSw/iSYGHRg== X-Google-Smtp-Source: ABdhPJwy4qQMK/7UAde33BeHLG8SssWXwY3J3+Riai/cUIJefm7KNlCt9zg6kKem4V8euFbj3rR0BQ== X-Received: by 2002:a05:6a00:138f:b029:1b8:b9d5:3a2c with SMTP id t15-20020a056a00138fb02901b8b9d53a2cmr13919424pfg.10.1613821828235; Sat, 20 Feb 2021 03:50:28 -0800 (PST) Received: from leoy-ThinkPad-X240s (96.45.177.130.16clouds.com. [96.45.177.130]) by smtp.gmail.com with ESMTPSA id j4sm12309877pfa.131.2021.02.20.03.50.22 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 20 Feb 2021 03:50:27 -0800 (PST) Date: Sat, 20 Feb 2021 19:50:19 +0800 From: Leo Yan To: James Clark Subject: Re: [PATCH 2/7] perf cs-etm: Only search timestamp in current sample's queue. Message-ID: <20210220115019.GB3824@leoy-ThinkPad-X240s> References: <20210212144513.31765-1-james.clark@arm.com> <20210212144513.31765-3-james.clark@arm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20210212144513.31765-3-james.clark@arm.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210220_065031_129100_0555C996 X-CRM114-Status: GOOD ( 31.75 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Mark Rutland , branislav.rankov@arm.com, al.grant@arm.com, denik@chromium.org, Mathieu Poirier , suzuki.poulose@arm.com, Alexander Shishkin , Will Deacon , coresight@lists.linaro.org, John Garry , linux-kernel@vger.kernel.org, Namhyung Kim , Jiri Olsa , linux-arm-kernel@lists.infradead.org, Mike Leach Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Fri, Feb 12, 2021 at 04:45:08PM +0200, James Clark wrote: > Change initial timestamp search to only operate on the queue > related to the current event. In a later change the bounds > of the aux record will also be used to reset the decoder and > the record is only relevant to a single queue. I roughly understand this patch tries to establish the mechanism for timstamp search per CPU, but I am struggling to understand what's issue you try to address. So could you give more description for "what's current issue?" and "why need to use this way to fix the issue?". This would be very appreciated. > This change makes some files that had coresight data > but didn't syntesise any events start working and generating > events. I'm not sure of the reason for that. I'd expect this > change to only affect the ordering of events. This seems to me that this patch introduces regression. > Signed-off-by: James Clark > --- > tools/perf/util/cs-etm.c | 30 ++++++++++++++---------------- > 1 file changed, 14 insertions(+), 16 deletions(-) > > diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c > index 27894facae5e..8f8b448632fb 100644 > --- a/tools/perf/util/cs-etm.c > +++ b/tools/perf/util/cs-etm.c > @@ -97,7 +97,7 @@ struct cs_etm_queue { > /* RB tree for quick conversion between traceID and metadata pointers */ > static struct intlist *traceid_list; > > -static int cs_etm__update_queues(struct cs_etm_auxtrace *etm); > +static int cs_etm__update_queues(struct cs_etm_auxtrace *etm, int cpu); > static int cs_etm__process_queues(struct cs_etm_auxtrace *etm); > static int cs_etm__process_timeless_queues(struct cs_etm_auxtrace *etm, > pid_t tid); > @@ -524,7 +524,6 @@ static void cs_etm__dump_event(struct cs_etm_auxtrace *etm, > static int cs_etm__flush_events(struct perf_session *session, > struct perf_tool *tool) > { > - int ret; > struct cs_etm_auxtrace *etm = container_of(session->auxtrace, > struct cs_etm_auxtrace, > auxtrace); > @@ -534,11 +533,6 @@ static int cs_etm__flush_events(struct perf_session *session, > if (!tool->ordered_events) > return -EINVAL; > > - ret = cs_etm__update_queues(etm); > - > - if (ret < 0) > - return ret; > - When flush events, it means the trace data is discontinuous or at the end of trace data. If the trace data is discontinuous, we need to use cs_etm__update_queues() to create new queues. So if we remove the calling cs_etm__update_queues(), I suspect it cannot handle the discontinuous trace data anymore. > if (etm->timeless_decoding) > return cs_etm__process_timeless_queues(etm, -1); > > @@ -851,10 +845,7 @@ static int cs_etm__setup_queue(struct cs_etm_auxtrace *etm, > etmq->queue_nr = queue_nr; > etmq->offset = 0; > > - if (etm->timeless_decoding) > - return 0; > - else > - return cs_etm__search_first_timestamp(etmq); > + return 0; > } > > static int cs_etm__setup_queues(struct cs_etm_auxtrace *etm) > @@ -874,14 +865,20 @@ static int cs_etm__setup_queues(struct cs_etm_auxtrace *etm) > return 0; > } > > -static int cs_etm__update_queues(struct cs_etm_auxtrace *etm) > +static int cs_etm__update_queues(struct cs_etm_auxtrace *etm, int cpu) > { > + int ret; > if (etm->queues.new_data) { > etm->queues.new_data = false; > - return cs_etm__setup_queues(etm); > + ret = cs_etm__setup_queues(etm); Just remind, the new parameter "cpu" is introduced in this function, in theory, cs_etm__update_queues() should work for the specified CPU. But it always setup queues for all CPUs with calling cs_etm__setup_queues(). > + if (ret) > + return ret; > } > > - return 0; > + if (!etm->timeless_decoding) > + return cs_etm__search_first_timestamp(etm->queues.queue_array[cpu].priv); > + else > + return 0; In the original code of cs_etm__update_queues(), if there have no any new data (or has already setup queues), then it does nothing and directly bails out. After applied the up change, it will always search the first timestamp for the "cpu". > } > > static inline > @@ -2358,8 +2355,9 @@ static int cs_etm__process_event(struct perf_session *session, > else > timestamp = 0; > > - if (timestamp || etm->timeless_decoding) { > - err = cs_etm__update_queues(etm); > + if ((timestamp || etm->timeless_decoding) > + && event->header.type == PERF_RECORD_AUX) { > + err = cs_etm__update_queues(etm, sample->cpu); Here might cause potential issue. Let's see an example: If CoreSight uses N:1 model for tracers and sink (just like multiple ETMs output trace to the same ETR), when "sample->cpu" is 0, it will only initialize the timestamp for CPU 0. In the same AUX trace data, it can contains trace for CPU 1, 2, etc; for this case, the flow doesn't prepare the timestamp for CPU1/2, so it will fail to generate samples for CPU 1/2, right? Thanks, Leo > if (err) > return err; > } > -- > 2.28.0 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel