Messages in this thread | | | From | Mathieu Poirier <> | Date | Wed, 14 Aug 2019 17:00:30 -0600 | Subject | Re: [PATCH v2] coresight: tmc-etr: Fix perf_data check. |
| |
Hi Yabin,
On Mon, 12 Aug 2019 at 13:03, Yabin Cui <yabinc@google.com> wrote: > > When tracing etm data of multiple threads on multiple cpus through > perf interface, each cpu has a unique etr_perf_buffer while sharing > the same etr device. There is no guarantee that the last cpu starts > etm tracing also stops last. This makes perf_data check fail.
Did you actually see the check fail or is this a theoretical thing? I'm really perplex here has I have tested this scenario many times without issues.
> > Fix it by checking etr_buf instead of etr_perf_buffer. > > Fixes: 3147da92a8a8 ("coresight: tmc-etr: Allocate and free ETR memory buffers for CPU-wide scenarios") > Signed-off-by: Yabin Cui <yabinc@google.com> > --- > > v1 -> v2: rename perf_data to perf_buf. Add fixes tag. > > --- > drivers/hwtracing/coresight/coresight-tmc-etr.c | 6 +++--- > drivers/hwtracing/coresight/coresight-tmc.h | 6 +++--- > 2 files changed, 6 insertions(+), 6 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c > index 17006705287a..90d1548ad268 100644 > --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c > +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c > @@ -1484,7 +1484,7 @@ tmc_update_etr_buffer(struct coresight_device *csdev, > goto out; > } > > - if (WARN_ON(drvdata->perf_data != etr_perf)) { > + if (WARN_ON(drvdata->perf_buf != etr_buf)) {
In CPU wide scenarios each perf event (one per CPU) is associated with an event_data during the setup process. The event_data is the etr_perf holding a reference to the perf ring buffer for that specific event along with the etr_buf, regardless of who created the latter. From there, when the event is installed on a CPU, the csdev for that CPU is given a reference to the event_data of that event[1]. Before going further notice how there is a per CPU csdev and event handle to keep track of event specifics[2]. As such both (per CPU) csdev and event handle carry the exact same reference to the etr_perf.
When an event is stopped the exact same per CPU references are taken[3] and later fed to sink->update_buffer(). If sink->update_buffer() is called on an event (again one per CPU) function etm_event_start() must have been called and (drvdata->perf_data == etf_perf) needs to hold.
If that is not the case, an event from a completely different trace session has been installed on that CPU and that can't happen thanks to drvdata->id.
As such if you get a condition where (drvdata->perf_buf != etr_buf), one of two things has happened:
1) Something went seriously wrong and the WARN_ON() did its job. 2) A corner case is manifesting in your test environment that I haven't provisioned for. If that is the case we need to figure out exactly what is happening before considering a fix.
Thanks, Mathieu
[1]. https://elixir.bootlin.com/linux/v5.3-rc4/source/drivers/hwtracing/coresight/coresight-tmc-etr.c#L1559 [2]. https://elixir.bootlin.com/linux/v5.3-rc4/source/drivers/hwtracing/coresight/coresight-etm-perf.c#L298 [3]. https://elixir.bootlin.com/linux/v5.3-rc4/source/drivers/hwtracing/coresight/coresight-etm-perf.c#L348
> lost = true; > spin_unlock_irqrestore(&drvdata->spinlock, flags); > goto out; > @@ -1497,7 +1497,7 @@ tmc_update_etr_buffer(struct coresight_device *csdev, > > CS_LOCK(drvdata->base); > /* Reset perf specific data */ > - drvdata->perf_data = NULL; > + drvdata->perf_buf = NULL; > spin_unlock_irqrestore(&drvdata->spinlock, flags); > > size = etr_buf->len; > @@ -1556,7 +1556,7 @@ static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, void *data) > } > > etr_perf->head = PERF_IDX2OFF(handle->head, etr_perf); > - drvdata->perf_data = etr_perf; > + drvdata->perf_buf = etr_perf->etr_buf; > > /* > * No HW configuration is needed if the sink is already in > diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h > index 1ed50411cc3c..f9a0c95e9ba2 100644 > --- a/drivers/hwtracing/coresight/coresight-tmc.h > +++ b/drivers/hwtracing/coresight/coresight-tmc.h > @@ -178,8 +178,8 @@ struct etr_buf { > * device configuration register (DEVID) > * @idr: Holds etr_bufs allocated for this ETR. > * @idr_mutex: Access serialisation for idr. > - * @perf_data: PERF buffer for ETR. > - * @sysfs_data: SYSFS buffer for ETR. > + * @sysfs_buf: SYSFS buffer for ETR. > + * @perf_buf: PERF buffer for ETR. > */ > struct tmc_drvdata { > void __iomem *base; > @@ -202,7 +202,7 @@ struct tmc_drvdata { > struct idr idr; > struct mutex idr_mutex; > struct etr_buf *sysfs_buf; > - void *perf_data; > + struct etr_buf *perf_buf; > }; > > struct etr_buf_operations { > -- > 2.23.0.rc1.153.gdeed80330f-goog >
| |