| Message ID | 20190325215632.17013-12-mathieu.poirier@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | coresight: Add support for CPU-wide trace scenarios | expand |
On 03/25/2019 09:56 PM, Mathieu Poirier wrote: > Refactoring function tmc_etr_setup_perf_buf() so that it only deals > with the high level etr_perf_buffer, and leaving the allocation of the > backend buffer (i.e etr_buf) to another function. > > That way the backend buffer allocation function can decide if it wants > to reuse an existing buffer (CPU-wide trace scenarios) or simply create > a new one. > > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> Looks good to me, except for one minor nit: > --- > .../hwtracing/coresight/coresight-tmc-etr.c | 46 +++++++++++++------ > 1 file changed, 31 insertions(+), 15 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c > index 6e2c2aa130d5..79fee9341446 100644 > --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c > +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c > @@ -1159,25 +1159,13 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev) > return ret; > } > > -/* > - * tmc_etr_setup_perf_buf: Allocate ETR buffer for use by perf. > - * The size of the hardware buffer is dependent on the size configured > - * via sysfs and the perf ring buffer size. We prefer to allocate the > - * largest possible size, scaling down the size by half until it > - * reaches a minimum limit (1M), beyond which we give up. > - */ > -static struct etr_perf_buffer * > -tmc_etr_setup_perf_buf(struct tmc_drvdata *drvdata, int node, int nr_pages, > - void **pages, bool snapshot) > +static struct etr_buf * > +tmc_etr_get_etr_buf(struct tmc_drvdata *drvdata, int node, > + int nr_pages, void **pages) nit: The name tmc_etr_get_etr_buf() sounds too generic and has nothing to do with the perf. It would be good to make it explicit that it is for perf session. May be, tmc_etr_perf_get_etr_buf() ? or may be even, simply get_perf_etr_buf(). Otherwise, Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
On Tue, Mar 26, 2019 at 03:29:03PM +0000, Suzuki K Poulose wrote: > On 03/25/2019 09:56 PM, Mathieu Poirier wrote: > > Refactoring function tmc_etr_setup_perf_buf() so that it only deals > > with the high level etr_perf_buffer, and leaving the allocation of the > > backend buffer (i.e etr_buf) to another function. > > > > That way the backend buffer allocation function can decide if it wants > > to reuse an existing buffer (CPU-wide trace scenarios) or simply create > > a new one. > > > > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> > > Looks good to me, except for one minor nit: > > > > --- > > .../hwtracing/coresight/coresight-tmc-etr.c | 46 +++++++++++++------ > > 1 file changed, 31 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c > > index 6e2c2aa130d5..79fee9341446 100644 > > --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c > > +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c > > @@ -1159,25 +1159,13 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev) > > return ret; > > } > > -/* > > - * tmc_etr_setup_perf_buf: Allocate ETR buffer for use by perf. > > - * The size of the hardware buffer is dependent on the size configured > > - * via sysfs and the perf ring buffer size. We prefer to allocate the > > - * largest possible size, scaling down the size by half until it > > - * reaches a minimum limit (1M), beyond which we give up. > > - */ > > -static struct etr_perf_buffer * > > -tmc_etr_setup_perf_buf(struct tmc_drvdata *drvdata, int node, int nr_pages, > > - void **pages, bool snapshot) > > +static struct etr_buf * > > +tmc_etr_get_etr_buf(struct tmc_drvdata *drvdata, int node, > > + int nr_pages, void **pages) > > nit: The name tmc_etr_get_etr_buf() sounds too generic and has nothing > to do with the perf. It would be good to make it explicit that it is for > perf session. > > May be, tmc_etr_perf_get_etr_buf() ? or may be even, simply > get_perf_etr_buf(). I don't have a strong preference here, the latter looks fine to me. > > Otherwise, > > Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com> Ok
diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c index 6e2c2aa130d5..79fee9341446 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c @@ -1159,25 +1159,13 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev) return ret; } -/* - * tmc_etr_setup_perf_buf: Allocate ETR buffer for use by perf. - * The size of the hardware buffer is dependent on the size configured - * via sysfs and the perf ring buffer size. We prefer to allocate the - * largest possible size, scaling down the size by half until it - * reaches a minimum limit (1M), beyond which we give up. - */ -static struct etr_perf_buffer * -tmc_etr_setup_perf_buf(struct tmc_drvdata *drvdata, int node, int nr_pages, - void **pages, bool snapshot) +static struct etr_buf * +tmc_etr_get_etr_buf(struct tmc_drvdata *drvdata, int node, + int nr_pages, void **pages) { struct etr_buf *etr_buf; - struct etr_perf_buffer *etr_perf; unsigned long size; - etr_perf = kzalloc_node(sizeof(*etr_perf), GFP_KERNEL, node); - if (!etr_perf) - return ERR_PTR(-ENOMEM); - /* * Try to match the perf ring buffer size if it is larger * than the size requested via sysfs. @@ -1201,6 +1189,34 @@ tmc_etr_setup_perf_buf(struct tmc_drvdata *drvdata, int node, int nr_pages, size /= 2; } while (size >= TMC_ETR_PERF_MIN_BUF_SIZE); + return ERR_PTR(-ENOMEM); + +done: + return etr_buf; +} + +/* + * tmc_etr_setup_perf_buf: Allocate ETR buffer for use by perf. + * The size of the hardware buffer is dependent on the size configured + * via sysfs and the perf ring buffer size. We prefer to allocate the + * largest possible size, scaling down the size by half until it + * reaches a minimum limit (1M), beyond which we give up. + */ +static struct etr_perf_buffer * +tmc_etr_setup_perf_buf(struct tmc_drvdata *drvdata, int node, + int nr_pages, void **pages, bool snapshot) +{ + struct etr_buf *etr_buf; + struct etr_perf_buffer *etr_perf; + + etr_perf = kzalloc_node(sizeof(*etr_perf), GFP_KERNEL, node); + if (!etr_perf) + return ERR_PTR(-ENOMEM); + + etr_buf = tmc_etr_get_etr_buf(drvdata, node, nr_pages, pages); + if (!IS_ERR(etr_buf)) + goto done; + kfree(etr_perf); return ERR_PTR(-ENOMEM);
Refactoring function tmc_etr_setup_perf_buf() so that it only deals with the high level etr_perf_buffer, and leaving the allocation of the backend buffer (i.e etr_buf) to another function. That way the backend buffer allocation function can decide if it wants to reuse an existing buffer (CPU-wide trace scenarios) or simply create a new one. Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> --- .../hwtracing/coresight/coresight-tmc-etr.c | 46 +++++++++++++------ 1 file changed, 31 insertions(+), 15 deletions(-)