Re: [PATCH v2 13/16] coresight: tmc-etr: Allow events to use the same ETR buffer

[Mathieu Poirier <mathieu.poirier@linaro.org>]

On Wed, 27 Mar 2019 at 05:30, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
>
> On 03/26/2019 05:55 PM, Mathieu Poirier wrote:
> > On Tue, Mar 26, 2019 at 04:18:35PM +0000, Suzuki K Poulose wrote:
> >> On 03/25/2019 09:56 PM, Mathieu Poirier wrote:
> >>> This patch uses the pid of the process being traced to aggregate traces
> >>> coming from different processors in the same sink, something that is
> >>> required when collecting traces in CPU-wide mode when the CoreSight HW
> >>> enacts a N:1 source/sink topology.
> >>>
> >>> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> >>> ---
> >>>    .../hwtracing/coresight/coresight-tmc-etr.c   | 71 +++++++++++++++++--
> >>>    1 file changed, 65 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> >>> index 7254fafdf1c2..cbabf88bd51d 100644
> >>> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
> >>> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> >>> @@ -8,6 +8,7 @@
> >>>    #include <linux/coresight.h>
> >>>    #include <linux/dma-mapping.h>
> >>>    #include <linux/iommu.h>
> >>> +#include <linux/idr.h>
> >>>    #include <linux/slab.h>
> >>>    #include <linux/types.h>
> >>>    #include <linux/vmalloc.h>
> >>> @@ -41,6 +42,9 @@ struct etr_perf_buffer {
> >>>     void                    **pages;
> >>>    };
> >>> +static DEFINE_IDR(session_idr);
> >>> +static DEFINE_MUTEX(session_idr_lock);
> >>
> >> Please correct me if I am wrong here. What we now do with this series is
> >>
> >> - One event per CPU and thus one ETR perf buf per CPU.
> >> - One *ETR buf* per PID, from the IDR hash list.
> >>
> >> However, if we have 1:1 situation, we will have :
> >>
> >> N (say 2 ETRs), but one *ETR buf* as they all share the same PID, implying
> >> multiple multiple ETRs will try to use the same etr buf,
> >> which could corrupt the trace data ? Instead,  what we need in that
> >> situation is :
> >>
> >> One ETR buf perf PID+ETR device. i.e, etr_bufs must be per ETR.
> >> So should the IDR be specific to an ETR ?
> >>
> >> Or do we not support a session with multiple ETRs involved (1:1) ?
> >
> > At this time 1:1 topologies are not supported and a fair amount of work will go
> > in doing so.  When I started working on this serie my thought was that because
> > of said amount of work there is no point thinking about 1:1, and that we can
> > deal with it when we get there.
> >
> > But taking a step back and having dealt with the harder (concurrency) problems
> > inherent to CPU-wide scenarios, it would be trivial to make the code 1:1 ready
> > and it will be one less thing to worry about down the road.
> >
> > That being said and answering your question above, I think we need and IDR per
> > ETR (in the drvdata) to avoid contention issues on systems with several ETRs.
> >
> > Thanks for bringing this back to my attention.
>
> Cool. Thanks for explaining the rationale. So, when we do that, I think
> we may be able to have one "etr_perf_buffer" per ETR and thus we may be
> able to move the refcount back to the etr_perf_buffer, just like we
> moved the PID and index etr_perf_buffer instead of the etr_buf ?

An etr_perf_buffer is associated with an event and holds the AUX ring
buffer that was created for that event.  In CPU-wide N:1 mode multiple
events (one per CPU), each with its own AUX ring buffer, share a sink
and as such we can't have a single etr_perf_buffer per ETR.

Thanks,
Mathieu

>
> Cheers
> Suzuki
>

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