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

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

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.
Mathieu

> 
> Cheers
> Suzuki

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