[PATCH 0/3] Generalise ARM perf-events backend for oprofile

[PATCH 0/3] Generalise ARM perf-events backend for oprofile

[Matt Fleming]

The perf-events backend for OProfile that Will Deacon wrote in
8c1fc96f6fd1f361428ba805103af0d0eee65179 ("ARM: 6072/1: oprofile: use
perf-events framework as backend") is of use to more architectures
than just ARM. Move the code into drivers/oprofile/ so that SH can use
it instead of the nearly identical copy of its OProfile code.

The benefit of the backend is that it becomes necessary to only
maintain one copy of the PMU accessor functions for each architecture,
with bug fixes and new features benefiting both OProfile and perf.

Note that I haven't been able to test these patches on an ARM board to
see if I've caused any regressions. If anyone else could do that I'd
appreciate it.

This patch series is based on tip/master.

Matt Fleming (3):
  sh: Accessor functions for the sh_pmu state
  oprofile: Abstract the perf-events backend for oprofile
  sh: Use the perf-events backend for oprofile

 arch/arm/oprofile/Makefile       |    4 +
 arch/arm/oprofile/common.c       |  176 +-----------------------------------
 arch/sh/include/asm/perf_event.h |    2 +
 arch/sh/kernel/perf_event.c      |   13 +++
 arch/sh/oprofile/Makefile        |    4 +
 arch/sh/oprofile/common.c        |   95 +++----------------
 arch/sh/oprofile/op_impl.h       |   33 -------
 drivers/oprofile/oprofile_perf.c |  188 ++++++++++++++++++++++++++++++++++++++
 include/linux/oprofile.h         |   12 +++
 9 files changed, 240 insertions(+), 287 deletions(-)
 delete mode 100644 arch/sh/oprofile/op_impl.h
 create mode 100644 drivers/oprofile/oprofile_perf.c

-- 
1.7.2.1

[PATCH 1/3] sh: Accessor functions for the sh_pmu state

[Matt Fleming]

Introduce some accessor functions for getting at the name and number of
counters of the current sh_pmu instance.

Signed-off-by: Matt Fleming <matt@console-pimps.org>
---
 arch/sh/include/asm/perf_event.h |    2 ++
 arch/sh/kernel/perf_event.c      |   13 +++++++++++++
 2 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/arch/sh/include/asm/perf_event.h b/arch/sh/include/asm/perf_event.h
index 3d0c9f3..5b7fa84 100644
--- a/arch/sh/include/asm/perf_event.h
+++ b/arch/sh/include/asm/perf_event.h
@@ -25,6 +25,8 @@ struct sh_pmu {
 extern int register_sh_pmu(struct sh_pmu *);
 extern int reserve_pmc_hardware(void);
 extern void release_pmc_hardware(void);
+extern int sh_pmu_num_events(void);
+extern const char *sh_pmu_name(void);
 
 static inline void set_perf_event_pending(void)
 {
diff --git a/arch/sh/kernel/perf_event.c b/arch/sh/kernel/perf_event.c
index 7a3dc35..086f788 100644
--- a/arch/sh/kernel/perf_event.c
+++ b/arch/sh/kernel/perf_event.c
@@ -60,6 +60,19 @@ static inline int sh_pmu_initialized(void)
 }
 
 /*
+ * Return the number of events for the current sh_pmu.
+ */
+int sh_pmu_num_events(void)
+{
+	return sh_pmu->num_events;
+}
+
+const char *sh_pmu_name(void)
+{
+	return sh_pmu->name;
+}
+
+/*
  * Release the PMU if this is the last perf_event.
  */
 static void hw_perf_event_destroy(struct perf_event *event)
-- 
1.7.2.1

[PATCH 2/3] oprofile: Abstract the perf-events backend

[Matt Fleming]

Move the perf-events backend from arch/arm/oprofile into
drivers/oprofile so that the code can be shared between architectures.

This allows each architecture to maintain only a single copy of the
PMU accessor functions instead of one for both perf and OProfile. It
also becomes possible for other architectures to delete much of their
OProfile code in favour of the common code now available in
drivers/oprofile/oprofile_perf.c.

Signed-off-by: Matt Fleming <matt@console-pimps.org>
---
 arch/arm/oprofile/Makefile       |    4 +
 arch/arm/oprofile/common.c       |  176 +-----------------------------------
 drivers/oprofile/oprofile_perf.c |  188 ++++++++++++++++++++++++++++++++++++++
 include/linux/oprofile.h         |   12 +++
 4 files changed, 207 insertions(+), 173 deletions(-)
 create mode 100644 drivers/oprofile/oprofile_perf.c

diff --git a/arch/arm/oprofile/Makefile b/arch/arm/oprofile/Makefile
index e666eaf..def77a1 100644
--- a/arch/arm/oprofile/Makefile
+++ b/arch/arm/oprofile/Makefile
@@ -6,4 +6,8 @@ DRIVER_OBJS = $(addprefix ../../../drivers/oprofile/, \
 		oprofilefs.o oprofile_stats.o \
 		timer_int.o )
 
+ifeq ($(CONFIG_HW_PERF_EVENTS),y)
+DRIVERS_OBJS += $(addprefix ../../../drivers/oprofile/, oprofile_perf.o)
+endif
+
 oprofile-y				:= $(DRIVER_OBJS) common.o
diff --git a/arch/arm/oprofile/common.c b/arch/arm/oprofile/common.c
index 0691176..50e4980 100644
--- a/arch/arm/oprofile/common.c
+++ b/arch/arm/oprofile/common.c
@@ -25,136 +25,9 @@
 #include <asm/ptrace.h>
 
 #ifdef CONFIG_HW_PERF_EVENTS
-/*
- * Per performance monitor configuration as set via oprofilefs.
- */
-struct op_counter_config {
-	unsigned long count;
-	unsigned long enabled;
-	unsigned long event;
-	unsigned long unit_mask;
-	unsigned long kernel;
-	unsigned long user;
-	struct perf_event_attr attr;
-};
-
 static int op_arm_enabled;
 static DEFINE_MUTEX(op_arm_mutex);
 
-static struct op_counter_config *counter_config;
-static struct perf_event **perf_events[nr_cpumask_bits];
-static int perf_num_counters;
-
-/*
- * Overflow callback for oprofile.
- */
-static void op_overflow_handler(struct perf_event *event, int unused,
-			struct perf_sample_data *data, struct pt_regs *regs)
-{
-	int id;
-	u32 cpu = smp_processor_id();
-
-	for (id = 0; id < perf_num_counters; ++id)
-		if (perf_events[cpu][id] == event)
-			break;
-
-	if (id != perf_num_counters)
-		oprofile_add_sample(regs, id);
-	else
-		pr_warning("oprofile: ignoring spurious overflow "
-				"on cpu %u\n", cpu);
-}
-
-/*
- * Called by op_arm_setup to create perf attributes to mirror the oprofile
- * settings in counter_config. Attributes are created as `pinned' events and
- * so are permanently scheduled on the PMU.
- */
-static void op_perf_setup(void)
-{
-	int i;
-	u32 size = sizeof(struct perf_event_attr);
-	struct perf_event_attr *attr;
-
-	for (i = 0; i < perf_num_counters; ++i) {
-		attr = &counter_config[i].attr;
-		memset(attr, 0, size);
-		attr->type		= PERF_TYPE_RAW;
-		attr->size		= size;
-		attr->config		= counter_config[i].event;
-		attr->sample_period	= counter_config[i].count;
-		attr->pinned		= 1;
-	}
-}
-
-static int op_create_counter(int cpu, int event)
-{
-	int ret = 0;
-	struct perf_event *pevent;
-
-	if (!counter_config[event].enabled || (perf_events[cpu][event] != NULL))
-		return ret;
-
-	pevent = perf_event_create_kernel_counter(&counter_config[event].attr,
-						  cpu, -1,
-						  op_overflow_handler);
-
-	if (IS_ERR(pevent)) {
-		ret = PTR_ERR(pevent);
-	} else if (pevent->state != PERF_EVENT_STATE_ACTIVE) {
-		pr_warning("oprofile: failed to enable event %d "
-				"on CPU %d\n", event, cpu);
-		ret = -EBUSY;
-	} else {
-		perf_events[cpu][event] = pevent;
-	}
-
-	return ret;
-}
-
-static void op_destroy_counter(int cpu, int event)
-{
-	struct perf_event *pevent = perf_events[cpu][event];
-
-	if (pevent) {
-		perf_event_release_kernel(pevent);
-		perf_events[cpu][event] = NULL;
-	}
-}
-
-/*
- * Called by op_arm_start to create active perf events based on the
- * perviously configured attributes.
- */
-static int op_perf_start(void)
-{
-	int cpu, event, ret = 0;
-
-	for_each_online_cpu(cpu) {
-		for (event = 0; event < perf_num_counters; ++event) {
-			ret = op_create_counter(cpu, event);
-			if (ret)
-				goto out;
-		}
-	}
-
-out:
-	return ret;
-}
-
-/*
- * Called by op_arm_stop@the end of a profiling run.
- */
-static void op_perf_stop(void)
-{
-	int cpu, event;
-
-	for_each_online_cpu(cpu)
-		for (event = 0; event < perf_num_counters; ++event)
-			op_destroy_counter(cpu, event);
-}
-
-
 static char *op_name_from_perf_id(enum arm_perf_pmu_ids id)
 {
 	switch (id) {
@@ -175,27 +48,6 @@ static char *op_name_from_perf_id(enum arm_perf_pmu_ids id)
 	}
 }
 
-static int op_arm_create_files(struct super_block *sb, struct dentry *root)
-{
-	unsigned int i;
-
-	for (i = 0; i < perf_num_counters; i++) {
-		struct dentry *dir;
-		char buf[4];
-
-		snprintf(buf, sizeof buf, "%d", i);
-		dir = oprofilefs_mkdir(sb, root, buf);
-		oprofilefs_create_ulong(sb, dir, "enabled", &counter_config[i].enabled);
-		oprofilefs_create_ulong(sb, dir, "event", &counter_config[i].event);
-		oprofilefs_create_ulong(sb, dir, "count", &counter_config[i].count);
-		oprofilefs_create_ulong(sb, dir, "unit_mask", &counter_config[i].unit_mask);
-		oprofilefs_create_ulong(sb, dir, "kernel", &counter_config[i].kernel);
-		oprofilefs_create_ulong(sb, dir, "user", &counter_config[i].user);
-	}
-
-	return 0;
-}
-
 static int op_arm_setup(void)
 {
 	spin_lock(&oprofilefs_lock);
@@ -349,39 +201,17 @@ static void arm_backtrace(struct pt_regs * const regs, unsigned int depth)
 
 int __init oprofile_arch_init(struct oprofile_operations *ops)
 {
-	int cpu, ret = 0;
-
-	perf_num_counters = armpmu_get_max_events();
-
-	counter_config = kcalloc(perf_num_counters,
-			sizeof(struct op_counter_config), GFP_KERNEL);
-
-	if (!counter_config) {
-		pr_info("oprofile: failed to allocate %d "
-				"counters\n", perf_num_counters);
-		return -ENOMEM;
-	}
+	int ret = 0;
 
 	ret = init_driverfs();
 	if (ret) {
-		kfree(counter_config);
 		return ret;
 	}
 
-	for_each_possible_cpu(cpu) {
-		perf_events[cpu] = kcalloc(perf_num_counters,
-				sizeof(struct perf_event *), GFP_KERNEL);
-		if (!perf_events[cpu]) {
-			pr_info("oprofile: failed to allocate %d perf events "
-					"for cpu %d\n", perf_num_counters, cpu);
-			while (--cpu >= 0)
-				kfree(perf_events[cpu]);
-			return -ENOMEM;
-		}
-	}
+	op_perf_set_num_counters(armpmu_get_max_events());
 
 	ops->backtrace		= arm_backtrace;
-	ops->create_files	= op_arm_create_files;
+	ops->create_files	= op_perf_create_files;
 	ops->setup		= op_arm_setup;
 	ops->start		= op_arm_start;
 	ops->stop		= op_arm_stop;
diff --git a/drivers/oprofile/oprofile_perf.c b/drivers/oprofile/oprofile_perf.c
new file mode 100644
index 0000000..9ab7b94
--- /dev/null
+++ b/drivers/oprofile/oprofile_perf.c
@@ -0,0 +1,188 @@
+/*
+ * Copyright 2010 ARM Ltd.
+ *
+ * Perf-events backend for OProfile.
+ */
+#include <linux/slab.h>
+#include <linux/oprofile.h>
+#include <linux/perf_event.h>
+
+/* Per-counter configuration as set via oprofilefs.  */
+struct op_counter_config {
+	unsigned long enabled;
+	unsigned long event;
+
+	unsigned long count;
+
+	/* Dummy values for userspace tool compliance */
+	unsigned long kernel;
+	unsigned long user;
+	unsigned long unit_mask;
+
+	struct perf_event_attr attr;
+};
+
+static struct op_counter_config *counter_config;
+static struct perf_event **perf_events[nr_cpumask_bits];
+static int perf_num_counters;
+
+/*
+ * Overflow callback for oprofile.
+ */
+static void op_overflow_handler(struct perf_event *event, int unused,
+				struct perf_sample_data *data,
+				struct pt_regs *regs)
+{
+	int id;
+	u32 cpu = smp_processor_id();
+
+	for (id = 0; id < perf_num_counters; ++id)
+		if (perf_events[cpu][id] == event)
+			break;
+
+	if (id != perf_num_counters)
+		oprofile_add_sample(regs, id);
+	else
+		pr_warning("oprofile: ignoring spurious overflow "
+				"on cpu %u\n", cpu);
+}
+
+/*
+ * Create perf attributes to mirror the oprofile settings in
+ * counter_config. Attributes are created as `pinned' events and so are
+ * permanently scheduled on the PMU.
+ */
+int op_perf_setup(void)
+{
+	int i;
+	u32 attr_size = sizeof(struct perf_event_attr);
+	struct perf_event_attr *attr;
+
+	for (i = 0; i < perf_num_counters; ++i) {
+		attr = &counter_config[i].attr;
+		memset(attr, 0, attr_size);
+		attr->type		= PERF_TYPE_RAW;
+		attr->size		= attr_size;
+		attr->config		= counter_config[i].event;
+		attr->sample_period	= counter_config[i].count;
+		attr->pinned		= 1;
+	}
+
+	return 0;
+}
+
+int op_create_counter(int cpu, int event)
+{
+	int ret = 0;
+	struct perf_event *pevent;
+
+	if (!counter_config[event].enabled || (perf_events[cpu][event] != NULL))
+		return ret;
+
+	pevent = perf_event_create_kernel_counter(&counter_config[event].attr,
+						  cpu, -1,
+						  op_overflow_handler);
+
+	if (IS_ERR(pevent)) {
+		ret = PTR_ERR(pevent);
+	} else if (pevent->state != PERF_EVENT_STATE_ACTIVE) {
+		pr_warning("oprofile: failed to enable event %d "
+				"on CPU %d\n", event, cpu);
+		ret = -EBUSY;
+	} else {
+		perf_events[cpu][event] = pevent;
+	}
+
+	return ret;
+}
+
+void op_destroy_counter(int cpu, int event)
+{
+	struct perf_event *pevent = perf_events[cpu][event];
+
+	if (pevent) {
+		perf_event_release_kernel(pevent);
+		perf_events[cpu][event] = NULL;
+	}
+}
+
+/*
+ * Create active perf events based on the perviously configured
+ * attributes.
+ */
+int op_perf_start(void)
+{
+	int cpu, event, ret = 0;
+
+	for_each_online_cpu(cpu) {
+		for (event = 0; event < perf_num_counters; ++event) {
+			ret = op_create_counter(cpu, event);
+			if (ret)
+				goto out;
+		}
+	}
+
+out:
+	return ret;
+}
+
+/*
+ * Called@the end of a profiling run.
+ */
+void op_perf_stop(void)
+{
+	int cpu, event;
+
+	for_each_online_cpu(cpu)
+		for (event = 0; event < perf_num_counters; ++event)
+			op_destroy_counter(cpu, event);
+}
+
+
+int op_perf_create_files(struct super_block *sb, struct dentry *root)
+{
+	u32 counter_size = sizeof(struct op_counter_config);
+	int i, cpu;
+
+	counter_config = kcalloc(perf_num_counters, counter_size, GFP_KERNEL);
+
+	if (!counter_config) {
+		pr_info("oprofile: failed to allocate %d "
+				"counters\n", perf_num_counters);
+		return -ENOMEM;
+	}
+
+	for_each_possible_cpu(cpu) {
+		perf_events[cpu] = kcalloc(perf_num_counters,
+				sizeof(struct perf_event *), GFP_KERNEL);
+		if (!perf_events[cpu]) {
+			pr_info("oprofile: failed to allocate %d perf events "
+					"for cpu %d\n", perf_num_counters, cpu);
+			while (--cpu >= 0)
+				kfree(perf_events[cpu]);
+			kfree(counter_config);
+			return -ENOMEM;
+		}
+	}
+
+	for (i = 0; i < perf_num_counters; i++) {
+		struct dentry *dir;
+		char buf[4];
+
+		snprintf(buf, sizeof buf, "%d", i);
+		dir = oprofilefs_mkdir(sb, root, buf);
+		oprofilefs_create_ulong(sb, dir, "enabled", &counter_config[i].enabled);
+		oprofilefs_create_ulong(sb, dir, "event", &counter_config[i].event);
+		oprofilefs_create_ulong(sb, dir, "count", &counter_config[i].count);
+		oprofilefs_create_ulong(sb, dir, "unit_mask", &counter_config[i].unit_mask);
+		oprofilefs_create_ulong(sb, dir, "kernel", &counter_config[i].kernel);
+		oprofilefs_create_ulong(sb, dir, "user", &counter_config[i].user);
+	}
+
+	return 0;
+}
+
+void op_perf_set_num_counters(int num_counters)
+{
+	perf_num_counters = num_counters;
+}
diff --git a/include/linux/oprofile.h b/include/linux/oprofile.h
index 5171639..4b3f8af 100644
--- a/include/linux/oprofile.h
+++ b/include/linux/oprofile.h
@@ -185,4 +185,16 @@ int oprofile_add_data(struct op_entry *entry, unsigned long val);
 int oprofile_add_data64(struct op_entry *entry, u64 val);
 int oprofile_write_commit(struct op_entry *entry);
 
+/* Oprofile perf wrapper functions. */
+
+#ifdef CONFIG_PERF_EVENTS
+int op_perf_setup(void);
+int op_create_counter(int cpu, int event);
+void op_destroy_counter(int cpu, int event);
+int op_perf_start(void);
+void op_perf_stop(void);
+int op_perf_create_files(struct super_block *sb, struct dentry *root);
+void op_perf_set_num_counters(int num_counters);
+#endif /* CONFIG_PERF_EVENTS */
+
 #endif /* OPROFILE_H */
-- 
1.7.2.1

[PATCH 3/3] sh: Use the perf-events backend for oprofile

[Matt Fleming]

Use the perf-events based wrapper for oprofile available in
drivers/oprofile. This allows us to centralise the code to control
performance counters.

Signed-off-by: Matt Fleming <matt@console-pimps.org>
---
 arch/sh/oprofile/Makefile  |    4 ++
 arch/sh/oprofile/common.c  |   95 ++++++-------------------------------------
 arch/sh/oprofile/op_impl.h |   33 ---------------
 3 files changed, 18 insertions(+), 114 deletions(-)
 delete mode 100644 arch/sh/oprofile/op_impl.h

diff --git a/arch/sh/oprofile/Makefile b/arch/sh/oprofile/Makefile
index 4886c5c..00ef946 100644
--- a/arch/sh/oprofile/Makefile
+++ b/arch/sh/oprofile/Makefile
@@ -6,4 +6,8 @@ DRIVER_OBJS = $(addprefix ../../../drivers/oprofile/, \
 		oprofilefs.o oprofile_stats.o \
 		timer_int.o )
 
+ifeq ($(CONFIG_PERF_EVENTS), y)
+DRIVER_OBJS += $(addprefix ../../../drivers/oprofile/, oprofile_perf.o)
+endif
+
 oprofile-y	:= $(DRIVER_OBJS) common.o backtrace.o
diff --git a/arch/sh/oprofile/common.c b/arch/sh/oprofile/common.c
index ac60493..b5b0d5e 100644
--- a/arch/sh/oprofile/common.c
+++ b/arch/sh/oprofile/common.c
@@ -17,73 +17,23 @@
 #include <linux/init.h>
 #include <linux/errno.h>
 #include <linux/smp.h>
+#include <linux/perf_event.h>
 #include <asm/processor.h>
-#include "op_impl.h"
-
-static struct op_sh_model *model;
-
-static struct op_counter_config ctr[20];
 
 extern void sh_backtrace(struct pt_regs * const regs, unsigned int depth);
 
-static int op_sh_setup(void)
-{
-	/* Pre-compute the values to stuff in the hardware registers.  */
-	model->reg_setup(ctr);
-
-	/* Configure the registers on all cpus.  */
-	on_each_cpu(model->cpu_setup, NULL, 1);
-
-        return 0;
-}
-
-static int op_sh_create_files(struct super_block *sb, struct dentry *root)
-{
-	int i, ret = 0;
-
-	for (i = 0; i < model->num_counters; i++) {
-		struct dentry *dir;
-		char buf[4];
-
-		snprintf(buf, sizeof(buf), "%d", i);
-		dir = oprofilefs_mkdir(sb, root, buf);
-
-		ret |= oprofilefs_create_ulong(sb, dir, "enabled", &ctr[i].enabled);
-		ret |= oprofilefs_create_ulong(sb, dir, "event", &ctr[i].event);
-		ret |= oprofilefs_create_ulong(sb, dir, "kernel", &ctr[i].kernel);
-		ret |= oprofilefs_create_ulong(sb, dir, "user", &ctr[i].user);
-
-		if (model->create_files)
-			ret |= model->create_files(sb, dir);
-		else
-			ret |= oprofilefs_create_ulong(sb, dir, "count", &ctr[i].count);
-
-		/* Dummy entries */
-		ret |= oprofilefs_create_ulong(sb, dir, "unit_mask", &ctr[i].unit_mask);
-	}
-
-	return ret;
-}
-
-static int op_sh_start(void)
+static char *op_name_from_perf_name(const char *name)
 {
-	/* Enable performance monitoring for all counters.  */
-	on_each_cpu(model->cpu_start, NULL, 1);
+	if (!strcmp(name, "SH-4A"))
+		return "sh/sh4a";
+	if (!strcmp(name, "SH7750"))
+		return "sh/sh7750";
 
-	return 0;
-}
-
-static void op_sh_stop(void)
-{
-	/* Disable performance monitoring for all counters.  */
-	on_each_cpu(model->cpu_stop, NULL, 1);
+	return NULL;
 }
 
 int __init oprofile_arch_init(struct oprofile_operations *ops)
 {
-	struct op_sh_model *lmodel = NULL;
-	int ret;
-
 	/*
 	 * Always assign the backtrace op. If the counter initialization
 	 * fails, we fall back to the timer which will still make use of
@@ -91,40 +41,23 @@ int __init oprofile_arch_init(struct oprofile_operations *ops)
 	 */
 	ops->backtrace = sh_backtrace;
 
-	/*
-	 * XXX
-	 *
-	 * All of the SH7750/SH-4A counters have been converted to perf,
-	 * this infrastructure hook is left for other users until they've
-	 * had a chance to convert over, at which point all of this
-	 * will be deleted.
-	 */
-
-	if (!lmodel)
-		return -ENODEV;
 	if (!(current_cpu_data.flags & CPU_HAS_PERF_COUNTER))
 		return -ENODEV;
 
-	ret = lmodel->init();
-	if (unlikely(ret != 0))
-		return ret;
-
-	model = lmodel;
+	ops->setup		= op_perf_setup;
+	ops->create_files	= op_perf_create_files;
+	ops->start		= op_perf_start;
+	ops->stop		= op_perf_stop;
+	ops->cpu_type		= op_name_from_perf_name(sh_pmu_name());
 
-	ops->setup		= op_sh_setup;
-	ops->create_files	= op_sh_create_files;
-	ops->start		= op_sh_start;
-	ops->stop		= op_sh_stop;
-	ops->cpu_type		= lmodel->cpu_type;
+	op_perf_set_num_counters(sh_pmu_num_events());
 
 	printk(KERN_INFO "oprofile: using %s performance monitoring.\n",
-	       lmodel->cpu_type);
+	       ops->cpu_type);
 
 	return 0;
 }
 
 void oprofile_arch_exit(void)
 {
-	if (model && model->exit)
-		model->exit();
 }
diff --git a/arch/sh/oprofile/op_impl.h b/arch/sh/oprofile/op_impl.h
deleted file mode 100644
index 1244479..0000000
--- a/arch/sh/oprofile/op_impl.h
+++ /dev/null
@@ -1,33 +0,0 @@
-#ifndef __OP_IMPL_H
-#define __OP_IMPL_H
-
-/* Per-counter configuration as set via oprofilefs.  */
-struct op_counter_config {
-	unsigned long enabled;
-	unsigned long event;
-
-	unsigned long count;
-
-	/* Dummy values for userspace tool compliance */
-	unsigned long kernel;
-	unsigned long user;
-	unsigned long unit_mask;
-};
-
-/* Per-architecture configury and hooks.  */
-struct op_sh_model {
-	void (*reg_setup)(struct op_counter_config *);
-	int (*create_files)(struct super_block *sb, struct dentry *dir);
-	void (*cpu_setup)(void *dummy);
-	int (*init)(void);
-	void (*exit)(void);
-	void (*cpu_start)(void *args);
-	void (*cpu_stop)(void *args);
-	char *cpu_type;
-	unsigned char num_counters;
-};
-
-/* arch/sh/oprofile/common.c */
-extern void sh_backtrace(struct pt_regs * const regs, unsigned int depth);
-
-#endif /* __OP_IMPL_H */
-- 
1.7.2.1

[PATCH 0/3] Generalise ARM perf-events backend for oprofile

[Christoph Hellwig]

On Mon, Aug 23, 2010 at 11:46:18AM +0100, Matt Fleming wrote:
> The perf-events backend for OProfile that Will Deacon wrote in
> 8c1fc96f6fd1f361428ba805103af0d0eee65179 ("ARM: 6072/1: oprofile: use
> perf-events framework as backend") is of use to more architectures
> than just ARM. Move the code into drivers/oprofile/ so that SH can use
> it instead of the nearly identical copy of its OProfile code.

Nice, I didn't know such a backend already existed.  Now that you
have made it generic we should aim towards making it the only oprofile
backend and getting rid of all the duplication.

[PATCH 0/3] Generalise ARM perf-events backend for oprofile

[Matt Fleming]

On Mon, Aug 23, 2010 at 06:57:59AM -0400, Christoph Hellwig wrote:
> On Mon, Aug 23, 2010 at 11:46:18AM +0100, Matt Fleming wrote:
> > The perf-events backend for OProfile that Will Deacon wrote in
> > 8c1fc96f6fd1f361428ba805103af0d0eee65179 ("ARM: 6072/1: oprofile: use
> > perf-events framework as backend") is of use to more architectures
> > than just ARM. Move the code into drivers/oprofile/ so that SH can use
> > it instead of the nearly identical copy of its OProfile code.
> 
> Nice, I didn't know such a backend already existed.  Now that you
> have made it generic we should aim towards making it the only oprofile
> backend and getting rid of all the duplication.

Definitely. I've added linux-arch in case there's some maintainers
that want to use this now.

The new generic code is in this patch,

	http://lkml.org/lkml/2010/8/23/118

For an example of how the SH oprofile stuff changed see,

	http://lkml.org/lkml/2010/8/23/117

[PATCH 0/3] Generalise ARM perf-events backend for oprofile

[Benjamin Herrenschmidt]

On Mon, 2010-08-23 at 06:57 -0400, Christoph Hellwig wrote:
> On Mon, Aug 23, 2010 at 11:46:18AM +0100, Matt Fleming wrote:
> > The perf-events backend for OProfile that Will Deacon wrote in
> > 8c1fc96f6fd1f361428ba805103af0d0eee65179 ("ARM: 6072/1: oprofile: use
> > perf-events framework as backend") is of use to more architectures
> > than just ARM. Move the code into drivers/oprofile/ so that SH can use
> > it instead of the nearly identical copy of its OProfile code.
> 
> Nice, I didn't know such a backend already existed.  Now that you
> have made it generic we should aim towards making it the only oprofile
> backend and getting rid of all the duplication.

Even better would be to do the surgery at a higher level and provide the
oprofile API without the oprofile buffer management. My experience is
that it doesn't scale, and on heavily threaded large SMP setup, there is
an enormous amount of time wasted contending on the global oprofile
buffer mutex.

Cheers,
Ben.

[PATCH 0/3] Generalise ARM perf-events backend for oprofile

[Will Deacon]

Ben,

On Wed, 2010-08-25 at 02:41 +0100, Benjamin Herrenschmidt wrote:
> On Mon, 2010-08-23 at 06:57 -0400, Christoph Hellwig wrote:
> > On Mon, Aug 23, 2010 at 11:46:18AM +0100, Matt Fleming wrote:
> > > The perf-events backend for OProfile that Will Deacon wrote in
> > > 8c1fc96f6fd1f361428ba805103af0d0eee65179 ("ARM: 6072/1: oprofile: use
> > > perf-events framework as backend") is of use to more architectures
> > > than just ARM. Move the code into drivers/oprofile/ so that SH can use
> > > it instead of the nearly identical copy of its OProfile code.
> >
> > Nice, I didn't know such a backend already existed.  Now that you
> > have made it generic we should aim towards making it the only oprofile
> > backend and getting rid of all the duplication.
> 
> Even better would be to do the surgery at a higher level and provide the
> oprofile API without the oprofile buffer management. My experience is
> that it doesn't scale, and on heavily threaded large SMP setup, there is
> an enormous amount of time wasted contending on the global oprofile
> buffer mutex.
> 
Ideally, the OProfile userspace tools would talk native perf and we
could do away with oprofilefs and the oprofile buffer altogether.
Compatibility with the old [read current] OProfile tools can be
maintained using these patches until the API is deprecated.

Or is that a bit too optimistic? :)

Will

[PATCH 0/3] Generalise ARM perf-events backend for oprofile

[Benjamin Herrenschmidt]

On Wed, 2010-08-25 at 10:07 +0100, Will Deacon wrote:
> Ideally, the OProfile userspace tools would talk native perf and we
> could do away with oprofilefs and the oprofile buffer altogether.
> Compatibility with the old [read current] OProfile tools can be
> maintained using these patches until the API is deprecated.
> 
> Or is that a bit too optimistic? :) 

Heh, well, dunno, I suspect just moving userspace to native perf tools
is going to happen as soon is not sooner :-)

Ben.

[PATCH 0/3] Generalise ARM perf-events backend for oprofile

[Will Deacon]

Hi Matt,

On Mon, 2010-08-23 at 11:46 +0100, Matt Fleming wrote:
> The perf-events backend for OProfile that Will Deacon wrote in
> 8c1fc96f6fd1f361428ba805103af0d0eee65179 ("ARM: 6072/1: oprofile: use
> perf-events framework as backend") is of use to more architectures
> than just ARM. Move the code into drivers/oprofile/ so that SH can use
> it instead of the nearly identical copy of its OProfile code.
> 
> The benefit of the backend is that it becomes necessary to only
> maintain one copy of the PMU accessor functions for each architecture,
> with bug fixes and new features benefiting both OProfile and perf.
> 
The downside is that it's only really applicable if all the subarch
targets which have OProfile support have equivalent perf support. I know
this is the case for SH and ARM, but I'm not sure about other
architectures.

> Note that I haven't been able to test these patches on an ARM board to
> see if I've caused any regressions. If anyone else could do that I'd
> appreciate it.
> 
I tried to test them but they don't compile:

arch/arm/oprofile/common.c: In function 'oprofile_arch_exit':
arch/arm/oprofile/common.c:234: error: 'perf_events' undeclared (first use in this function)
arch/arm/oprofile/common.c:234: error: (Each undeclared identifier is reported only once
arch/arm/oprofile/common.c:234: error: for each function it appears in.)
arch/arm/oprofile/common.c:237: error: 'perf_num_counters' undeclared (first use in this function)
arch/arm/oprofile/common.c:246: error: 'counter_config' undeclared (first use in this function)

This is because the oprofile_arch_exit implementation for ARM frees
data structures that were previously allocated in oprofile_arch_init.
Since this is now done in op_perf_create_files, I'm not sure where the
freeing should be done. OProfile can be compiled as a module, so this
does need to be implemented somewhere (plus, if oprofile_arch_init fails
oprofile_arch_exit is called immediately). Perhaps an op_perf_exit()
function could be called from the arch code?

Looking at the existing ARM implementation, it's not entirely safe in
the case that oprofile_arch_init fails and needs something like:

diff --git a/arch/arm/oprofile/common.c b/arch/arm/oprofile/common.c
index 0691176..15d379f 100644
--- a/arch/arm/oprofile/common.c
+++ b/arch/arm/oprofile/common.c
@@ -275,10 +275,12 @@ out:
        return ret;
 }
 
-static void  exit_driverfs(void)
+static void __exit exit_driverfs(void)
 {
-       platform_device_unregister(oprofile_pdev);
-       platform_driver_unregister(&oprofile_driver);
+       if (!IS_ERR_OR_NULL(oprofile_pdev)) {
+               platform_device_unregister(oprofile_pdev);
+               platform_driver_unregister(&oprofile_driver);
+       }
 }
 #else
 static int __init init_driverfs(void) { return 0; }
@@ -363,10 +365,8 @@ int __init oprofile_arch_init(struct oprofile_operations *ops)
        }
 
        ret = init_driverfs();
-       if (ret) {
-               kfree(counter_config);
+       if (ret)
                return ret;
-       }
 
        for_each_possible_cpu(cpu) {
                perf_events[cpu] = kcalloc(perf_num_counters,
@@ -396,13 +396,14 @@ int __init oprofile_arch_init(struct oprofile_operations *ops)
        return ret;
 }
 
-void oprofile_arch_exit(void)
+void __exit oprofile_arch_exit(void)
 {
        int cpu, id;
        struct perf_event *event;
 
+       exit_driverfs();
+
        if (*perf_events) {
-               exit_driverfs();
                for_each_possible_cpu(cpu) {
                        for (id = 0; id < perf_num_counters; ++id) {
                                event = perf_events[cpu][id];
@@ -422,5 +423,5 @@ int __init oprofile_arch_init(struct oprofile_operations *ops)
        pr_info("oprofile: hardware counters not available\n");
        return -ENODEV;
 }
-void oprofile_arch_exit(void) {}
+void __exit oprofile_arch_exit(void) {}
 #endif /* CONFIG_HW_PERF_EVENTS */


I can submit this as a separate patch or you can fold it into your changes
to avoid any conflicts.

Cheers,

Will

[PATCH 0/3] Generalise ARM perf-events backend for oprofile

[Matt Fleming]

On Mon, Aug 23, 2010 at 04:51:17PM +0100, Will Deacon wrote:
> Hi Matt,
> 
> On Mon, 2010-08-23 at 11:46 +0100, Matt Fleming wrote:
> > The perf-events backend for OProfile that Will Deacon wrote in
> > 8c1fc96f6fd1f361428ba805103af0d0eee65179 ("ARM: 6072/1: oprofile: use
> > perf-events framework as backend") is of use to more architectures
> > than just ARM. Move the code into drivers/oprofile/ so that SH can use
> > it instead of the nearly identical copy of its OProfile code.
> > 
> > The benefit of the backend is that it becomes necessary to only
> > maintain one copy of the PMU accessor functions for each architecture,
> > with bug fixes and new features benefiting both OProfile and perf.
> > 
> The downside is that it's only really applicable if all the subarch
> targets which have OProfile support have equivalent perf support. I know
> this is the case for SH and ARM, but I'm not sure about other
> architectures.

Sure. This doesn't have to be a flag day. Architectures can move over if and
when they're ready. I haven't looked very closely at any other architectures
but I'm sure some of them could make use of this series.

> > Note that I haven't been able to test these patches on an ARM board to
> > see if I've caused any regressions. If anyone else could do that I'd
> > appreciate it.
> > 
> I tried to test them but they don't compile:
> 
> arch/arm/oprofile/common.c: In function 'oprofile_arch_exit':
> arch/arm/oprofile/common.c:234: error: 'perf_events' undeclared (first use in this function)
> arch/arm/oprofile/common.c:234: error: (Each undeclared identifier is reported only once
> arch/arm/oprofile/common.c:234: error: for each function it appears in.)
> arch/arm/oprofile/common.c:237: error: 'perf_num_counters' undeclared (first use in this function)
> arch/arm/oprofile/common.c:246: error: 'counter_config' undeclared (first use in this function)
> 
> This is because the oprofile_arch_exit implementation for ARM frees
> data structures that were previously allocated in oprofile_arch_init.
> Since this is now done in op_perf_create_files, I'm not sure where the
> freeing should be done. OProfile can be compiled as a module, so this
> does need to be implemented somewhere (plus, if oprofile_arch_init fails
> oprofile_arch_exit is called immediately). Perhaps an op_perf_exit()
> function could be called from the arch code?

Eek! I totally messed this up, sorry. Thanks very much for compiling
these and reviewing them. I've just grabbed an ARM toolchain so I'll
compile the next version before I post it ;-)

You've highlighted a good point - the allocation and freeing is done in
the wrong places. We need a function in drivers/oprofile/oprofile_perf.c
that is called from oprofile_arch_init() that allocates the
'counter_config' and 'perf_events[]' data structures. These can then be
freed by a similiar function in oprofile_arch_exit().

> Looking at the existing ARM implementation, it's not entirely safe in
> the case that oprofile_arch_init fails and needs something like:
> 
> diff --git a/arch/arm/oprofile/common.c b/arch/arm/oprofile/common.c
> index 0691176..15d379f 100644
> --- a/arch/arm/oprofile/common.c
> +++ b/arch/arm/oprofile/common.c
> @@ -275,10 +275,12 @@ out:
>         return ret;
>  }
>  
> -static void  exit_driverfs(void)
> +static void __exit exit_driverfs(void)
>  {
> -       platform_device_unregister(oprofile_pdev);
> -       platform_driver_unregister(&oprofile_driver);
> +       if (!IS_ERR_OR_NULL(oprofile_pdev)) {
> +               platform_device_unregister(oprofile_pdev);
> +               platform_driver_unregister(&oprofile_driver);
> +       }
>  }
>  #else
>  static int __init init_driverfs(void) { return 0; }
> @@ -363,10 +365,8 @@ int __init oprofile_arch_init(struct oprofile_operations *ops)
>         }
>  
>         ret = init_driverfs();
> -       if (ret) {
> -               kfree(counter_config);
> +       if (ret)
>                 return ret;
> -       }
>  
>         for_each_possible_cpu(cpu) {
>                 perf_events[cpu] = kcalloc(perf_num_counters,
> @@ -396,13 +396,14 @@ int __init oprofile_arch_init(struct oprofile_operations *ops)
>         return ret;
>  }
>  
> -void oprofile_arch_exit(void)
> +void __exit oprofile_arch_exit(void)
>  {
>         int cpu, id;
>         struct perf_event *event;
>  
> +       exit_driverfs();
> +
>         if (*perf_events) {
> -               exit_driverfs();
>                 for_each_possible_cpu(cpu) {
>                         for (id = 0; id < perf_num_counters; ++id) {
>                                 event = perf_events[cpu][id];
> @@ -422,5 +423,5 @@ int __init oprofile_arch_init(struct oprofile_operations *ops)
>         pr_info("oprofile: hardware counters not available\n");
>         return -ENODEV;
>  }
> -void oprofile_arch_exit(void) {}
> +void __exit oprofile_arch_exit(void) {}
>  #endif /* CONFIG_HW_PERF_EVENTS */
> 
> 
> I can submit this as a separate patch or you can fold it into your changes
> to avoid any conflicts.

Ah, I see what you mean. I'll fold this change into my series to avoid
conflicts, but as a separate patch. I'll retain your authorship in the
commit just be sure to check it when I send out V2 of this series to
make sure I haven't messed your patch up.

Thanks for the review!