Re: [PATCH 2/4] coresight: tmc-etr: Track perf handler.

[Leo Yan <leo.yan@linaro.org>]

Hi Daniel,

On Wed, Apr 21, 2021 at 02:04:11PM +0200, Daniel Kiss wrote:

[...]

> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> index dd19d1d1c3b38..bf9f6311d8663 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> @@ -1511,6 +1511,12 @@ tmc_update_etr_buffer(struct coresight_device *csdev,
>  		goto out;
>  	}
>  
> +	/* Serve only the tracer with the right handler */
> +	if (drvdata->perf_handle != handle) {
> +		spin_unlock_irqrestore(&drvdata->spinlock, flags);
> +		goto out;
> +	}
> +

I have concern for this change, Let's use the system-wide tracing as
an example.

If a system have 4 CPUs, for the perf with system wide tracing, the
tool maps the AUX ring buffers for four times, but the CoreSight
driver only allocates pages once and maps these physical pages for
four times to user space.  Therefore, the perf tool in the userspace
manages 4 AUX ring buffers, every AUX ring buffer is served for one
CPU.

The confusion between the CoreSight driver (in the kernel) and the
perf tool (in the userspace) is: there actually has only one ring
buffer for the enabled sink (let's say ETR), but there have four ring
buffer control structures, the control structure is
'perf_event_mmap_page' which is resident in the first page for perf's
general ring buffer (please note, this ring buffer is different from
AUX ring buffer).

IIUC, this patch only allows the first CPU which enables coresight path
to update the AUX ring buffer.  This can break the case:

  - Step 1: perf tool opens ETM event; we can use the command:

    # perf record -o ${perfdata} -e cs_etm/@tmc_etr0/ -a
           -- dd if=/dev/zero of=/dev/null

  - Step 2: the profiled program "dd" is firstly schedued in CPU0, so
    its "perf_handle" will be assigned to "drvdata->perf_handle";

  - Step 3: if the program "dd" is migrated to CPU1 and it never runs
    on CPU0 afterwards, then this patch will prevent to update the AUX
    ring buffer, due to the "drvdata->perf_handle" cannot match with
    CPU1's handler.

On the other hand, I think we should change to always stick to the
same "perf_output_handle" for all CPUs, thus it can allow all CPUs
to use the same structure 'perf_event_mmap_page' for AUX ring buffer
management.

[...]

Thanks,
Leo

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel