diff mbox series

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

Message ID 20210421120413.3110775-3-daniel.kiss@arm.com (mailing list archive)
State New, archived
Headers show
Series coresight: Add ETR-PERF polling. | expand

Commit Message

Daniel Kiss April 21, 2021, 12:04 p.m. UTC 
Keep track of the perf handler that is registred by the first tracer.
This will be used by the update call from polling.

Signed-off-by: Daniel Kiss <daniel.kiss@arm.com>
Signed-off-by: Branislav Rankov <Branislav.Rankov@arm.com>
---
 drivers/hwtracing/coresight/coresight-tmc-etr.c | 8 ++++++++
 drivers/hwtracing/coresight/coresight-tmc.h     | 2 ++
 2 files changed, 10 insertions(+)

Comments

Leo Yan April 23, 2021, 9:20 a.m. UTC | #1 
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
Leo Yan April 26, 2021, 12:25 a.m. UTC | #2 
On Fri, Apr 23, 2021 at 05:20:38PM +0800, Leo Yan wrote:
> 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.

Want to clarify, this case only happens with "snapshot" mode; With
Mathieu's reminding, "snapshot" mode is quite special: it creates AUX
ring buffer per CPU, but when enable the tracing, if without
specifying the option "-a" for system wide tracing, it only enables
ETM tracer for a CPU when the profiled program is scheduled on that CPU.

To avoid over complexsity, let's give this low priority and firstly
focus on the system-wide tracing case.

Thanks,
Leo

> 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
diff mbox series

Patch

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;
+	}
+
 	if (WARN_ON(drvdata->perf_buf != etr_buf)) {
 		lost = true;
 		spin_unlock_irqrestore(&drvdata->spinlock, flags);
@@ -1623,6 +1629,7 @@  static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, void *data)
 		drvdata->pid = pid;
 		drvdata->mode = CS_MODE_PERF;
 		drvdata->perf_buf = etr_perf->etr_buf;
+		drvdata->perf_handle = handle;
 		atomic_inc(csdev->refcnt);
 	}
 
@@ -1670,6 +1677,7 @@  static int tmc_disable_etr_sink(struct coresight_device *csdev)
 	drvdata->mode = CS_MODE_DISABLED;
 	/* Reset perf specific data */
 	drvdata->perf_buf = NULL;
+	drvdata->perf_handle = NULL;
 
 	spin_unlock_irqrestore(&drvdata->spinlock, flags);
 
diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h
index b91ec7dde7bc9..81583ffb973dc 100644
--- a/drivers/hwtracing/coresight/coresight-tmc.h
+++ b/drivers/hwtracing/coresight/coresight-tmc.h
@@ -184,6 +184,7 @@  struct etr_buf {
  * @idr_mutex:	Access serialisation for idr.
  * @sysfs_buf:	SYSFS buffer for ETR.
  * @perf_buf:	PERF buffer for ETR.
+ * @perf_handle: PERF handle for ETR.
  */
 struct tmc_drvdata {
 	void __iomem		*base;
@@ -207,6 +208,7 @@  struct tmc_drvdata {
 	struct mutex		idr_mutex;
 	struct etr_buf		*sysfs_buf;
 	struct etr_buf		*perf_buf;
+	struct perf_output_handle *perf_handle;
 };
 
 struct etr_buf_operations {