Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 3 Feb 2021 12:05:13 -0700
From:   Mathieu Poirier <mathieu.poirier@linaro.org>
To:     Anshuman Khandual <anshuman.khandual@arm.com>
Cc:     linux-arm-kernel@lists.infradead.org, coresight@lists.linaro.org,
        suzuki.poulose@arm.com, mike.leach@linaro.org,
        lcherian@marvell.com, linux-kernel@vger.kernel.org,
        Leo Yan <leo.yan@linaro.org>
Subject: Re: [PATCH V3 07/14] coresight: etm-perf: Handle stale output handles
Message-ID: <20210203190513.GF1536093@xps15>
References: <1611737738-1493-1-git-send-email-anshuman.khandual@arm.com>
 <1611737738-1493-8-git-send-email-anshuman.khandual@arm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1611737738-1493-8-git-send-email-anshuman.khandual@arm.com>
Precedence: bulk

On Wed, Jan 27, 2021 at 02:25:31PM +0530, Anshuman Khandual wrote:
> From: Suzuki K Poulose <suzuki.poulose@arm.com>
> 
> 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 <mathieu.poirier@linaro.org>
> Cc: Anshuman Khandual <anshuman.khandual@arm.com>
> Cc: Leo Yan <leo.yan@linaro.org>
> Cc: Mike Leach <mike.leach@linaro.org>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
>  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:

Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
  
>  	if (event->hw.state == PERF_HES_STOPPED)
>  		return;
>  
> +	/* We must have a valid event_data for a running event */
> +	if (WARN_ON(!event_data))
> +		return;
> +
>  	if (!csdev)
>  		return;
>  
> @@ -416,7 +445,13 @@ static void etm_event_stop(struct perf_event *event, int mode)
>  	/* tell the core */
>  	event->hw.state = PERF_HES_STOPPED;
>  
> -	if (mode & PERF_EF_UPDATE) {
> +	/*
> +	 * If the handle is not bound to an event anymore
> +	 * (e.g, the sink driver was unable to restart the
> +	 * handle due to lack of buffer space), we don't
> +	 * have to do anything here.
> +	 */
> +	if (handle->event && (mode & PERF_EF_UPDATE)) {
>  		if (WARN_ON_ONCE(handle->event != event))
>  			return;
>  
> -- 
> 2.7.4
>