[V2,19/30] coresight: etb10: implementing the setup_aux() API

Message ID 1445192687-24112-20-git-send-email-mathieu.poirier@linaro.org
State New
Headers show

Commit Message

Mathieu Poirier Oct. 18, 2015, 6:24 p.m.
Adding an ETB10 specific auxiliary area setup operation to be
used by the perf framework when events are initialised.

Part of this operation involves modeling the mmap'ed area based
on the specific ways a sink buffer gathers information.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 drivers/hwtracing/coresight/coresight-etb10.c | 55 +++++++++++++++++++++++++++
 include/linux/coresight.h                     |  3 ++
 2 files changed, 58 insertions(+)

Comments

Alexander Shishkin Oct. 19, 2015, 1:44 p.m. | #1
Mathieu Poirier <mathieu.poirier@linaro.org> writes:

> Adding an ETB10 specific auxiliary area setup operation to be
> used by the perf framework when events are initialised.
>
> Part of this operation involves modeling the mmap'ed area based
> on the specific ways a sink buffer gathers information.

It really doesn't seem to be ETB10 specific at all. When you add more
sinks, you'll probably end up copying this code every time.

Furthermore,

> +static void *etb_setup_aux(struct coresight_device *csdev, int cpu,
> +			   void **pages, int nr_pages, bool overwrite)
> +{
> +	int node, pg;
> +	struct cs_buffers *buf;
> +
> +	if (cpu == -1)
> +		cpu = smp_processor_id();
> +	node = cpu_to_node(cpu);
> +
> +	buf = kzalloc_node(offsetof(struct cs_buffers, addr[nr_pages]),
> +			   GFP_KERNEL, node);
> +	if (!buf)
> +		return NULL;
> +
> +	buf->snapshot = overwrite;
> +	buf->nr_pages = nr_pages;
> +
> +	/* Record information about buffers */
> +	for (pg = 0; pg < buf->nr_pages; pg++)
> +		buf->addr[pg] = pages[pg];
> +
> +	return buf;
> +}
> +

this one is so generic that I'm tempted to move this into perf's
ring_buffer code, because by the looks of it we'll need it pretty much
in every setup_aux().

Regards,
--
Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Alexander Shishkin Oct. 20, 2015, 11:37 a.m. | #2
Mathieu Poirier <mathieu.poirier@linaro.org> writes:

>  /**
> + * struct cs_buffer - keep track of a recording session' specifics
> + * @cur:	index of the current buffer
> + * @nr_pages:	max number of pages granted to us
> + * @nr_bufs:	number of clustered pages
> + * @offset:	offset within the current buffer
> + * @size:	how much space we have for this run
> + * @data_size:	how much we collected in this run
> + * @head:	head of the ring buffer
> + * @lost:	other than zero if we had a HW buffer wrap around
> + * @snapshot:	is this run in snapshot mode
> + * @addr:	virtual address this buffer starts at
> + */
> +struct cs_buffers {
> +	unsigned int		cur;
> +	unsigned int		nr_pages;
> +	unsigned int		nr_bufs;

This one is not really used.

> +	unsigned long		offset;
> +	unsigned long		size;

And this one seems to be only set in one place.

> +	local_t			data_size;
> +	local_t			head;

And so is this one.

> +	local_t			lost;
> +	bool			snapshot;
> +	void			*addr[0];

And this one seems to be a copy of what perf's ring buffer gives us.

> +static void *etb_setup_aux(struct coresight_device *csdev, int cpu,
> +			   void **pages, int nr_pages, bool overwrite)
> +{
> +	int node, pg;
> +	struct cs_buffers *buf;
> +
> +	if (cpu == -1)
> +		cpu = smp_processor_id();
> +	node = cpu_to_node(cpu);
> +
> +	buf = kzalloc_node(offsetof(struct cs_buffers, addr[nr_pages]),
> +			   GFP_KERNEL, node);
> +	if (!buf)
> +		return NULL;
> +
> +	buf->snapshot = overwrite;
> +	buf->nr_pages = nr_pages;
> +
> +	/* Record information about buffers */
> +	for (pg = 0; pg < buf->nr_pages; pg++)
> +		buf->addr[pg] = pages[pg];

Yes, buf::addr is a copy of @pages. You could save some space by just
saving @pages, it's going to be around until pmu::free_aux().

Regards,
--
Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Mathieu Poirier Oct. 20, 2015, 4:40 p.m. | #3
On 19 October 2015 at 07:44, Alexander Shishkin
<alexander.shishkin@linux.intel.com> wrote:
> Mathieu Poirier <mathieu.poirier@linaro.org> writes:
>
>> Adding an ETB10 specific auxiliary area setup operation to be
>> used by the perf framework when events are initialised.
>>
>> Part of this operation involves modeling the mmap'ed area based
>> on the specific ways a sink buffer gathers information.
>
> It really doesn't seem to be ETB10 specific at all. When you add more
> sinks, you'll probably end up copying this code every time.

That will depend on how that specific sinks work, but indeed, it is
pretty generic.

>
> Furthermore,
>
>> +static void *etb_setup_aux(struct coresight_device *csdev, int cpu,
>> +                        void **pages, int nr_pages, bool overwrite)
>> +{
>> +     int node, pg;
>> +     struct cs_buffers *buf;
>> +
>> +     if (cpu == -1)
>> +             cpu = smp_processor_id();
>> +     node = cpu_to_node(cpu);
>> +
>> +     buf = kzalloc_node(offsetof(struct cs_buffers, addr[nr_pages]),
>> +                        GFP_KERNEL, node);
>> +     if (!buf)
>> +             return NULL;
>> +
>> +     buf->snapshot = overwrite;
>> +     buf->nr_pages = nr_pages;
>> +
>> +     /* Record information about buffers */
>> +     for (pg = 0; pg < buf->nr_pages; pg++)
>> +             buf->addr[pg] = pages[pg];
>> +
>> +     return buf;
>> +}
>> +
>
> this one is so generic that I'm tempted to move this into perf's
> ring_buffer code, because by the looks of it we'll need it pretty much
> in every setup_aux().

It is tempting but I suggest we wait to see what kind of trend we get
before moving ahead with this.  There is always opportunity for
further consolidation should the need arise.

>
> Regards,
> --
> Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Patch hide | download patch | download mbox

diff --git a/drivers/hwtracing/coresight/coresight-etb10.c b/drivers/hwtracing/coresight/coresight-etb10.c
index ecdbe0dd4d08..7f34e7af465e 100644
--- a/drivers/hwtracing/coresight/coresight-etb10.c
+++ b/drivers/hwtracing/coresight/coresight-etb10.c
@@ -27,6 +27,9 @@ 
 #include <linux/coresight.h>
 #include <linux/amba/bus.h>
 #include <linux/clk.h>
+#include <linux/mm.h>
+
+#include <asm/local.h>
 
 #include "coresight-priv.h"
 
@@ -64,6 +67,32 @@ 
 #define ETB_FRAME_SIZE_WORDS	4
 
 /**
+ * struct cs_buffer - keep track of a recording session' specifics
+ * @cur:	index of the current buffer
+ * @nr_pages:	max number of pages granted to us
+ * @nr_bufs:	number of clustered pages
+ * @offset:	offset within the current buffer
+ * @size:	how much space we have for this run
+ * @data_size:	how much we collected in this run
+ * @head:	head of the ring buffer
+ * @lost:	other than zero if we had a HW buffer wrap around
+ * @snapshot:	is this run in snapshot mode
+ * @addr:	virtual address this buffer starts at
+ */
+struct cs_buffers {
+	unsigned int		cur;
+	unsigned int		nr_pages;
+	unsigned int		nr_bufs;
+	unsigned long		offset;
+	unsigned long		size;
+	local_t			data_size;
+	local_t			head;
+	local_t			lost;
+	bool			snapshot;
+	void			*addr[0];
+};
+
+/**
  * struct etb_drvdata - specifics associated to an ETB component
  * @base:	memory mapped base address for this component.
  * @dev:	the device entity associated to this component.
@@ -252,9 +281,35 @@  static void etb_disable(struct coresight_device *csdev)
 	dev_info(drvdata->dev, "ETB disabled\n");
 }
 
+static void *etb_setup_aux(struct coresight_device *csdev, int cpu,
+			   void **pages, int nr_pages, bool overwrite)
+{
+	int node, pg;
+	struct cs_buffers *buf;
+
+	if (cpu == -1)
+		cpu = smp_processor_id();
+	node = cpu_to_node(cpu);
+
+	buf = kzalloc_node(offsetof(struct cs_buffers, addr[nr_pages]),
+			   GFP_KERNEL, node);
+	if (!buf)
+		return NULL;
+
+	buf->snapshot = overwrite;
+	buf->nr_pages = nr_pages;
+
+	/* Record information about buffers */
+	for (pg = 0; pg < buf->nr_pages; pg++)
+		buf->addr[pg] = pages[pg];
+
+	return buf;
+}
+
 static const struct coresight_ops_sink etb_sink_ops = {
 	.enable		= etb_enable,
 	.disable	= etb_disable,
+	.setup_aux	= etb_setup_aux,
 };
 
 static const struct coresight_ops etb_cs_ops = {
diff --git a/include/linux/coresight.h b/include/linux/coresight.h
index dd530a6d3e21..b1c25eba83b4 100644
--- a/include/linux/coresight.h
+++ b/include/linux/coresight.h
@@ -187,10 +187,13 @@  struct coresight_device {
  * Operations available for sinks
  * @enable:	enables the sink.
  * @disable:	disables the sink.
+ * @setup_aux:	initialises perf's ring buffer for trace collection.
  */
 struct coresight_ops_sink {
 	int (*enable)(struct coresight_device *csdev);
 	void (*disable)(struct coresight_device *csdev);
+	void *(*setup_aux)(struct coresight_device *csdev, int cpu,
+			   void **pages, int nr_pages, bool overwrite);
 };
 
 /**