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=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 BBF91C433FE for ; Fri, 11 Dec 2020 20:33:26 +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 1424F23C18 for ; Fri, 11 Dec 2020 20:33:25 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 1424F23C18 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=5gZzJUlALUNvNjztaSeqdsLC5s/KUgqJZpMMcwZcV8w=; b=0GN7anEWnsEhCzirIiaaPIf4s yNRjq0+DSm4SPo7b1FN2Ixolla718/T9ohb0FFan+YHwSy0+ZuroOKp0B6a2i1t+lepaiih3Ne2dO zCZKfa6C5p25x9+9KtODtKKEc/RoJ4Sk0zmmYDAkrKoZNmZFnOongInQwNP2Aepl6dYqPKWk0gbJK 4HJxUr8L73l7CNoNZJgktUW+1kJfyG2uG6iHDbEv+98G306x9WknyJOnnGiFZRGYYGCviWhyyxr0O gBa0IynVmth4lkGpYe5U7vshEL0b2yBUkGyBr6Ud2mzmBm7nBbzYeLovq2N2MvKYSgBZseN6hxGcB 97mSqgK9w==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1knp4g-0002Ik-Fh; Fri, 11 Dec 2020 20:31:34 +0000 Received: from mail-pg1-x542.google.com ([2607:f8b0:4864:20::542]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1knp4d-0002Hl-J0 for linux-arm-kernel@lists.infradead.org; Fri, 11 Dec 2020 20:31:32 +0000 Received: by mail-pg1-x542.google.com with SMTP id x24so2068674pgf.0 for ; Fri, 11 Dec 2020 12:31:30 -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=hT2zG6CB4UvRgVp2IbTFwUn+l/bry/ipSKxWwb+b3ns=; b=Oic1QPQjgU2g/fqxeor2skzkpdjzE9SQq9F2LkamHoM1EieAZf9+ljH+rd449lcNj6 NDzp3oMuGR+rOEzDLuLY1fjxPf2/LA71n1VhdVUEAqX+NliZfmCBh5C+q3DeuE5LXIJT 0Yq4aUz0W+DF88k7+WydIfhp65BtALc6fQ2PZGwyXIOwsQllMQmzZqzPyMs1/oy69VWL FhFW+Shsi0MNF4tsCMr2pPI4wivt/oJh0ux43aQ9fcsKYcn6rDA/W8SCepZAZ9yQUfLh nkAzUZYHLkWeAz4BDETjw/ThRxR6gD+V1n97o/EyAPROeMrPlbxnHUc6sw+YPec8lXwh UZNg== 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=hT2zG6CB4UvRgVp2IbTFwUn+l/bry/ipSKxWwb+b3ns=; b=qX1wJkIFTx7lezSUZdwbRR6o0eWBJOcS2KXz2jWlUTuMWhtM0h4OSPZ0BhoqCiFy5M xOgaeIhl1b4jaGvF/9HAg4qtMffVO5T1vBAdCKX6HEmHY45nBXBUQCa2X/0KorfejjBk 8DAhrNW0WEVTDKpO5lAaGo/EoDPwyd3YZMZy4i5sO1A2H8LjNMwdiM8/qqM1rsgs4JHC X5AJZwC7eHR4N4FurBe/a8tVXie+wT9lSRtCG4TIXROCibZ7cjJVn+KbSto2NHSuJrLC HVryzwyt8f21cV7FSWftBJ7PgjgIxxnCHoWDPtZp6/HriSd9zrpOpSWdXa5Q2YEJUXjq DAzw== X-Gm-Message-State: AOAM530GPjs1yKMrl9f/asz87Jezbahv7zdIuSlacKe1oftxd4TD65+n Les0YBnS0wlu9iyIqolCiU3SYw== X-Google-Smtp-Source: ABdhPJwNU+uAow8T+7jNNd1YUfOcjMrHuJF1JJUMaDMvPdGBEpl+crSzKXRnjjsyVXa1Dh4J7fK43g== X-Received: by 2002:a63:7f03:: with SMTP id a3mr13299524pgd.313.1607718689333; Fri, 11 Dec 2020 12:31:29 -0800 (PST) Received: from xps15 (S0106889e681aac74.cg.shawcable.net. [68.147.0.187]) by smtp.gmail.com with ESMTPSA id p185sm8588444pfb.165.2020.12.11.12.31.28 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 11 Dec 2020 12:31:28 -0800 (PST) Date: Fri, 11 Dec 2020 13:31:26 -0700 From: Mathieu Poirier To: Suzuki K Poulose Subject: Re: [RFC 09/11] coresight: etm-perf: Disable the path before capturing the trace data Message-ID: <20201211203126.GA1921322@xps15> References: <1605012309-24812-1-git-send-email-anshuman.khandual@arm.com> <1605012309-24812-10-git-send-email-anshuman.khandual@arm.com> <2019e06d-65e1-fee8-f75d-bfa5750d2458@arm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <2019e06d-65e1-fee8-f75d-bfa5750d2458@arm.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20201211_153131_695077_5EA2DECB X-CRM114-Status: GOOD ( 37.14 ) 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: Al Grant , Anshuman Khandual , coresight@lists.linaro.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, mike.leach@linaro.org 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, Nov 27, 2020 at 10:32:28AM +0000, Suzuki K Poulose wrote: > On 11/10/20 12:45 PM, Anshuman Khandual wrote: > > perf handle structure needs to be shared with the TRBE IRQ handler for > > capturing trace data and restarting the handle. There is a probability > > of an undefined reference based crash when etm event is being stopped > > while a TRBE IRQ also getting processed. This happens due the release > > of perf handle via perf_aux_output_end(). This stops the sinks via the > > link before releasing the handle, which will ensure that a simultaneous > > TRBE IRQ could not happen. > > Or in other words : > > We now have : > > update_buffer() > > perf_aux_output_end(handle) > > ... > disable_path() > > This is problematic due to various reasons : > > 1) The semantics of update_buffer() is not clear. i.e, whether it > should leave the "sink" "stopped" or "disabled" or "active" I'm a little confused by the above as the modes that apply here are CS_MODE_DISABLED and CS_MODE_PERF, so I'll go with those. Let me know if you meant something else. So far ->update_buffer() doesn't touch drvdata->mode and as such it is still set to CS_MODE_PERF when the update has completed. > > 2) This breaks the recommended trace collection sequence of > "flush" and "stop" from source to the sink for trace collection. > i.e, we stop the source now. But don't flush the components > from source to sink, rather we stop and flush from the sink. > And we flush and stop the path after we have collected the > trace data at sink, which is pointless. The above assesment is correct. Fixing it though has far reaching ramifications that go far beyond the scope of this patch. > > 3) For a sink with IRQ handler, if we don't stop the sink with > update_buffer(), we could have a situation : > > update_buffer() > > perf_aux_outpuf_end(handle) # handle is invalid now > > -----------------> IRQ -> irq_handler() > perf_aux_output_end(handle) # Wrong ! > > > disable_path() That's the picture of the issue I had in my head when looking at the code - I'm glad we came to the same conclusion. > > The sysfs mode is fine, as we defer the trace collection to disable_path(). > > The proposed patch is still racy, as we could still hit the problem. > > So, to avoid all of these situations, I think we should defer the the > update_buffer() to sink_ops->disable(), when we have flushed and stopped > the all the components upstream and avoid any races with the IRQ > handler. > > i.e, > > source_ops->stop(csdev); > > disable_path(handle); // similar to the enable_path > > > sink_ops->disable(csdev, handle) > { > /* flush & stop */ > > /* collect trace */ > perf_aux_output_end(handle, size); > } That is one solution. The advantage here is that it takes care of the flusing problem you described above. On the flip side it is moving a lot of code around, something that is better to do in another set. Another solution is to disable the TRBE IRQ in ->udpate_buffer(). The ETR does the same kind of thing with tmc_flush_and_stop(). I don't know how feasible that is but it would be a simple solution for this set. Properly flushing the pipeline could be done later. I'm fine with either approach. Thanks, Mathieu > > > Kind regards > Suzuki > > > > > > > Signed-off-by: Anshuman Khandual > > --- > > This might cause problem with traditional sink devices which can be > > operated in both sysfs and perf mode. This needs to be addressed > > correctly. One option would be to move the update_buffer callback > > into the respective sink devices. e.g, disable(). > > > > drivers/hwtracing/coresight/coresight-etm-perf.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c > > index 534e205..1a37991 100644 > > --- a/drivers/hwtracing/coresight/coresight-etm-perf.c > > +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c > > @@ -429,7 +429,9 @@ static void etm_event_stop(struct perf_event *event, int mode) > > size = sink_ops(sink)->update_buffer(sink, handle, > > event_data->snk_config); > > + coresight_disable_path(path); > > perf_aux_output_end(handle, size); > > + return; > > } > > /* Disabling the path make its elements available to other sessions */ > > > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel