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

[PATCH V3 0/6] 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. Though, I have been able to compile this version of the
series.

This patch series is based on,

    git://git.kernel.org/pub/scm/linux/kernel/git/rric/oprofile.git core

These patches can also be found at,

    git://git.kernel.org/pub/scm/linux/kernel/git/mfleming/sh-2.6.git perf-oprofile

Changes from v2:
 - Rebased against Robert's oprofile core branch
 - Moved even more of the ARM code into the generic oprofile code
 - Broke the patches up into more logical steps

Changes from v1:
 - Prefix the new functons with "oprofile_" instead of "op_".
 - Fix ARM compilation errors
 - Move all the oprofile-perf logic into oprofile_perf.c
 - Include cleanup patch from Will

Matt Fleming (6):
  perf: Add helper function to return number of counters
  ARM: oprofile: Rename op_arm to oprofile_perf
  ARM: oprofile: Move non-ARM code into separate init/exit functions
  oprofile: Abstract the perf-events backend
  ARM: Make oprofile depend on CONFIG_HW_PERF_EVENTS
  sh: oprofile: Use perf-events oprofile backend

 arch/arm/Kconfig                 |    2 +-
 arch/arm/kernel/perf_event.c     |    6 +
 arch/arm/oprofile/Makefile       |    2 +-
 arch/arm/oprofile/common.c       |  315 +------------------------------------
 arch/sh/Kconfig                  |    2 +-
 arch/sh/kernel/perf_event.c      |   17 ++
 arch/sh/oprofile/Makefile        |    2 +-
 arch/sh/oprofile/common.c        |  104 ++-----------
 arch/sh/oprofile/op_impl.h       |   33 ----
 drivers/oprofile/oprofile_perf.c |  322 ++++++++++++++++++++++++++++++++++++++
 include/linux/oprofile.h         |    6 +
 include/linux/perf_event.h       |    1 +
 12 files changed, 368 insertions(+), 444 deletions(-)
 delete mode 100644 arch/sh/oprofile/op_impl.h
 create mode 100644 drivers/oprofile/oprofile_perf.c

[PATCH 1/6] perf: Add helper function to return number of counters

[Matt Fleming]

The number of counters for the registered pmu is needed in a few places
so provide a helper function that returns this number.

Signed-off-by: Matt Fleming <matt@console-pimps.org>
---
 arch/arm/kernel/perf_event.c |    6 ++++++
 arch/arm/oprofile/common.c   |   25 +++++++++++--------------
 arch/sh/kernel/perf_event.c  |    9 +++++++++
 include/linux/perf_event.h   |    1 +
 4 files changed, 27 insertions(+), 14 deletions(-)

diff --git a/arch/arm/kernel/perf_event.c b/arch/arm/kernel/perf_event.c
index 417c392..3b0aedf 100644
--- a/arch/arm/kernel/perf_event.c
+++ b/arch/arm/kernel/perf_event.c
@@ -123,6 +123,12 @@ armpmu_get_max_events(void)
 }
 EXPORT_SYMBOL_GPL(armpmu_get_max_events);
 
+int perf_num_counters(void)
+{
+	return armpmu_get_max_events();
+}
+EXPORT_SYMBOL_GPL(perf_num_counters);
+
 #define HW_OP_UNSUPPORTED		0xFFFF
 
 #define C(_x) \
diff --git a/arch/arm/oprofile/common.c b/arch/arm/oprofile/common.c
index d660cb8..6498e58 100644
--- a/arch/arm/oprofile/common.c
+++ b/arch/arm/oprofile/common.c
@@ -43,7 +43,6 @@ 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.
@@ -54,11 +53,11 @@ static void op_overflow_handler(struct perf_event *event, int unused,
 	int id;
 	u32 cpu = smp_processor_id();
 
-	for (id = 0; id < perf_num_counters; ++id)
+	for (id = 0; id < perf_num_counters(); ++id)
 		if (perf_events[cpu][id] == event)
 			break;
 
-	if (id != perf_num_counters)
+	if (id != perf_num_counters())
 		oprofile_add_sample(regs, id);
 	else
 		pr_warning("oprofile: ignoring spurious overflow "
@@ -76,7 +75,7 @@ static void op_perf_setup(void)
 	u32 size = sizeof(struct perf_event_attr);
 	struct perf_event_attr *attr;
 
-	for (i = 0; i < perf_num_counters; ++i) {
+	for (i = 0; i < perf_num_counters(); ++i) {
 		attr = &counter_config[i].attr;
 		memset(attr, 0, size);
 		attr->type		= PERF_TYPE_RAW;
@@ -131,7 +130,7 @@ static int op_perf_start(void)
 	int cpu, event, ret = 0;
 
 	for_each_online_cpu(cpu) {
-		for (event = 0; event < perf_num_counters; ++event) {
+		for (event = 0; event < perf_num_counters(); ++event) {
 			ret = op_create_counter(cpu, event);
 			if (ret)
 				goto out;
@@ -150,7 +149,7 @@ static void op_perf_stop(void)
 	int cpu, event;
 
 	for_each_online_cpu(cpu)
-		for (event = 0; event < perf_num_counters; ++event)
+		for (event = 0; event < perf_num_counters(); ++event)
 			op_destroy_counter(cpu, event);
 }
 
@@ -179,7 +178,7 @@ static int op_arm_create_files(struct super_block *sb, struct dentry *root)
 {
 	unsigned int i;
 
-	for (i = 0; i < perf_num_counters; i++) {
+	for (i = 0; i < perf_num_counters(); i++) {
 		struct dentry *dir;
 		char buf[4];
 
@@ -353,14 +352,12 @@ int __init oprofile_arch_init(struct oprofile_operations *ops)
 
 	memset(&perf_events, 0, sizeof(perf_events));
 
-	perf_num_counters = armpmu_get_max_events();
-
-	counter_config = kcalloc(perf_num_counters,
+	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);
+				"counters\n", perf_num_counters());
 		ret = -ENOMEM;
 		goto out;
 	}
@@ -370,11 +367,11 @@ int __init oprofile_arch_init(struct oprofile_operations *ops)
 		goto out;
 
 	for_each_possible_cpu(cpu) {
-		perf_events[cpu] = kcalloc(perf_num_counters,
+		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);
+					"for cpu %d\n", perf_num_counters(), cpu);
 			ret = -ENOMEM;
 			goto out;
 		}
@@ -409,7 +406,7 @@ void __exit oprofile_arch_exit(void)
 	struct perf_event *event;
 
 	for_each_possible_cpu(cpu) {
-		for (id = 0; id < perf_num_counters; ++id) {
+		for (id = 0; id < perf_num_counters(); ++id) {
 			event = perf_events[cpu][id];
 			if (event)
 				perf_event_release_kernel(event);
diff --git a/arch/sh/kernel/perf_event.c b/arch/sh/kernel/perf_event.c
index 7a3dc35..2cb9ad5 100644
--- a/arch/sh/kernel/perf_event.c
+++ b/arch/sh/kernel/perf_event.c
@@ -59,6 +59,15 @@ static inline int sh_pmu_initialized(void)
 	return !!sh_pmu;
 }
 
+int perf_num_counters(void)
+{
+	if (!sh_pmu)
+		return 0;
+
+	return sh_pmu->num_events;
+}
+EXPORT_SYMBOL_GPL(perf_num_counters);
+
 /*
  * Release the PMU if this is the last perf_event.
  */
diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 716f99b..1a02192 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -849,6 +849,7 @@ extern int perf_max_events;
 
 extern const struct pmu *hw_perf_event_init(struct perf_event *event);
 
+extern int perf_num_counters(void);
 extern void perf_event_task_sched_in(struct task_struct *task);
 extern void perf_event_task_sched_out(struct task_struct *task, struct task_struct *next);
 extern void perf_event_task_tick(struct task_struct *task);
-- 
1.7.1

[PATCH 1/6] perf: Add helper function to return number of counters

[Robert Richter]

On 13.09.10 02:07:32, Matt Fleming wrote:
> The number of counters for the registered pmu is needed in a few places
> so provide a helper function that returns this number.
> 
> Signed-off-by: Matt Fleming <matt@console-pimps.org>
> ---
>  arch/arm/kernel/perf_event.c |    6 ++++++
>  arch/arm/oprofile/common.c   |   25 +++++++++++--------------
>  arch/sh/kernel/perf_event.c  |    9 +++++++++
>  include/linux/perf_event.h   |    1 +
>  4 files changed, 27 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/arm/kernel/perf_event.c b/arch/arm/kernel/perf_event.c
> index 417c392..3b0aedf 100644
> --- a/arch/arm/kernel/perf_event.c
> +++ b/arch/arm/kernel/perf_event.c
> @@ -123,6 +123,12 @@ armpmu_get_max_events(void)
>  }
>  EXPORT_SYMBOL_GPL(armpmu_get_max_events);
>  
> +int perf_num_counters(void)
> +{
> +	return armpmu_get_max_events();
> +}
> +EXPORT_SYMBOL_GPL(perf_num_counters);
> +
>  #define HW_OP_UNSUPPORTED		0xFFFF
>  
>  #define C(_x) \
> diff --git a/arch/arm/oprofile/common.c b/arch/arm/oprofile/common.c
> index d660cb8..6498e58 100644
> --- a/arch/arm/oprofile/common.c
> +++ b/arch/arm/oprofile/common.c
> @@ -43,7 +43,6 @@ 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.
> @@ -54,11 +53,11 @@ static void op_overflow_handler(struct perf_event *event, int unused,
>  	int id;
>  	u32 cpu = smp_processor_id();
>  
> -	for (id = 0; id < perf_num_counters; ++id)
> +	for (id = 0; id < perf_num_counters(); ++id)
>  		if (perf_events[cpu][id] == event)
>  			break;
>  
> -	if (id != perf_num_counters)
> +	if (id != perf_num_counters())
>  		oprofile_add_sample(regs, id);
>  	else
>  		pr_warning("oprofile: ignoring spurious overflow "
> @@ -76,7 +75,7 @@ static void op_perf_setup(void)
>  	u32 size = sizeof(struct perf_event_attr);
>  	struct perf_event_attr *attr;
>  
> -	for (i = 0; i < perf_num_counters; ++i) {
> +	for (i = 0; i < perf_num_counters(); ++i) {
>  		attr = &counter_config[i].attr;
>  		memset(attr, 0, size);
>  		attr->type		= PERF_TYPE_RAW;
> @@ -131,7 +130,7 @@ static int op_perf_start(void)
>  	int cpu, event, ret = 0;
>  
>  	for_each_online_cpu(cpu) {
> -		for (event = 0; event < perf_num_counters; ++event) {
> +		for (event = 0; event < perf_num_counters(); ++event) {
>  			ret = op_create_counter(cpu, event);
>  			if (ret)
>  				goto out;
> @@ -150,7 +149,7 @@ static void op_perf_stop(void)
>  	int cpu, event;
>  
>  	for_each_online_cpu(cpu)
> -		for (event = 0; event < perf_num_counters; ++event)
> +		for (event = 0; event < perf_num_counters(); ++event)
>  			op_destroy_counter(cpu, event);
>  }
>  
> @@ -179,7 +178,7 @@ static int op_arm_create_files(struct super_block *sb, struct dentry *root)
>  {
>  	unsigned int i;
>  
> -	for (i = 0; i < perf_num_counters; i++) {
> +	for (i = 0; i < perf_num_counters(); i++) {
>  		struct dentry *dir;
>  		char buf[4];
>  
> @@ -353,14 +352,12 @@ int __init oprofile_arch_init(struct oprofile_operations *ops)
>  
>  	memset(&perf_events, 0, sizeof(perf_events));
>  
> -	perf_num_counters = armpmu_get_max_events();
> -

Maybe we keep this variable and only call perf_num_counters() once
during initialization as the function's return value should be
constant.  Otherwise this will cause unnecessary cache pollution and
execution cycles esp. in the overflow handler.

Also it wouldn't be safe if the value changes anyway, for example you
enable 4 counters and and after the value changed you disable only 3.

Module initialization should fail on errors or if no counters are
available.

-Robert

> -	counter_config = kcalloc(perf_num_counters,
> +	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);
> +				"counters\n", perf_num_counters());
>  		ret = -ENOMEM;
>  		goto out;
>  	}
> @@ -370,11 +367,11 @@ int __init oprofile_arch_init(struct oprofile_operations *ops)
>  		goto out;
>  
>  	for_each_possible_cpu(cpu) {
> -		perf_events[cpu] = kcalloc(perf_num_counters,
> +		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);
> +					"for cpu %d\n", perf_num_counters(), cpu);
>  			ret = -ENOMEM;
>  			goto out;
>  		}
> @@ -409,7 +406,7 @@ void __exit oprofile_arch_exit(void)
>  	struct perf_event *event;
>  
>  	for_each_possible_cpu(cpu) {
> -		for (id = 0; id < perf_num_counters; ++id) {
> +		for (id = 0; id < perf_num_counters(); ++id) {
>  			event = perf_events[cpu][id];
>  			if (event)
>  				perf_event_release_kernel(event);
> diff --git a/arch/sh/kernel/perf_event.c b/arch/sh/kernel/perf_event.c
> index 7a3dc35..2cb9ad5 100644
> --- a/arch/sh/kernel/perf_event.c
> +++ b/arch/sh/kernel/perf_event.c
> @@ -59,6 +59,15 @@ static inline int sh_pmu_initialized(void)
>  	return !!sh_pmu;
>  }
>  
> +int perf_num_counters(void)
> +{
> +	if (!sh_pmu)
> +		return 0;
> +
> +	return sh_pmu->num_events;
> +}
> +EXPORT_SYMBOL_GPL(perf_num_counters);
> +
>  /*
>   * Release the PMU if this is the last perf_event.
>   */
> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index 716f99b..1a02192 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -849,6 +849,7 @@ extern int perf_max_events;
>  
>  extern const struct pmu *hw_perf_event_init(struct perf_event *event);
>  
> +extern int perf_num_counters(void);
>  extern void perf_event_task_sched_in(struct task_struct *task);
>  extern void perf_event_task_sched_out(struct task_struct *task, struct task_struct *next);
>  extern void perf_event_task_tick(struct task_struct *task);
> -- 
> 1.7.1
> 
> 

-- 
Advanced Micro Devices, Inc.
Operating System Research Center

[PATCH 2/6] ARM: oprofile: Rename op_arm to oprofile_perf

[Matt Fleming]

In preparation for moving the generic functions out of this file, give
the functions more general names (e.g. remove "arm" from the names).

Signed-off-by: Matt Fleming <matt@console-pimps.org>
---
 arch/arm/oprofile/common.c |   68 ++++++++++++++++++++++----------------------
 1 files changed, 34 insertions(+), 34 deletions(-)

diff --git a/arch/arm/oprofile/common.c b/arch/arm/oprofile/common.c
index 6498e58..ed8291a 100644
--- a/arch/arm/oprofile/common.c
+++ b/arch/arm/oprofile/common.c
@@ -38,8 +38,8 @@ struct op_counter_config {
 	struct perf_event_attr attr;
 };
 
-static int op_arm_enabled;
-static DEFINE_MUTEX(op_arm_mutex);
+static int oprofile_perf_enabled;
+static DEFINE_MUTEX(oprofile_perf_mutex);
 
 static struct op_counter_config *counter_config;
 static struct perf_event **perf_events[nr_cpumask_bits];
@@ -65,7 +65,7 @@ static void op_overflow_handler(struct perf_event *event, int unused,
 }
 
 /*
- * Called by op_arm_setup to create perf attributes to mirror the oprofile
+ * Called by oprofile_perf_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.
  */
@@ -122,7 +122,7 @@ static void op_destroy_counter(int cpu, int event)
 }
 
 /*
- * Called by op_arm_start to create active perf events based on the
+ * Called by oprofile_perf_start to create active perf events based on the
  * perviously configured attributes.
  */
 static int op_perf_start(void)
@@ -142,7 +142,7 @@ out:
 }
 
 /*
- * Called by op_arm_stop at the end of a profiling run.
+ * Called by oprofile_perf_stop at the end of a profiling run.
  */
 static void op_perf_stop(void)
 {
@@ -174,7 +174,7 @@ 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)
+static int oprofile_perf_create_files(struct super_block *sb, struct dentry *root)
 {
 	unsigned int i;
 
@@ -195,7 +195,7 @@ static int op_arm_create_files(struct super_block *sb, struct dentry *root)
 	return 0;
 }
 
-static int op_arm_setup(void)
+static int oprofile_perf_setup(void)
 {
 	spin_lock(&oprofilefs_lock);
 	op_perf_setup();
@@ -203,54 +203,54 @@ static int op_arm_setup(void)
 	return 0;
 }
 
-static int op_arm_start(void)
+static int oprofile_perf_start(void)
 {
 	int ret = -EBUSY;
 
-	mutex_lock(&op_arm_mutex);
-	if (!op_arm_enabled) {
+	mutex_lock(&oprofile_perf_mutex);
+	if (!oprofile_perf_enabled) {
 		ret = 0;
 		op_perf_start();
-		op_arm_enabled = 1;
+		oprofile_perf_enabled = 1;
 	}
-	mutex_unlock(&op_arm_mutex);
+	mutex_unlock(&oprofile_perf_mutex);
 	return ret;
 }
 
-static void op_arm_stop(void)
+static void oprofile_perf_stop(void)
 {
-	mutex_lock(&op_arm_mutex);
-	if (op_arm_enabled)
+	mutex_lock(&oprofile_perf_mutex);
+	if (oprofile_perf_enabled)
 		op_perf_stop();
-	op_arm_enabled = 0;
-	mutex_unlock(&op_arm_mutex);
+	oprofile_perf_enabled = 0;
+	mutex_unlock(&oprofile_perf_mutex);
 }
 
 #ifdef CONFIG_PM
-static int op_arm_suspend(struct platform_device *dev, pm_message_t state)
+static int oprofile_perf_suspend(struct platform_device *dev, pm_message_t state)
 {
-	mutex_lock(&op_arm_mutex);
-	if (op_arm_enabled)
+	mutex_lock(&oprofile_perf_mutex);
+	if (oprofile_perf_enabled)
 		op_perf_stop();
-	mutex_unlock(&op_arm_mutex);
+	mutex_unlock(&oprofile_perf_mutex);
 	return 0;
 }
 
-static int op_arm_resume(struct platform_device *dev)
+static int oprofile_perf_resume(struct platform_device *dev)
 {
-	mutex_lock(&op_arm_mutex);
-	if (op_arm_enabled && op_perf_start())
-		op_arm_enabled = 0;
-	mutex_unlock(&op_arm_mutex);
+	mutex_lock(&oprofile_perf_mutex);
+	if (oprofile_perf_enabled && op_perf_start())
+		oprofile_perf_enabled = 0;
+	mutex_unlock(&oprofile_perf_mutex);
 	return 0;
 }
 
 static struct platform_driver oprofile_driver = {
 	.driver		= {
-		.name		= "arm-oprofile",
+		.name		= "perf-oprofile",
 	},
-	.resume		= op_arm_resume,
-	.suspend	= op_arm_suspend,
+	.resume		= oprofile_perf_resume,
+	.suspend	= oprofile_perf_suspend,
 };
 
 static struct platform_device *oprofile_pdev;
@@ -378,11 +378,11 @@ int __init oprofile_arch_init(struct oprofile_operations *ops)
 	}
 
 	ops->backtrace		= arm_backtrace;
-	ops->create_files	= op_arm_create_files;
-	ops->setup		= op_arm_setup;
-	ops->start		= op_arm_start;
-	ops->stop		= op_arm_stop;
-	ops->shutdown		= op_arm_stop;
+	ops->create_files	= oprofile_perf_create_files;
+	ops->setup		= oprofile_perf_setup;
+	ops->start		= oprofile_perf_start;
+	ops->stop		= oprofile_perf_stop;
+	ops->shutdown		= oprofile_perf_stop;
 	ops->cpu_type		= op_name_from_perf_id(armpmu_get_pmu_id());
 
 	if (!ops->cpu_type)
-- 
1.7.1

[PATCH 2/6] ARM: oprofile: Rename op_arm to oprofile_perf

[Robert Richter]

On 13.09.10 02:07:33, Matt Fleming wrote:
> In preparation for moving the generic functions out of this file, give
> the functions more general names (e.g. remove "arm" from the names).
> 
> Signed-off-by: Matt Fleming <matt@console-pimps.org>
> ---
>  arch/arm/oprofile/common.c |   68 ++++++++++++++++++++++----------------------
>  1 files changed, 34 insertions(+), 34 deletions(-)

>  static struct platform_driver oprofile_driver = {
>  	.driver		= {
> -		.name		= "arm-oprofile",
> +		.name		= "perf-oprofile",

This should be "oprofile-perf" instead, otherwise the driver belongs
to perf.

-Robert

>  	},
> -	.resume		= op_arm_resume,
> -	.suspend	= op_arm_suspend,
> +	.resume		= oprofile_perf_resume,
> +	.suspend	= oprofile_perf_suspend,
>  };

-- 
Advanced Micro Devices, Inc.
Operating System Research Center

[PATCH 3/6] ARM: oprofile: Move non-ARM code into separate init/exit functions

[Matt Fleming]

In preparation for moving the majority of this oprofile code into an
architecture-neutral place separate the architecture-independent code
into oprofile_perf_init() and oprofile_perf_exit().

Signed-off-by: Matt Fleming <matt@console-pimps.org>
---
 arch/arm/oprofile/common.c |   34 +++++++++++++++++++++++++---------
 1 files changed, 25 insertions(+), 9 deletions(-)

diff --git a/arch/arm/oprofile/common.c b/arch/arm/oprofile/common.c
index ed8291a..4d814c3 100644
--- a/arch/arm/oprofile/common.c
+++ b/arch/arm/oprofile/common.c
@@ -346,7 +346,7 @@ static void arm_backtrace(struct pt_regs * const regs, unsigned int depth)
 		tail = user_backtrace(tail);
 }
 
-int __init oprofile_arch_init(struct oprofile_operations *ops)
+int __init oprofile_perf_init(struct oprofile_operations *ops)
 {
 	int cpu, ret = 0;
 
@@ -377,13 +377,16 @@ int __init oprofile_arch_init(struct oprofile_operations *ops)
 		}
 	}
 
-	ops->backtrace		= arm_backtrace;
-	ops->create_files	= oprofile_perf_create_files;
-	ops->setup		= oprofile_perf_setup;
-	ops->start		= oprofile_perf_start;
-	ops->stop		= oprofile_perf_stop;
-	ops->shutdown		= oprofile_perf_stop;
-	ops->cpu_type		= op_name_from_perf_id(armpmu_get_pmu_id());
+	if (!ops->create_files)
+		ops->create_files = oprofile_perf_create_files;
+	if (!ops->setup)
+		ops->setup = oprofile_perf_setup;
+	if (!ops->start)
+		ops->start = oprofile_perf_start;
+	if (!ops->stop)
+		ops->stop = oprofile_perf_stop;
+	if (!ops->shutdown)
+		ops->shutdown = oprofile_perf_stop;
 
 	if (!ops->cpu_type)
 		ret = -ENODEV;
@@ -400,7 +403,15 @@ out:
 	return ret;
 }
 
-void __exit oprofile_arch_exit(void)
+int __init oprofile_arch_init(struct oprofile_operations *ops)
+{
+	ops->backtrace		= arm_backtrace;
+	ops->cpu_type		= op_name_from_perf_id(armpmu_get_pmu_id());
+
+	return oprofile_perf_init(ops);
+}
+
+void __exit oprofile_perf_exit(void)
 {
 	int cpu, id;
 	struct perf_event *event;
@@ -418,6 +429,11 @@ void __exit oprofile_arch_exit(void)
 	kfree(counter_config);
 	exit_driverfs();
 }
+
+void __exit oprofile_arch_exit(void)
+{
+	oprofile_perf_exit();
+}
 #else
 int __init oprofile_arch_init(struct oprofile_operations *ops)
 {
-- 
1.7.1

[PATCH 3/6] ARM: oprofile: Move non-ARM code into separate init/exit functions

[Robert Richter]

On 13.09.10 02:07:34, Matt Fleming wrote:
> In preparation for moving the majority of this oprofile code into an
> architecture-neutral place separate the architecture-independent code
> into oprofile_perf_init() and oprofile_perf_exit().
> 
> Signed-off-by: Matt Fleming <matt@console-pimps.org>
> ---
>  arch/arm/oprofile/common.c |   34 +++++++++++++++++++++++++---------
>  1 files changed, 25 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/arm/oprofile/common.c b/arch/arm/oprofile/common.c
> index ed8291a..4d814c3 100644
> --- a/arch/arm/oprofile/common.c
> +++ b/arch/arm/oprofile/common.c
> @@ -346,7 +346,7 @@ static void arm_backtrace(struct pt_regs * const regs, unsigned int depth)
>  		tail = user_backtrace(tail);
>  }
>  
> -int __init oprofile_arch_init(struct oprofile_operations *ops)
> +int __init oprofile_perf_init(struct oprofile_operations *ops)
>  {
>  	int cpu, ret = 0;
>  
> @@ -377,13 +377,16 @@ int __init oprofile_arch_init(struct oprofile_operations *ops)
>  		}
>  	}
>  
> -	ops->backtrace		= arm_backtrace;
> -	ops->create_files	= oprofile_perf_create_files;
> -	ops->setup		= oprofile_perf_setup;
> -	ops->start		= oprofile_perf_start;
> -	ops->stop		= oprofile_perf_stop;
> -	ops->shutdown		= oprofile_perf_stop;
> -	ops->cpu_type		= op_name_from_perf_id(armpmu_get_pmu_id());
> +	if (!ops->create_files)
> +		ops->create_files = oprofile_perf_create_files;
> +	if (!ops->setup)
> +		ops->setup = oprofile_perf_setup;
> +	if (!ops->start)
> +		ops->start = oprofile_perf_start;
> +	if (!ops->stop)
> +		ops->stop = oprofile_perf_stop;
> +	if (!ops->shutdown)
> +		ops->shutdown = oprofile_perf_stop;

I rather tend to not check this and instand hard overwrite it. Do you
have something in mind why we have checks here? I mean, if something
must be customizable, the arch init could overwrite it after doing
perf init.

-Robert

>  
>  	if (!ops->cpu_type)
>  		ret = -ENODEV;
> @@ -400,7 +403,15 @@ out:
>  	return ret;
>  }
>  
> -void __exit oprofile_arch_exit(void)
> +int __init oprofile_arch_init(struct oprofile_operations *ops)
> +{
> +	ops->backtrace		= arm_backtrace;
> +	ops->cpu_type		= op_name_from_perf_id(armpmu_get_pmu_id());
> +
> +	return oprofile_perf_init(ops);
> +}
> +
> +void __exit oprofile_perf_exit(void)
>  {
>  	int cpu, id;
>  	struct perf_event *event;
> @@ -418,6 +429,11 @@ void __exit oprofile_arch_exit(void)
>  	kfree(counter_config);
>  	exit_driverfs();
>  }
> +
> +void __exit oprofile_arch_exit(void)
> +{
> +	oprofile_perf_exit();
> +}
>  #else
>  int __init oprofile_arch_init(struct oprofile_operations *ops)
>  {
> -- 
> 1.7.1
> 
> 

-- 
Advanced Micro Devices, Inc.
Operating System Research Center

[PATCH 4/6] 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       |    2 +-
 arch/arm/oprofile/common.c       |  315 -------------------------------------
 drivers/oprofile/oprofile_perf.c |  322 ++++++++++++++++++++++++++++++++++++++
 include/linux/oprofile.h         |    6 +
 4 files changed, 329 insertions(+), 316 deletions(-)
 create mode 100644 drivers/oprofile/oprofile_perf.c

diff --git a/arch/arm/oprofile/Makefile b/arch/arm/oprofile/Makefile
index e666eaf..07c93f6 100644
--- a/arch/arm/oprofile/Makefile
+++ b/arch/arm/oprofile/Makefile
@@ -4,6 +4,6 @@ DRIVER_OBJS = $(addprefix ../../../drivers/oprofile/, \
 		oprof.o cpu_buffer.o buffer_sync.o \
 		event_buffer.o oprofile_files.o \
 		oprofilefs.o oprofile_stats.o \
-		timer_int.o )
+		timer_int.o oprofile_perf.o )
 
 oprofile-y				:= $(DRIVER_OBJS) common.o
diff --git a/arch/arm/oprofile/common.c b/arch/arm/oprofile/common.c
index 4d814c3..9a3ed26 100644
--- a/arch/arm/oprofile/common.c
+++ b/arch/arm/oprofile/common.c
@@ -25,135 +25,6 @@
 #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 oprofile_perf_enabled;
-static DEFINE_MUTEX(oprofile_perf_mutex);
-
-static struct op_counter_config *counter_config;
-static struct perf_event **perf_events[nr_cpumask_bits];
-
-/*
- * 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 oprofile_perf_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 oprofile_perf_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 oprofile_perf_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) {
@@ -174,116 +45,6 @@ static char *op_name_from_perf_id(enum arm_perf_pmu_ids id)
 	}
 }
 
-static int oprofile_perf_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 oprofile_perf_setup(void)
-{
-	spin_lock(&oprofilefs_lock);
-	op_perf_setup();
-	spin_unlock(&oprofilefs_lock);
-	return 0;
-}
-
-static int oprofile_perf_start(void)
-{
-	int ret = -EBUSY;
-
-	mutex_lock(&oprofile_perf_mutex);
-	if (!oprofile_perf_enabled) {
-		ret = 0;
-		op_perf_start();
-		oprofile_perf_enabled = 1;
-	}
-	mutex_unlock(&oprofile_perf_mutex);
-	return ret;
-}
-
-static void oprofile_perf_stop(void)
-{
-	mutex_lock(&oprofile_perf_mutex);
-	if (oprofile_perf_enabled)
-		op_perf_stop();
-	oprofile_perf_enabled = 0;
-	mutex_unlock(&oprofile_perf_mutex);
-}
-
-#ifdef CONFIG_PM
-static int oprofile_perf_suspend(struct platform_device *dev, pm_message_t state)
-{
-	mutex_lock(&oprofile_perf_mutex);
-	if (oprofile_perf_enabled)
-		op_perf_stop();
-	mutex_unlock(&oprofile_perf_mutex);
-	return 0;
-}
-
-static int oprofile_perf_resume(struct platform_device *dev)
-{
-	mutex_lock(&oprofile_perf_mutex);
-	if (oprofile_perf_enabled && op_perf_start())
-		oprofile_perf_enabled = 0;
-	mutex_unlock(&oprofile_perf_mutex);
-	return 0;
-}
-
-static struct platform_driver oprofile_driver = {
-	.driver		= {
-		.name		= "perf-oprofile",
-	},
-	.resume		= oprofile_perf_resume,
-	.suspend	= oprofile_perf_suspend,
-};
-
-static struct platform_device *oprofile_pdev;
-
-static int __init init_driverfs(void)
-{
-	int ret;
-
-	ret = platform_driver_register(&oprofile_driver);
-	if (ret)
-		goto out;
-
-	oprofile_pdev =	platform_device_register_simple(
-				oprofile_driver.driver.name, 0, NULL, 0);
-	if (IS_ERR(oprofile_pdev)) {
-		ret = PTR_ERR(oprofile_pdev);
-		platform_driver_unregister(&oprofile_driver);
-	}
-
-out:
-	return ret;
-}
-
-static void __exit exit_driverfs(void)
-{
-	platform_device_unregister(oprofile_pdev);
-	platform_driver_unregister(&oprofile_driver);
-}
-#else
-static int __init init_driverfs(void) { return 0; }
-#define exit_driverfs() do { } while (0)
-#endif /* CONFIG_PM */
-
 static int report_trace(struct stackframe *frame, void *d)
 {
 	unsigned int *depth = d;
@@ -346,63 +107,6 @@ static void arm_backtrace(struct pt_regs * const regs, unsigned int depth)
 		tail = user_backtrace(tail);
 }
 
-int __init oprofile_perf_init(struct oprofile_operations *ops)
-{
-	int cpu, ret = 0;
-
-	memset(&perf_events, 0, sizeof(perf_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());
-		ret = -ENOMEM;
-		goto out;
-	}
-
-	ret = init_driverfs();
-	if (ret)
-		goto out;
-
-	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);
-			ret = -ENOMEM;
-			goto out;
-		}
-	}
-
-	if (!ops->create_files)
-		ops->create_files = oprofile_perf_create_files;
-	if (!ops->setup)
-		ops->setup = oprofile_perf_setup;
-	if (!ops->start)
-		ops->start = oprofile_perf_start;
-	if (!ops->stop)
-		ops->stop = oprofile_perf_stop;
-	if (!ops->shutdown)
-		ops->shutdown = oprofile_perf_stop;
-
-	if (!ops->cpu_type)
-		ret = -ENODEV;
-	else
-		pr_info("oprofile: using %s\n", ops->cpu_type);
-
-out:
-	if (ret) {
-		for_each_possible_cpu(cpu)
-			kfree(perf_events[cpu]);
-		kfree(counter_config);
-	}
-
-	return ret;
-}
-
 int __init oprofile_arch_init(struct oprofile_operations *ops)
 {
 	ops->backtrace		= arm_backtrace;
@@ -411,25 +115,6 @@ int __init oprofile_arch_init(struct oprofile_operations *ops)
 	return oprofile_perf_init(ops);
 }
 
-void __exit oprofile_perf_exit(void)
-{
-	int cpu, id;
-	struct perf_event *event;
-
-	for_each_possible_cpu(cpu) {
-		for (id = 0; id < perf_num_counters(); ++id) {
-			event = perf_events[cpu][id];
-			if (event)
-				perf_event_release_kernel(event);
-		}
-
-		kfree(perf_events[cpu]);
-	}
-
-	kfree(counter_config);
-	exit_driverfs();
-}
-
 void __exit oprofile_arch_exit(void)
 {
 	oprofile_perf_exit();
diff --git a/drivers/oprofile/oprofile_perf.c b/drivers/oprofile/oprofile_perf.c
new file mode 100644
index 0000000..46d7b76
--- /dev/null
+++ b/drivers/oprofile/oprofile_perf.c
@@ -0,0 +1,322 @@
+/*
+ * Copyright 2010 ARM Ltd.
+ *
+ * Perf-events backend for OProfile.
+ */
+#include <linux/perf_event.h>
+#include <linux/oprofile.h>
+#include <linux/slab.h>
+
+/*
+ * 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 oprofile_perf_enabled;
+static DEFINE_MUTEX(oprofile_perf_mutex);
+
+static struct op_counter_config *counter_config;
+static struct perf_event **perf_events[nr_cpumask_bits];
+
+/*
+ * 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 oprofile_perf_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 oprofile_perf_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 oprofile_perf_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 int oprofile_perf_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 oprofile_perf_setup(void)
+{
+	spin_lock(&oprofilefs_lock);
+	op_perf_setup();
+	spin_unlock(&oprofilefs_lock);
+	return 0;
+}
+
+static int oprofile_perf_start(void)
+{
+	int ret = -EBUSY;
+
+	mutex_lock(&oprofile_perf_mutex);
+	if (!oprofile_perf_enabled) {
+		ret = 0;
+		op_perf_start();
+		oprofile_perf_enabled = 1;
+	}
+	mutex_unlock(&oprofile_perf_mutex);
+	return ret;
+}
+
+static void oprofile_perf_stop(void)
+{
+	mutex_lock(&oprofile_perf_mutex);
+	if (oprofile_perf_enabled)
+		op_perf_stop();
+	oprofile_perf_enabled = 0;
+	mutex_unlock(&oprofile_perf_mutex);
+}
+
+#ifdef CONFIG_PM
+static int oprofile_perf_suspend(struct platform_device *dev, pm_message_t state)
+{
+	mutex_lock(&oprofile_perf_mutex);
+	if (oprofile_perf_enabled)
+		op_perf_stop();
+	mutex_unlock(&oprofile_perf_mutex);
+	return 0;
+}
+
+static int oprofile_perf_resume(struct platform_device *dev)
+{
+	mutex_lock(&oprofile_perf_mutex);
+	if (oprofile_perf_enabled && op_perf_start())
+		oprofile_perf_enabled = 0;
+	mutex_unlock(&oprofile_perf_mutex);
+	return 0;
+}
+
+static struct platform_driver oprofile_driver = {
+	.driver		= {
+		.name		= "perf-oprofile",
+	},
+	.resume		= oprofile_perf_resume,
+	.suspend	= oprofile_perf_suspend,
+};
+
+static struct platform_device *oprofile_pdev;
+
+static int __init init_driverfs(void)
+{
+	int ret;
+
+	ret = platform_driver_register(&oprofile_driver);
+	if (ret)
+		goto out;
+
+	oprofile_pdev =	platform_device_register_simple(
+				oprofile_driver.driver.name, 0, NULL, 0);
+	if (IS_ERR(oprofile_pdev)) {
+		ret = PTR_ERR(oprofile_pdev);
+		platform_driver_unregister(&oprofile_driver);
+	}
+
+out:
+	return ret;
+}
+
+static void __exit exit_driverfs(void)
+{
+	platform_device_unregister(oprofile_pdev);
+	platform_driver_unregister(&oprofile_driver);
+}
+#else
+static int __init init_driverfs(void) { return 0; }
+#define exit_driverfs() do { } while (0)
+#endif /* CONFIG_PM */
+
+int __init oprofile_perf_init(struct oprofile_operations *ops)
+{
+	int cpu, ret = 0;
+
+	memset(&perf_events, 0, sizeof(perf_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());
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	ret = init_driverfs();
+	if (ret)
+		goto out;
+
+	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);
+			ret = -ENOMEM;
+			goto out;
+		}
+	}
+
+	if (!ops->create_files)
+		ops->create_files = oprofile_perf_create_files;
+	if (!ops->setup)
+		ops->setup = oprofile_perf_setup;
+	if (!ops->start)
+		ops->start = oprofile_perf_start;
+	if (!ops->stop)
+		ops->stop = oprofile_perf_stop;
+	if (!ops->shutdown)
+		ops->shutdown = oprofile_perf_stop;
+
+	if (!ops->cpu_type)
+		ret = -ENODEV;
+	else
+		pr_info("oprofile: using %s\n", ops->cpu_type);
+
+out:
+	if (ret) {
+		for_each_possible_cpu(cpu)
+			kfree(perf_events[cpu]);
+		kfree(counter_config);
+	}
+
+	return ret;
+}
+
+void __exit oprofile_perf_exit(void)
+{
+	int cpu, id;
+	struct perf_event *event;
+
+	for_each_possible_cpu(cpu) {
+		for (id = 0; id < perf_num_counters(); ++id) {
+			event = perf_events[cpu][id];
+			if (event)
+				perf_event_release_kernel(event);
+		}
+
+		kfree(perf_events[cpu]);
+	}
+
+	kfree(counter_config);
+	exit_driverfs();
+}
diff --git a/include/linux/oprofile.h b/include/linux/oprofile.h
index 5171639..a53bfc0 100644
--- a/include/linux/oprofile.h
+++ b/include/linux/oprofile.h
@@ -15,6 +15,7 @@
 
 #include <linux/types.h>
 #include <linux/spinlock.h>
+#include <linux/init.h>
 #include <asm/atomic.h>
  
 /* Each escaped entry is prefixed by ESCAPE_CODE
@@ -185,4 +186,9 @@ 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);
 
+#ifdef CONFIG_PERF_EVENTS
+int __init oprofile_perf_init(struct oprofile_operations *ops);
+void __exit oprofile_perf_exit(void);
+#endif /* CONFIG_PERF_EVENTS */
+
 #endif /* OPROFILE_H */
-- 
1.7.1

[PATCH 4/6] oprofile: Abstract the perf-events backend

[Robert Richter]

On 13.09.10 02:07:35, Matt Fleming wrote:
> 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       |    2 +-
>  arch/arm/oprofile/common.c       |  315 -------------------------------------
>  drivers/oprofile/oprofile_perf.c |  322 ++++++++++++++++++++++++++++++++++++++
>  include/linux/oprofile.h         |    6 +
>  4 files changed, 329 insertions(+), 316 deletions(-)
>  create mode 100644 drivers/oprofile/oprofile_perf.c

This patch looks good.

-Robert

-- 
Advanced Micro Devices, Inc.
Operating System Research Center

[PATCH 5/6] ARM: Make oprofile depend on CONFIG_HW_PERF_EVENTS

[Matt Fleming]

Currently, oprofile support is only functional if CONFIG_HW_PERF_EVENTS
is set. If this symbol is not set, oprofile initialisation will fail at
runtime. Instead of allowing the oprofile code to build but fail at
runtime it seems less confusing to not allow the code to be built unless
hardware performance counter support is available.

Signed-off-by: Matt Fleming <matt@console-pimps.org>
---
 arch/arm/Kconfig           |    2 +-
 arch/arm/oprofile/common.c |    9 ---------
 2 files changed, 1 insertions(+), 10 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index a7ed21f..d29075c 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -14,7 +14,7 @@ config ARM
 	select RTC_LIB
 	select SYS_SUPPORTS_APM_EMULATION
 	select GENERIC_ATOMIC64 if (!CPU_32v6K)
-	select HAVE_OPROFILE if (HAVE_PERF_EVENTS)
+	select HAVE_OPROFILE if (HW_PERF_EVENTS)
 	select HAVE_ARCH_KGDB
 	select HAVE_KPROBES if (!XIP_KERNEL)
 	select HAVE_KRETPROBES if (HAVE_KPROBES)
diff --git a/arch/arm/oprofile/common.c b/arch/arm/oprofile/common.c
index 9a3ed26..444ea86 100644
--- a/arch/arm/oprofile/common.c
+++ b/arch/arm/oprofile/common.c
@@ -24,7 +24,6 @@
 #include <asm/perf_event.h>
 #include <asm/ptrace.h>
 
-#ifdef CONFIG_HW_PERF_EVENTS
 static char *op_name_from_perf_id(enum arm_perf_pmu_ids id)
 {
 	switch (id) {
@@ -119,11 +118,3 @@ void __exit oprofile_arch_exit(void)
 {
 	oprofile_perf_exit();
 }
-#else
-int __init oprofile_arch_init(struct oprofile_operations *ops)
-{
-	pr_info("oprofile: hardware counters not available\n");
-	return -ENODEV;
-}
-void __exit oprofile_arch_exit(void) {}
-#endif /* CONFIG_HW_PERF_EVENTS */
-- 
1.7.1

[PATCH 5/6] ARM: Make oprofile depend on CONFIG_HW_PERF_EVENTS

[Will Deacon]

Hi Matt,

On Mon, 2010-09-13 at 07:07 +0100, Matt Fleming wrote:
> Currently, oprofile support is only functional if CONFIG_HW_PERF_EVENTS
> is set. If this symbol is not set, oprofile initialisation will fail at
> runtime. Instead of allowing the oprofile code to build but fail at
> runtime it seems less confusing to not allow the code to be built unless
> hardware performance counter support is available.
> 
> Signed-off-by: Matt Fleming <matt@console-pimps.org>
> ---
>  arch/arm/Kconfig           |    2 +-
>  arch/arm/oprofile/common.c |    9 ---------
>  2 files changed, 1 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index a7ed21f..d29075c 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -14,7 +14,7 @@ config ARM
>         select RTC_LIB
>         select SYS_SUPPORTS_APM_EMULATION
>         select GENERIC_ATOMIC64 if (!CPU_32v6K)
> -       select HAVE_OPROFILE if (HAVE_PERF_EVENTS)
> +       select HAVE_OPROFILE if (HW_PERF_EVENTS)
>         select HAVE_ARCH_KGDB
>         select HAVE_KPROBES if (!XIP_KERNEL)
>         select HAVE_KRETPROBES if (HAVE_KPROBES)
> diff --git a/arch/arm/oprofile/common.c b/arch/arm/oprofile/common.c
> index 9a3ed26..444ea86 100644
> --- a/arch/arm/oprofile/common.c
> +++ b/arch/arm/oprofile/common.c
> @@ -24,7 +24,6 @@
>  #include <asm/perf_event.h>
>  #include <asm/ptrace.h>
> 
> -#ifdef CONFIG_HW_PERF_EVENTS
>  static char *op_name_from_perf_id(enum arm_perf_pmu_ids id)
>  {
>         switch (id) {
> @@ -119,11 +118,3 @@ void __exit oprofile_arch_exit(void)
>  {
>         oprofile_perf_exit();
>  }
> -#else
> -int __init oprofile_arch_init(struct oprofile_operations *ops)
> -{
> -       pr_info("oprofile: hardware counters not available\n");
> -       return -ENODEV;
> -}
> -void __exit oprofile_arch_exit(void) {}
> -#endif /* CONFIG_HW_PERF_EVENTS */
> --
> 1.7.1


When oprofile_arch_init fails (-ENODEV), oprofile will fall back to
timer mode. With this patch, how can you make use of a timer fallback?

I'll try and get around to testing the rest of this series today.

Cheers,

Will

[PATCH 5/6] ARM: Make oprofile depend on CONFIG_HW_PERF_EVENTS

[Robert Richter]

On 13.09.10 04:39:52, Will Deacon wrote:
> Hi Matt,
> 
> On Mon, 2010-09-13 at 07:07 +0100, Matt Fleming wrote:
> > Currently, oprofile support is only functional if CONFIG_HW_PERF_EVENTS
> > is set. If this symbol is not set, oprofile initialisation will fail at
> > runtime. Instead of allowing the oprofile code to build but fail at
> > runtime it seems less confusing to not allow the code to be built unless
> > hardware performance counter support is available.
> > 
> > Signed-off-by: Matt Fleming <matt@console-pimps.org>
> > ---
> >  arch/arm/Kconfig           |    2 +-
> >  arch/arm/oprofile/common.c |    9 ---------
> >  2 files changed, 1 insertions(+), 10 deletions(-)

> When oprofile_arch_init fails (-ENODEV), oprofile will fall back to
> timer mode. With this patch, how can you make use of a timer fallback?

True, if you want an error at compile time, maybe we use the #error
pragma instead. But falling back to timer mode is fine to me too.
Will leave that decision to the arm folks.

-Robert

> 
> I'll try and get around to testing the rest of this series today.
> 
> Cheers,
> 
> Will
> 
> 
> 
> 

-- 
Advanced Micro Devices, Inc.
Operating System Research Center

[PATCH 5/6] ARM: Make oprofile depend on CONFIG_HW_PERF_EVENTS

[Sergei Shtylyov]

Hello.

On 13-09-2010 10:07, Matt Fleming wrote:

> Currently, oprofile support is only functional if CONFIG_HW_PERF_EVENTS
> is set. If this symbol is not set, oprofile initialisation will fail at
> runtime. Instead of allowing the oprofile code to build but fail at
> runtime it seems less confusing to not allow the code to be built unless
> hardware performance counter support is available.

> Signed-off-by: Matt Fleming<matt@console-pimps.org>
> ---
>   arch/arm/Kconfig           |    2 +-
>   arch/arm/oprofile/common.c |    9 ---------
>   2 files changed, 1 insertions(+), 10 deletions(-)

> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index a7ed21f..d29075c 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -14,7 +14,7 @@ config ARM
>   	select RTC_LIB
>   	select SYS_SUPPORTS_APM_EMULATION
>   	select GENERIC_ATOMIC64 if (!CPU_32v6K)
> -	select HAVE_OPROFILE if (HAVE_PERF_EVENTS)
> +	select HAVE_OPROFILE if (HW_PERF_EVENTS)

    Could remove useless parens while at it...

WBR, Sergei

[PATCH 6/6] sh: oprofile: Use perf-events oprofile backend

[Matt Fleming]

Now that we've got a generic perf-events based oprofile backend we might
as well make use of it seeing as SH doesn't do anything special with its
oprofile backend.

Signed-off-by: Matt Fleming <matt@console-pimps.org>
---
 arch/sh/Kconfig             |    2 +-
 arch/sh/kernel/perf_event.c |    8 +++
 arch/sh/oprofile/Makefile   |    2 +-
 arch/sh/oprofile/common.c   |  104 ++++--------------------------------------
 arch/sh/oprofile/op_impl.h  |   33 --------------
 5 files changed, 20 insertions(+), 129 deletions(-)
 delete mode 100644 arch/sh/oprofile/op_impl.h

diff --git a/arch/sh/Kconfig b/arch/sh/Kconfig
index 33990fa..dc4a017 100644
--- a/arch/sh/Kconfig
+++ b/arch/sh/Kconfig
@@ -11,7 +11,7 @@ config SUPERH
 	select HAVE_CLK
 	select HAVE_IDE if HAS_IOPORT
 	select HAVE_MEMBLOCK
-	select HAVE_OPROFILE
+	select HAVE_OPROFILE if PERF_EVENTS
 	select HAVE_GENERIC_DMA_COHERENT
 	select HAVE_ARCH_TRACEHOOK
 	select HAVE_DMA_API_DEBUG
diff --git a/arch/sh/kernel/perf_event.c b/arch/sh/kernel/perf_event.c
index 2cb9ad5..3c3fc9a 100644
--- a/arch/sh/kernel/perf_event.c
+++ b/arch/sh/kernel/perf_event.c
@@ -59,6 +59,14 @@ static inline int sh_pmu_initialized(void)
 	return !!sh_pmu;
 }
 
+const char *sh_pmu_name(void)
+{
+	if (!sh_pmu)
+		return NULL;
+
+	return sh_pmu->name;
+}
+
 int perf_num_counters(void)
 {
 	if (!sh_pmu)
diff --git a/arch/sh/oprofile/Makefile b/arch/sh/oprofile/Makefile
index 4886c5c..e1015ae 100644
--- a/arch/sh/oprofile/Makefile
+++ b/arch/sh/oprofile/Makefile
@@ -4,6 +4,6 @@ DRIVER_OBJS = $(addprefix ../../../drivers/oprofile/, \
 		oprof.o cpu_buffer.o buffer_sync.o \
 		event_buffer.o oprofile_files.o \
 		oprofilefs.o oprofile_stats.o \
-		timer_int.o )
+		timer_int.o oprofile_perf.o )
 
 oprofile-y	:= $(DRIVER_OBJS) common.o backtrace.o
diff --git a/arch/sh/oprofile/common.c b/arch/sh/oprofile/common.c
index ac60493..87ce88e 100644
--- a/arch/sh/oprofile/common.c
+++ b/arch/sh/oprofile/common.c
@@ -18,113 +18,29 @@
 #include <linux/errno.h>
 #include <linux/smp.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);
+extern const char *sh_pmu_name(void);
 
-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, "SH7750"))
+		return "sh/sh7750";
+	if (!strcmp(name, "SH-4A"))
+		return "sh/sh4a";
 
-	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
-	 * this.
-	 */
 	ops->backtrace = sh_backtrace;
+	ops->cpu_type  = op_name_from_perf_name(sh_pmu_name());
 
-	/*
-	 * 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_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;
-
-	printk(KERN_INFO "oprofile: using %s performance monitoring.\n",
-	       lmodel->cpu_type);
-
-	return 0;
+	return oprofile_perf_init(ops);
 }
 
 void oprofile_arch_exit(void)
 {
-	if (model && model->exit)
-		model->exit();
+	oprofile_perf_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.1

[PATCH 6/6] sh: oprofile: Use perf-events oprofile backend

[Robert Richter]

On 13.09.10 02:07:37, Matt Fleming wrote:
> Now that we've got a generic perf-events based oprofile backend we might
> as well make use of it seeing as SH doesn't do anything special with its
> oprofile backend.
> 
> Signed-off-by: Matt Fleming <matt@console-pimps.org>
> ---
>  arch/sh/Kconfig             |    2 +-
>  arch/sh/kernel/perf_event.c |    8 +++
>  arch/sh/oprofile/Makefile   |    2 +-
>  arch/sh/oprofile/common.c   |  104 ++++--------------------------------------
>  arch/sh/oprofile/op_impl.h  |   33 --------------
>  5 files changed, 20 insertions(+), 129 deletions(-)
>  delete mode 100644 arch/sh/oprofile/op_impl.h
> 
> diff --git a/arch/sh/Kconfig b/arch/sh/Kconfig
> index 33990fa..dc4a017 100644
> --- a/arch/sh/Kconfig
> +++ b/arch/sh/Kconfig
> @@ -11,7 +11,7 @@ config SUPERH
>  	select HAVE_CLK
>  	select HAVE_IDE if HAS_IOPORT
>  	select HAVE_MEMBLOCK
> -	select HAVE_OPROFILE
> +	select HAVE_OPROFILE if PERF_EVENTS

Same here, we might want to fall back to timer mode.

>  	select HAVE_GENERIC_DMA_COHERENT
>  	select HAVE_ARCH_TRACEHOOK
>  	select HAVE_DMA_API_DEBUG
> diff --git a/arch/sh/kernel/perf_event.c b/arch/sh/kernel/perf_event.c
> index 2cb9ad5..3c3fc9a 100644
> --- a/arch/sh/kernel/perf_event.c
> +++ b/arch/sh/kernel/perf_event.c
> @@ -59,6 +59,14 @@ static inline int sh_pmu_initialized(void)
>  	return !!sh_pmu;
>  }
>  
> +const char *sh_pmu_name(void)
> +{
> +	if (!sh_pmu)
> +		return NULL;
> +
> +	return sh_pmu->name;
> +}

Couldn't we make this a generic function like perf_num_counters()?

[...]

>  void oprofile_arch_exit(void)
>  {
> -	if (model && model->exit)
> -		model->exit();
> +	oprofile_perf_exit();
>  }

This will probalby cause a section missmatch. Use __exit marker for
oprofile_arch_exit().

-Robert

-- 
Advanced Micro Devices, Inc.
Operating System Research Center

[PATCH 6/6] sh: oprofile: Use perf-events oprofile backend

[Matt Fleming]

On Thu, Sep 16, 2010 at 04:32:54PM +0200, Robert Richter wrote:
> > diff --git a/arch/sh/kernel/perf_event.c b/arch/sh/kernel/perf_event.c
> > index 2cb9ad5..3c3fc9a 100644
> > --- a/arch/sh/kernel/perf_event.c
> > +++ b/arch/sh/kernel/perf_event.c
> > @@ -59,6 +59,14 @@ static inline int sh_pmu_initialized(void)
> >  	return !!sh_pmu;
> >  }
> >  
> > +const char *sh_pmu_name(void)
> > +{
> > +	if (!sh_pmu)
> > +		return NULL;
> > +
> > +	return sh_pmu->name;
> > +}
> 
> Couldn't we make this a generic function like perf_num_counters()?

Well, ARM doesn't have names as strings for its pmus currently. What's
more, ARM wouldn't use it; SH would be the only user of this function. I
don't think this one makes sense to be a generic function.

[PATCH 6/6] sh: oprofile: Use perf-events oprofile backend

[Robert Richter]

On 27.09.10 16:01:38, Matt Fleming wrote:
> On Thu, Sep 16, 2010 at 04:32:54PM +0200, Robert Richter wrote:
> > > diff --git a/arch/sh/kernel/perf_event.c b/arch/sh/kernel/perf_event.c
> > > index 2cb9ad5..3c3fc9a 100644
> > > --- a/arch/sh/kernel/perf_event.c
> > > +++ b/arch/sh/kernel/perf_event.c
> > > @@ -59,6 +59,14 @@ static inline int sh_pmu_initialized(void)
> > >  	return !!sh_pmu;
> > >  }
> > >  
> > > +const char *sh_pmu_name(void)
> > > +{
> > > +	if (!sh_pmu)
> > > +		return NULL;
> > > +
> > > +	return sh_pmu->name;
> > > +}
> > 
> > Couldn't we make this a generic function like perf_num_counters()?
> 
> Well, ARM doesn't have names as strings for its pmus currently. What's
> more, ARM wouldn't use it; SH would be the only user of this function. I
> don't think this one makes sense to be a generic function.

I didn't catch this with my first review, the function will need an
EXPORT_SYMBOL_GPL() to allow building modules. This will mean an
interface extension what should be non-arch. So, for architectures we
need the pmu name like SH we just implement the generic function. For
ARM we don't need to provide this function.

Most of the interface is defined in linux/perf_event.h. We shouldn't
move this to asm/perf_event.h, so this is one more argument for the
non-arch implementation.

As the implementation of the function would be optional, why should we
make it architectural?

-Robert

-- 
Advanced Micro Devices, Inc.
Operating System Research Center

[PATCH 6/6] sh: oprofile: Use perf-events oprofile backend

[Matt Fleming]

On Tue, Sep 28, 2010 at 12:07:03AM +0200, Robert Richter wrote:
> On 27.09.10 16:01:38, Matt Fleming wrote:
> > On Thu, Sep 16, 2010 at 04:32:54PM +0200, Robert Richter wrote:
> > > > diff --git a/arch/sh/kernel/perf_event.c b/arch/sh/kernel/perf_event.c
> > > > index 2cb9ad5..3c3fc9a 100644
> > > > --- a/arch/sh/kernel/perf_event.c
> > > > +++ b/arch/sh/kernel/perf_event.c
> > > > @@ -59,6 +59,14 @@ static inline int sh_pmu_initialized(void)
> > > >  	return !!sh_pmu;
> > > >  }
> > > >  
> > > > +const char *sh_pmu_name(void)
> > > > +{
> > > > +	if (!sh_pmu)
> > > > +		return NULL;
> > > > +
> > > > +	return sh_pmu->name;
> > > > +}
> > > 
> > > Couldn't we make this a generic function like perf_num_counters()?
> > 
> > Well, ARM doesn't have names as strings for its pmus currently. What's
> > more, ARM wouldn't use it; SH would be the only user of this function. I
> > don't think this one makes sense to be a generic function.
> 
> I didn't catch this with my first review, the function will need an
> EXPORT_SYMBOL_GPL() to allow building modules. This will mean an
> interface extension what should be non-arch. So, for architectures we
> need the pmu name like SH we just implement the generic function. For
> ARM we don't need to provide this function.

Yeah my bad, I just hit this EXPORT_SYMBOL_GPL issue when compiling
oprofile as a module.

> Most of the interface is defined in linux/perf_event.h. We shouldn't
> move this to asm/perf_event.h, so this is one more argument for the
> non-arch implementation.

I'm not advocating moving this function to asm/perf_event.h.

> As the implementation of the function would be optional, why should we
> make it architectural?

I don't see why we should pollute the perf namespace with a function
that is only being used inside the SH oprofile code? There would be
exactly one use of this function and I doubt the perf guys will want
this function exposed. In it's current state, it really is no use to any
architecture other than SH.

We can always add a generic perf_pmu_name() function later if needed,
but it's definitely not needed at the moment.

[PATCH 6/6] sh: oprofile: Use perf-events oprofile backend

[Robert Richter]

On 27.09.10 18:26:27, Matt Fleming wrote:
> On Tue, Sep 28, 2010 at 12:07:03AM +0200, Robert Richter wrote:
> > On 27.09.10 16:01:38, Matt Fleming wrote:
> > > On Thu, Sep 16, 2010 at 04:32:54PM +0200, Robert Richter wrote:
> > > > > diff --git a/arch/sh/kernel/perf_event.c b/arch/sh/kernel/perf_event.c
> > > > > index 2cb9ad5..3c3fc9a 100644
> > > > > --- a/arch/sh/kernel/perf_event.c
> > > > > +++ b/arch/sh/kernel/perf_event.c
> > > > > @@ -59,6 +59,14 @@ static inline int sh_pmu_initialized(void)
> > > > >  	return !!sh_pmu;
> > > > >  }
> > > > >  
> > > > > +const char *sh_pmu_name(void)
> > > > > +{
> > > > > +	if (!sh_pmu)
> > > > > +		return NULL;
> > > > > +
> > > > > +	return sh_pmu->name;
> > > > > +}
> > > > 
> > > > Couldn't we make this a generic function like perf_num_counters()?
> > > 
> > > Well, ARM doesn't have names as strings for its pmus currently. What's
> > > more, ARM wouldn't use it; SH would be the only user of this function. I
> > > don't think this one makes sense to be a generic function.
> > 
> > I didn't catch this with my first review, the function will need an
> > EXPORT_SYMBOL_GPL() to allow building modules. This will mean an
> > interface extension what should be non-arch. So, for architectures we
> > need the pmu name like SH we just implement the generic function. For
> > ARM we don't need to provide this function.
> 
> Yeah my bad, I just hit this EXPORT_SYMBOL_GPL issue when compiling
> oprofile as a module.
> 
> > Most of the interface is defined in linux/perf_event.h. We shouldn't
> > move this to asm/perf_event.h, so this is one more argument for the
> > non-arch implementation.
> 
> I'm not advocating moving this function to asm/perf_event.h.
> 
> > As the implementation of the function would be optional, why should we
> > make it architectural?
> 
> I don't see why we should pollute the perf namespace with a function
> that is only being used inside the SH oprofile code? There would be
> exactly one use of this function and I doubt the perf guys will want
> this function exposed. In it's current state, it really is no use to any
> architecture other than SH.
> 
> We can always add a generic perf_pmu_name() function later if needed,
> but it's definitely not needed at the moment.

Ok, so then let's implement sh_pmu_name() in
arch/sh/oprofile/common.c, we import asm/perf_event.h to get access to
struct sh_pmu. This could be implemented as a static function then and
we also get rid of this interface thing.

-Robert

-- 
Advanced Micro Devices, Inc.
Operating System Research Center

[PATCH 6/6] sh: oprofile: Use perf-events oprofile backend

[Matt Fleming]

On Tue, Sep 28, 2010 at 12:45:51AM +0200, Robert Richter wrote:
> On 27.09.10 18:26:27, Matt Fleming wrote:
> > 
> > I don't see why we should pollute the perf namespace with a function
> > that is only being used inside the SH oprofile code? There would be
> > exactly one use of this function and I doubt the perf guys will want
> > this function exposed. In it's current state, it really is no use to any
> > architecture other than SH.
> > 
> > We can always add a generic perf_pmu_name() function later if needed,
> > but it's definitely not needed at the moment.
> 
> Ok, so then let's implement sh_pmu_name() in
> arch/sh/oprofile/common.c, we import asm/perf_event.h to get access to
> struct sh_pmu. This could be implemented as a static function then and
> we also get rid of this interface thing.

That seems OK to me s'long as Paul (the author of the perf events code
for SH) doesn't have a problem with us exposing the struct sh_pmu *
outside of arch/sh/kernel/perf_event.c

[PATCH 6/6] sh: oprofile: Use perf-events oprofile backend

[Paul Mundt]

On Mon, Sep 27, 2010 at 11:26:27PM +0100, Matt Fleming wrote:
> On Tue, Sep 28, 2010 at 12:07:03AM +0200, Robert Richter wrote:
> > On 27.09.10 16:01:38, Matt Fleming wrote:
> > > Well, ARM doesn't have names as strings for its pmus currently. What's
> > > more, ARM wouldn't use it; SH would be the only user of this function. I
> > > don't think this one makes sense to be a generic function.
> > 

Er, what? Yes it does. It has this silly id to string mapping thing that
is at present duplicated between the perf and the oprofile code for no
reason. Having a generic (albeit optional) perf_pmu_name() would allow
this to be cleaned up.

> > As the implementation of the function would be optional, why should we
> > make it architectural?
> 
> I don't see why we should pollute the perf namespace with a function
> that is only being used inside the SH oprofile code? There would be
> exactly one use of this function and I doubt the perf guys will want
> this function exposed. In it's current state, it really is no use to any
> architecture other than SH.
> 
This has nothing at all to do with the SH oprofile code and everything to
do with oprofile in general. This is the vary basis for the CPU model
matching in the oprofile userspace tools, it's string based by design.
Given that we're already seeing architetures double up their strings in
both places, this really wants a consensus and to be dealt with before
other architetures start getting the wrong idea.

[PATCH 6/6] sh: oprofile: Use perf-events oprofile backend

[Will Deacon]

Paul,

On Thu, 2010-09-30 at 02:04 +0100, Paul Mundt wrote:
> On Mon, Sep 27, 2010 at 11:26:27PM +0100, Matt Fleming wrote:
> > On Tue, Sep 28, 2010 at 12:07:03AM +0200, Robert Richter wrote:
> > > On 27.09.10 16:01:38, Matt Fleming wrote:
> > > > Well, ARM doesn't have names as strings for its pmus currently. What's
> > > > more, ARM wouldn't use it; SH would be the only user of this function. I
> > > > don't think this one makes sense to be a generic function.
> > >
> 
> Er, what? Yes it does. It has this silly id to string mapping thing that
> is at present duplicated between the perf and the oprofile code for no
> reason. Having a generic (albeit optional) perf_pmu_name() would allow
> this to be cleaned up.
> 

It's not as silly as it appears. As you mention below, OProfile uses
strings to identify the correct CPU model in userspace. With ARM, there
is some horrible legacy here which means that the strings are extremely
misleading (arm/mpcore => arm/11mpcore, arm/armv7 => Cortex A8). Rather
than continue this mess into perf, I decided to separate the two and use
a common numeric scheme for when they have to talk to each other.

> > > As the implementation of the function would be optional, why should we
> > > make it architectural?
> >
> > I don't see why we should pollute the perf namespace with a function
> > that is only being used inside the SH oprofile code? There would be
> > exactly one use of this function and I doubt the perf guys will want
> > this function exposed. In it's current state, it really is no use to any
> > architecture other than SH.
> >
> This has nothing at all to do with the SH oprofile code and everything to
> do with oprofile in general. This is the vary basis for the CPU model
> matching in the oprofile userspace tools, it's string based by design.
> Given that we're already seeing architetures double up their strings in
> both places, this really wants a consensus and to be dealt with before
> other architetures start getting the wrong idea.


I agree that if you have sane strings for OProfile then there's no
reason to define new ones for perf.

Will

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

[Marc Titinger]

Hi, 

I'm just being curious : do these patches change the way those chips should be supported, that do not have a PMU-like IP, but implement PC-sampling thanks to a general purpose timer (not the system timer) ?

Thanks and Regards,
Marc. 


> From: matt at console-pimps.org
> To: robert.richter at amd.com
> Subject: [PATCH V3 0/6] Generalise ARM perf-events backend for oprofile
> Date: Mon, 13 Sep 2010 07:07:31 +0100
> CC: linux-arch at vger.kernel.org; linux at arm.linux.org.uk; linux-sh at vger.kernel.org; peterz at infradead.org; fweisbec at gmail.com; will.deacon at arm.com; acme at redhat.com; lethal at linux-sh.org; mingo at elte.hu; linux-arm-kernel at lists.infradead.org
> 
> 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. Though, I have been able to compile this version of the
> series.
> 
> This patch series is based on,
> 
>     git://git.kernel.org/pub/scm/linux/kernel/git/rric/oprofile.git core
> 
> These patches can also be found at,
> 
>     git://git.kernel.org/pub/scm/linux/kernel/git/mfleming/sh-2.6.git perf-oprofile
> 
> Changes from v2:
>  - Rebased against Robert's oprofile core branch
>  - Moved even more of the ARM code into the generic oprofile code
>  - Broke the patches up into more logical steps
> 
> Changes from v1:
>  - Prefix the new functons with "oprofile_" instead of "op_".
>  - Fix ARM compilation errors
>  - Move all the oprofile-perf logic into oprofile_perf.c
>  - Include cleanup patch from Will
> 
> Matt Fleming (6):
>   perf: Add helper function to return number of counters
>   ARM: oprofile: Rename op_arm to oprofile_perf
>   ARM: oprofile: Move non-ARM code into separate init/exit functions
>   oprofile: Abstract the perf-events backend
>   ARM: Make oprofile depend on CONFIG_HW_PERF_EVENTS
>   sh: oprofile: Use perf-events oprofile backend
> 
>  arch/arm/Kconfig                 |    2 +-
>  arch/arm/kernel/perf_event.c     |    6 +
>  arch/arm/oprofile/Makefile       |    2 +-
>  arch/arm/oprofile/common.c       |  315 +------------------------------------
>  arch/sh/Kconfig                  |    2 +-
>  arch/sh/kernel/perf_event.c      |   17 ++
>  arch/sh/oprofile/Makefile        |    2 +-
>  arch/sh/oprofile/common.c        |  104 ++-----------
>  arch/sh/oprofile/op_impl.h       |   33 ----
>  drivers/oprofile/oprofile_perf.c |  322 ++++++++++++++++++++++++++++++++++++++
>  include/linux/oprofile.h         |    6 +
>  include/linux/perf_event.h       |    1 +
>  12 files changed, 368 insertions(+), 444 deletions(-)
>  delete mode 100644 arch/sh/oprofile/op_impl.h
>  create mode 100644 drivers/oprofile/oprofile_perf.c
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
 		 	   		  
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100913/02dd179a/attachment-0001.html>

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

[Matt Fleming]

On Mon, Sep 13, 2010 at 09:13:32AM +0200, Marc Titinger wrote:
> 
> Hi, 
> 
> I'm just being curious : do these patches change the way those chips
> should be supported, that do not have a PMU-like IP, but implement
> PC-sampling thanks to a general purpose timer (not the system timer)
> ?

CPUs that do not have a PMU are not required to use the perf-events
oprofile backend, it is entirely optional. The pc-sampling timer in
oprofile is not affected by this series.

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

[Ingo Molnar]

* Matt Fleming <matt@console-pimps.org> wrote:

> On Mon, Sep 13, 2010 at 09:13:32AM +0200, Marc Titinger wrote:
> > 
> > Hi, 
> > 
> > I'm just being curious : do these patches change the way those chips 
> > should be supported, that do not have a PMU-like IP, but implement 
> > PC-sampling thanks to a general purpose timer (not the system timer) 
> > ?
> 
> CPUs that do not have a PMU are not required to use the perf-events 
> oprofile backend, it is entirely optional. The pc-sampling timer in 
> oprofile is not affected by this series.

It should still work fine though: a generalized oprofile backend should 
simply use hrtimer based events. That also has a chance to be higher 
quality than the system time fallback, on PMU-less (but high-res-timer 
capable) systems.

Thanks,

	Ingo

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

[Matt Fleming]

On Mon, Sep 13, 2010 at 10:51:37AM +0200, Ingo Molnar wrote:
> 
> * Matt Fleming <matt@console-pimps.org> wrote:
> 
> > On Mon, Sep 13, 2010 at 09:13:32AM +0200, Marc Titinger wrote:
> > > 
> > > Hi, 
> > > 
> > > I'm just being curious : do these patches change the way those chips 
> > > should be supported, that do not have a PMU-like IP, but implement 
> > > PC-sampling thanks to a general purpose timer (not the system timer) 
> > > ?
> > 
> > CPUs that do not have a PMU are not required to use the perf-events 
> > oprofile backend, it is entirely optional. The pc-sampling timer in 
> > oprofile is not affected by this series.
> 
> It should still work fine though: a generalized oprofile backend should 
> simply use hrtimer based events. That also has a chance to be higher 
> quality than the system time fallback, on PMU-less (but high-res-timer 
> capable) systems.

Yeah, that's a good point. It would make a nice addition to this patch
series. I may get chance to take a look at it at some point.

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

[Will Deacon]

Hi Matt,

On Mon, 2010-09-13 at 07:07 +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.
> 
[...]
> 
> 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. Though, I have been able to compile this version of the
> series.
> 
I've tested this patch series on a multicore Cortex-A9 board. If I
revert patch 5/6 (ARM: Make oprofile depend on CONFIG_HW_PERF_EVENTS)
then everything seems to work fine. Otherwise, testing without
HW_PERF_EVENTS doesn't fall back to timer mode.

So, with the exception of the patch above:

Tested-by: Will Deacon <will.deacon@arm.com>

Cheers!

Will

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

[Matt Fleming]

On Mon, Sep 13, 2010 at 11:08:53AM +0100, Will Deacon wrote:
> Hi Matt,
> 
> On Mon, 2010-09-13 at 07:07 +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.
> > 
> [...]
> > 
> > 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. Though, I have been able to compile this version of the
> > series.
> > 
> I've tested this patch series on a multicore Cortex-A9 board. If I
> revert patch 5/6 (ARM: Make oprofile depend on CONFIG_HW_PERF_EVENTS)
> then everything seems to work fine. Otherwise, testing without
> HW_PERF_EVENTS doesn't fall back to timer mode.
> 
> So, with the exception of the patch above:
> 
> Tested-by: Will Deacon <will.deacon@arm.com>

Hi Will, thanks for testing!

Ah yeah, making oprofile rely on CONFIG_HW_PERF_EVENTS is too
strict. I hadn't noticed that armpmu_get_pmu_id() is wrapped in
CONFIG_HW_PERF_EVENTS and had intended op->cpu_type to be NULL and so
we'd fallback to the timer mode.

This patch needs to be dropped entirely (though another patch should
conditionally include oprofile_perf.o based on CONFIG_HW_PERF_EVENT
like I had in my original series).

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

[Robert Richter]

On 13.09.10 07:18:50, Matt Fleming wrote:
> On Mon, Sep 13, 2010 at 11:08:53AM +0100, Will Deacon wrote:

> > I've tested this patch series on a multicore Cortex-A9 board. If I
> > revert patch 5/6 (ARM: Make oprofile depend on CONFIG_HW_PERF_EVENTS)
> > then everything seems to work fine. Otherwise, testing without
> > HW_PERF_EVENTS doesn't fall back to timer mode.
> > 
> > So, with the exception of the patch above:
> > 
> > Tested-by: Will Deacon <will.deacon@arm.com>
> 
> Hi Will, thanks for testing!
> 
> Ah yeah, making oprofile rely on CONFIG_HW_PERF_EVENTS is too
> strict. I hadn't noticed that armpmu_get_pmu_id() is wrapped in
> CONFIG_HW_PERF_EVENTS and had intended op->cpu_type to be NULL and so
> we'd fallback to the timer mode.
> 
> This patch needs to be dropped entirely (though another patch should
> conditionally include oprofile_perf.o based on CONFIG_HW_PERF_EVENT
> like I had in my original series).

Matt,

yes, the patch set looks good so far. With the exception of some minor
comments I made and patch #5 dropped, we should be fine. Please update
the patches.

Thanks,

-Robert

-- 
Advanced Micro Devices, Inc.
Operating System Research Center