[1/5] coresight: Fix buffer size in snapshot mode
diff mbox series

Message ID 20190501175052.29667-2-mathieu.poirier@linaro.org
State New
Headers show
Series
  • coresight: Fix snapshot mode
Related show

Commit Message

Mathieu Poirier May 1, 2019, 5:50 p.m. UTC
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.

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(-)

Comments

Leo Yan May 7, 2019, 7:38 a.m. UTC | #1
On Wed, May 01, 2019 at 11:50:48AM -0600, 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.
> 
> 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):

Here is delibrately to write as '4096 byte'?  Though I think should be
'4096 bytes' but I am not confident which is right ...

> +	 *
> +	 *    # cat /sys/bus/coresight/devices/20010000.etb/mgmt/rdp
> +	 *    0x2000
> +	 *    # perf record -e cs_etm/@20010000.etf/ -S -m,8 --per-thread $APP
> +	 *
> +	 */
> +	drvdata = dev_get_drvdata(csdev->dev.parent);
> +	capacity = drvdata->buffer_depth * ETB_FRAME_SIZE_WORDS;
> +
> +	if (overwrite &&
> +	    ((nr_pages << PAGE_SHIFT) != capacity)) {
> +		dev_err(&csdev->dev, "Ring buffer not equal to device buffer");
> +		return NULL;
> +	}
>  
>  	if (cpu == -1)
>  		cpu = smp_processor_id();
> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c
> index 2527b5d3b65e..7694833b13cb 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
> @@ -380,6 +380,26 @@ static void *tmc_alloc_etf_buffer(struct coresight_device *csdev,
>  {
>  	int node, cpu = event->cpu;
>  	struct cs_buffers *buf;
> +	struct tmc_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.etf/buffer_size
> +	 *    0x10000
> +	 *    # perf record -e cs_etm/@20010000.etf/ -S -m,16 --per-thread $APP
> +	 *
> +	 */
> +	drvdata = dev_get_drvdata(csdev->dev.parent);
> +	if (overwrite &&
> +	    ((nr_pages << PAGE_SHIFT) != drvdata->size)) {
> +		dev_err(&csdev->dev, "Ring buffer not equal to device buffer");
> +		return NULL;
> +	}
>  
>  	if (cpu == -1)
>  		cpu = smp_processor_id();
> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> index df6e4b0b84e9..b9881d6d41ba 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> @@ -1188,9 +1188,13 @@ alloc_etr_buf(struct tmc_drvdata *drvdata, struct perf_event *event,
>  
>  	/*
>  	 * Try to match the perf ring buffer size if it is larger
> -	 * than the size requested via sysfs.
> +	 * than the size requested via sysfs.  In snapsot mode the size
> +	 * of the perf ring buffer needs to be equal to the allocated
> +	 * size if we want to reuse the generic AUX buffer management
> +	 * mechanic.
>  	 */
> -	if ((nr_pages << PAGE_SHIFT) > drvdata->size) {
> +	if (snapshot ||
> +	    (nr_pages << PAGE_SHIFT) > drvdata->size) {
>  		etr_buf = tmc_alloc_etr_buf(drvdata, (nr_pages << PAGE_SHIFT),
>  					    0, node, NULL);
>  		if (!IS_ERR(etr_buf))

If tmc_alloc_etr_buf() returns failure then it's possible to run into
the below sequence to allocate smaller buffer size for snapshot mode;
which is not expected for snapshot mode.

So here if tmc_alloc_etr_buf() fails to allocate buffer for snapshot
mode, should directly bail out with error code.

Thanks,
Leo Yan
Suzuki K Poulose May 7, 2019, 8:50 a.m. UTC | #2
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 ?

> 
> 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
Mathieu Poirier May 7, 2019, 5:24 p.m. UTC | #3
On Tue, 7 May 2019 at 01:39, Leo Yan <leo.yan@linaro.org> wrote:
>
> On Wed, May 01, 2019 at 11:50:48AM -0600, 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.
> >
> > 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):
>
> Here is delibrately to write as '4096 byte'?  Though I think should be
> '4096 bytes' but I am not confident which is right ...

Well, English isn't my first language either but I think it is correct
since I am referring to "the" page size and "4096 byte" is an
adjective of it.

>
> > +      *
> > +      *    # cat /sys/bus/coresight/devices/20010000.etb/mgmt/rdp
> > +      *    0x2000
> > +      *    # perf record -e cs_etm/@20010000.etf/ -S -m,8 --per-thread $APP
> > +      *
> > +      */
> > +     drvdata = dev_get_drvdata(csdev->dev.parent);
> > +     capacity = drvdata->buffer_depth * ETB_FRAME_SIZE_WORDS;
> > +
> > +     if (overwrite &&
> > +         ((nr_pages << PAGE_SHIFT) != capacity)) {
> > +             dev_err(&csdev->dev, "Ring buffer not equal to device buffer");
> > +             return NULL;
> > +     }
> >
> >       if (cpu == -1)
> >               cpu = smp_processor_id();
> > diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c
> > index 2527b5d3b65e..7694833b13cb 100644
> > --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
> > +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
> > @@ -380,6 +380,26 @@ static void *tmc_alloc_etf_buffer(struct coresight_device *csdev,
> >  {
> >       int node, cpu = event->cpu;
> >       struct cs_buffers *buf;
> > +     struct tmc_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.etf/buffer_size
> > +      *    0x10000
> > +      *    # perf record -e cs_etm/@20010000.etf/ -S -m,16 --per-thread $APP
> > +      *
> > +      */
> > +     drvdata = dev_get_drvdata(csdev->dev.parent);
> > +     if (overwrite &&
> > +         ((nr_pages << PAGE_SHIFT) != drvdata->size)) {
> > +             dev_err(&csdev->dev, "Ring buffer not equal to device buffer");
> > +             return NULL;
> > +     }
> >
> >       if (cpu == -1)
> >               cpu = smp_processor_id();
> > diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> > index df6e4b0b84e9..b9881d6d41ba 100644
> > --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
> > +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> > @@ -1188,9 +1188,13 @@ alloc_etr_buf(struct tmc_drvdata *drvdata, struct perf_event *event,
> >
> >       /*
> >        * Try to match the perf ring buffer size if it is larger
> > -      * than the size requested via sysfs.
> > +      * than the size requested via sysfs.  In snapsot mode the size
> > +      * of the perf ring buffer needs to be equal to the allocated
> > +      * size if we want to reuse the generic AUX buffer management
> > +      * mechanic.
> >        */
> > -     if ((nr_pages << PAGE_SHIFT) > drvdata->size) {
> > +     if (snapshot ||
> > +         (nr_pages << PAGE_SHIFT) > drvdata->size) {
> >               etr_buf = tmc_alloc_etr_buf(drvdata, (nr_pages << PAGE_SHIFT),
> >                                           0, node, NULL);
> >               if (!IS_ERR(etr_buf))
>
> If tmc_alloc_etr_buf() returns failure then it's possible to run into
> the below sequence to allocate smaller buffer size for snapshot mode;
> which is not expected for snapshot mode.
>
> So here if tmc_alloc_etr_buf() fails to allocate buffer for snapshot
> mode, should directly bail out with error code.

You are quite right - I need to fix this.

>
> Thanks,
> Leo Yan
Mathieu Poirier May 7, 2019, 8:22 p.m. UTC | #4
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

Patch
diff mbox series

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
+	 *
+	 */
+	drvdata = dev_get_drvdata(csdev->dev.parent);
+	capacity = drvdata->buffer_depth * ETB_FRAME_SIZE_WORDS;
+
+	if (overwrite &&
+	    ((nr_pages << PAGE_SHIFT) != capacity)) {
+		dev_err(&csdev->dev, "Ring buffer not equal to device buffer");
+		return NULL;
+	}
 
 	if (cpu == -1)
 		cpu = smp_processor_id();
diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c
index 2527b5d3b65e..7694833b13cb 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
@@ -380,6 +380,26 @@  static void *tmc_alloc_etf_buffer(struct coresight_device *csdev,
 {
 	int node, cpu = event->cpu;
 	struct cs_buffers *buf;
+	struct tmc_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.etf/buffer_size
+	 *    0x10000
+	 *    # perf record -e cs_etm/@20010000.etf/ -S -m,16 --per-thread $APP
+	 *
+	 */
+	drvdata = dev_get_drvdata(csdev->dev.parent);
+	if (overwrite &&
+	    ((nr_pages << PAGE_SHIFT) != drvdata->size)) {
+		dev_err(&csdev->dev, "Ring buffer not equal to device buffer");
+		return NULL;
+	}
 
 	if (cpu == -1)
 		cpu = smp_processor_id();
diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
index df6e4b0b84e9..b9881d6d41ba 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
@@ -1188,9 +1188,13 @@  alloc_etr_buf(struct tmc_drvdata *drvdata, struct perf_event *event,
 
 	/*
 	 * Try to match the perf ring buffer size if it is larger
-	 * than the size requested via sysfs.
+	 * than the size requested via sysfs.  In snapsot mode the size
+	 * of the perf ring buffer needs to be equal to the allocated
+	 * size if we want to reuse the generic AUX buffer management
+	 * mechanic.
 	 */
-	if ((nr_pages << PAGE_SHIFT) > drvdata->size) {
+	if (snapshot ||
+	    (nr_pages << PAGE_SHIFT) > drvdata->size) {
 		etr_buf = tmc_alloc_etr_buf(drvdata, (nr_pages << PAGE_SHIFT),
 					    0, node, NULL);
 		if (!IS_ERR(etr_buf))