Hi Mathieu,
On 27/03/2019 17:01, Mathieu Poirier wrote:
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.
Ok. Thanks for clarifying this. So we have one AUX buffer per event, but they all could end up in the same ETR HW buffer and may be copied to only one of the AUX buffer, which happens to really disable the ETR. And thus we need to have a sufficiently large AUX buffer in place to allow we don't overflow all the time with trace from multiple events. Makes sense for the N:1 topology. Also the decoding phase must extract the trace to the corresponding event based on the TRACEDI of the packets.
Thats the best we could do at the moment. If there was a way to have one single large AUX buffer for a set of events, which is easily accessible from a given event in the set, rather than "N" large buffers which could potentially impact memory consumption.
It would be good to have this clearly documented somewhere in the implementation to keep us in track.
Cheers Suzuki
Thanks, Mathieu
Cheers Suzuki