| Message ID | 20190501175052.29667-2-mathieu.poirier@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | coresight: Fix snapshot mode | expand |
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
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
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
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
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))
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(-)