Re: [PATCH 1/5] coresight: Fix buffer size in snapshot mode

From: Mathieu Poirier <mathieu.poirier@linaro.org>
To: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: Jon Corbet <corbet@lwn.net>,
	Alexander Shishkin <alexander.shishkin@linux.intel.com>,
	Coresight ML <coresight@lists.linaro.org>,
	Arnaldo Carvalho de Melo <acme@kernel.org>,
	Peter Zijlstra <peterz@infradead.org>,
	Ingo Molnar <mingo@redhat.com>, Mike Leach <Mike.leach@arm.com>,
	Leo Yan <leo.yan@linaro.org>,
	linux-arm-kernel <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH 1/5] coresight: Fix buffer size in snapshot mode
Date: Tue, 7 May 2019 14:22:16 -0600	[thread overview]
Message-ID: <CANLsYkxzFcGJZm5Vd2x=467+6t1Wm6YqHGMd3sQMh-Eak-nFRg@mail.gmail.com> (raw)
In-Reply-To: <808f4985-bbc8-6473-52c2-31b3743199bb@arm.com>

Hey Suzuki,

On Tue, 7 May 2019 at 02:50, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
>
> Hi Mathieu,
>
> On 01/05/2019 18:50, Mathieu Poirier wrote:
> > In snapshot mode the buffer used by the sink devices need to be
> > equal to the ring buffer size in order for the user space mechanic
> > to work properly.
>
> The patch as such looks good to me. However I don't understand the
> need for it for ETB and ETF. We can't use the AUX buf directly anyway
> for these devices. We could always pretend that there was no overflow and
> simply copy it to the AUX buf. The decoder would know the end of trace packets.
> What am I missing here ?

The problem here is to figure out how to recognised a buffer wrap has
occurred in function cs_etm_find_snapshot() and how to compute the
value of '*old'.  But looking at patch 4/5 again I came up with a
better way to proceed, one that should remove the need for this patch.
I will send another revision.

Mathieu

>
> >
> > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> > ---
> >   drivers/hwtracing/coresight/coresight-etb10.c | 23 +++++++++++++++++++
> >   .../hwtracing/coresight/coresight-tmc-etf.c   | 20 ++++++++++++++++
> >   .../hwtracing/coresight/coresight-tmc-etr.c   |  8 +++++--
> >   3 files changed, 49 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/hwtracing/coresight/coresight-etb10.c b/drivers/hwtracing/coresight/coresight-etb10.c
> > index 4ee4c80a4354..0764647b92bc 100644
> > --- a/drivers/hwtracing/coresight/coresight-etb10.c
> > +++ b/drivers/hwtracing/coresight/coresight-etb10.c
> > @@ -374,7 +374,30 @@ static void *etb_alloc_buffer(struct coresight_device *csdev,
> >                             int nr_pages, bool overwrite)
> >   {
> >       int node, cpu = event->cpu;
> > +     u32 capacity;
> >       struct cs_buffers *buf;
> > +     struct etb_drvdata *drvdata;
> > +
> > +     /*
> > +      * In snapsot mode the size of the perf ring buffer needs to be equal
> > +      * to the size of the device's internal memory if we want to reuse the
> > +      * generic AUX buffer management mechanic.
> > +      *
> > +      * For example (assuming 4096 byte page size):
> > +      *
> > +      *    # cat /sys/bus/coresight/devices/20010000.etb/mgmt/rdp
> > +      *    0x2000
> > +      *    # perf record -e cs_etm/@20010000.etf/ -S -m,8 --per-thread $APP
> > +      *
> > +      */
>
> If at all we need to do this, Is there a way to force the perf to use the
> appropriate size for AUX buf depending on the sink ? I would prefer that instead
> of finding this magic number and calculating it based on the page_size. If we
> could not do that easily, it may be a good idea to expose something like,
> "buffer_pages" under sysfs to help the user a bit.
>
> Suzuki

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