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=-16.0 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,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED,USER_AGENT_SANE_1 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 AC76EC433DB for ; Wed, 3 Feb 2021 23:37:44 +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 5961664F43 for ; Wed, 3 Feb 2021 23:37:44 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 5961664F43 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=arm.com 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-Type: Content-Transfer-Encoding:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:Date:Message-ID:From: References:To:Subject:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=scUpsBiedKsG7cvmL4I4qNglDt/EYM5MLX4mdDgK3eU=; b=bfY28ELfSmDbwuZDqgFr3nPnj pqTi62TfAE25ZWMxymagJSOp9lkwPPUPXJa8qkClWDwHry9Knza36La8VilUv93SuZnG+ebRD5Dol UnkpxD0DGXPDHlgZurm9kZivhvOt7T+3c7IH5xIXfJZeYt5s6THpVZFF5k+Mi8J5H7SZ9bUSTROz2 2zdOiZl+iTHcdQBcl8jIYIReMlVifwrF5xCJ+46OestghnwJhfMsd1Jx/oZDdIvvFNUHQtHy9iNfc tsSqYtooutadhHfhef5aOzIvsCluOxyFZp0QIE7XKniS/p9FLa/byM461RZw2yL26C8N09xOgzcB+ EsnzRluvQ==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1l7RhA-0008AA-Hu; Wed, 03 Feb 2021 23:36:24 +0000 Received: from foss.arm.com ([217.140.110.172]) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1l7Rh7-00089r-EM for linux-arm-kernel@lists.infradead.org; Wed, 03 Feb 2021 23:36:22 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id F06A1D6E; Wed, 3 Feb 2021 15:36:16 -0800 (PST) Received: from [10.57.47.183] (unknown [10.57.47.183]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id B82EE3F694; Wed, 3 Feb 2021 15:36:15 -0800 (PST) Subject: Re: [PATCH V3 07/14] coresight: etm-perf: Handle stale output handles To: Mathieu Poirier , Anshuman Khandual References: <1611737738-1493-1-git-send-email-anshuman.khandual@arm.com> <1611737738-1493-8-git-send-email-anshuman.khandual@arm.com> <20210203190513.GF1536093@xps15> From: Suzuki K Poulose Message-ID: Date: Wed, 3 Feb 2021 23:36:02 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.7.0 MIME-Version: 1.0 In-Reply-To: <20210203190513.GF1536093@xps15> Content-Language: en-GB X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210203_183621_699361_F25D71A0 X-CRM114-Status: GOOD ( 31.26 ) 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: coresight@lists.linaro.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Leo Yan , lcherian@marvell.com, mike.leach@linaro.org Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 2/3/21 7:05 PM, Mathieu Poirier wrote: > On Wed, Jan 27, 2021 at 02:25:31PM +0530, Anshuman Khandual wrote: >> From: Suzuki K Poulose >> >> The context associated with an ETM for a given perf event >> includes : >> - handle -> the perf output handle for the AUX buffer. >> - the path for the trace components >> - the buffer config for the sink. >> >> The path and the buffer config are part of the "aux_priv" data >> (etm_event_data) setup by the setup_aux() callback, and made available >> via perf_get_aux(handle). >> >> Now with a sink supporting IRQ, the sink could "end" an output >> handle when the buffer reaches the programmed limit and would try >> to restart a handle. This could fail if there is not enough >> space left the AUX buffer (e.g, the userspace has not consumed >> the data). This leaves the "handle" disconnected from the "event" >> and also the "perf_get_aux()" cleared. This all happens within >> the sink driver, without the etm_perf driver being aware. >> Now when the event is actually stopped, etm_event_stop() >> will need to access the "event_data". But since the handle >> is not valid anymore, we loose the information to stop the >> "trace" path. So, we need a reliable way to access the etm_event_data >> even when the handle may not be active. >> >> This patch replaces the per_cpu handle array with a per_cpu context >> for the ETM, which tracks the "handle" as well as the "etm_event_data". >> The context notes the etm_event_data at etm_event_start() and clears >> it at etm_event_stop(). This makes sure that we don't access a >> stale "etm_event_data" as we are guaranteed that it is not >> freed by free_aux() as long as the event is active and tracing, >> also provides us with access to the critical information >> needed to wind up a session even in the absence of an active >> output_handle. >> >> This is not an issue for the legacy sinks as none of them supports >> an IRQ and is centrally handled by the etm-perf. >> >> Cc: Mathieu Poirier >> Cc: Anshuman Khandual >> Cc: Leo Yan >> Cc: Mike Leach >> Signed-off-by: Suzuki K Poulose >> Signed-off-by: Anshuman Khandual >> --- >> drivers/hwtracing/coresight/coresight-etm-perf.c | 45 +++++++++++++++++++++--- >> 1 file changed, 40 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c >> index eb9e7e9..a3977b0 100644 >> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c >> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c >> @@ -24,7 +24,26 @@ >> static struct pmu etm_pmu; >> static bool etm_perf_up; >> >> -static DEFINE_PER_CPU(struct perf_output_handle, ctx_handle); >> +/* >> + * An ETM context for a running event includes the perf aux handle >> + * and aux_data. For ETM, the aux_data (etm_event_data), consists of >> + * the trace path and the sink configuration. The event data is accessible >> + * via perf_get_aux(handle). However, a sink could "end" a perf output >> + * handle via the IRQ handler. And if the "sink" encounters a failure >> + * to "begin" another session (e.g due to lack of space in the buffer), >> + * the handle will be cleared. Thus, the event_data may not be accessible >> + * from the handle when we get to the etm_event_stop(), which is required >> + * for stopping the trace path. The event_data is guaranteed to stay alive >> + * until "free_aux()", which cannot happen as long as the event is active on >> + * the ETM. Thus the event_data for the session must be part of the ETM context >> + * to make sure we can disable the trace path. >> + */ >> +struct etm_ctxt { >> + struct perf_output_handle handle; >> + struct etm_event_data *event_data; >> +}; >> + >> +static DEFINE_PER_CPU(struct etm_ctxt, etm_ctxt); >> static DEFINE_PER_CPU(struct coresight_device *, csdev_src); >> >> /* ETMv3.5/PTM's ETMCR is 'config' */ >> @@ -332,7 +351,8 @@ static void etm_event_start(struct perf_event *event, int flags) >> { >> int cpu = smp_processor_id(); >> struct etm_event_data *event_data; >> - struct perf_output_handle *handle = this_cpu_ptr(&ctx_handle); >> + struct etm_ctxt *ctxt = this_cpu_ptr(&etm_ctxt); >> + struct perf_output_handle *handle = &ctxt->handle; >> struct coresight_device *sink, *csdev = per_cpu(csdev_src, cpu); >> struct list_head *path; >> > if (!csdev) > goto fail; > > /* > * Something went wrong if an event data is already associated > * with a context. > */ > if (WARN_ONE(ctxt->event_data)) > goto fail; > >> @@ -374,6 +394,8 @@ static void etm_event_start(struct perf_event *event, int flags) >> if (source_ops(csdev)->enable(csdev, event, CS_MODE_PERF)) >> goto fail_disable_path; >> >> + /* Save the event_data for this ETM */ >> + ctxt->event_data = event_data; >> out: >> return; >> >> @@ -392,13 +414,20 @@ static void etm_event_stop(struct perf_event *event, int mode) >> int cpu = smp_processor_id(); >> unsigned long size; >> struct coresight_device *sink, *csdev = per_cpu(csdev_src, cpu); >> - struct perf_output_handle *handle = this_cpu_ptr(&ctx_handle); >> - struct etm_event_data *event_data = perf_get_aux(handle); >> + struct etm_ctxt *ctxt = this_cpu_ptr(&etm_ctxt); >> + struct perf_output_handle *handle = &ctxt->handle; > > struct etm_event_data *event_data = perf_get_aux(handle); >> struct list_head *path; >> > > if (WARN_ON(event_data && event_data != ctxt->event_data)) > return; > > event_data = ctxt->event_data; > /* Clear the event_data as this ETM is stopping the trace. */ > ctxt->event_data = NULL; > > With the above: I have folded the following changes in : diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c index cf0b0c2d5eed..9f85bf28a82f 100644 --- a/drivers/hwtracing/coresight/coresight-etm-perf.c +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c @@ -375,6 +375,10 @@ static void etm_event_start(struct perf_event *event, int flags) if (!csdev) goto fail; + /* Have we messed up our tracking ? */ + if (WARN_ON(ctxt->event_data)) + goto fail; + /* * Deal with the ring buffer API and get a handle on the * session's information. @@ -432,11 +436,21 @@ static void etm_event_stop(struct perf_event *event, int mode) struct coresight_device *sink, *csdev = per_cpu(csdev_src, cpu); struct etm_ctxt *ctxt = this_cpu_ptr(&etm_ctxt); struct perf_output_handle *handle = &ctxt->handle; - struct etm_event_data *event_data = ctxt->event_data; + struct etm_event_data *event_data; struct list_head *path; + /* + * If we still have access to the event_data via handle, + * confirm that we haven't messed up the tracking. + */ + if (handle->event && + WARN_ON(perf_get_aux(handle) != ctxt->event_data)) + return; + + event_data = ctxt->event_data; /* Clear the event_data as this ETM is stopping the trace. */ ctxt->event_data = NULL; + if (event->hw.state == PERF_HES_STOPPED) return; > > Reviewed-by: Mathieu Poirier Thanks Suzuki _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel