[PATCH 0/5] oprofile updates for v2.6.37

[PATCH 0/5] oprofile updates for v2.6.37

[Robert Richter]

This patch set contains some rework and cleanups of the oprofile code.
It also contains changes of the oprofile perf backend that is
currently implemented for the ARM architecture, but will be ported to
oprofile generic code.

Will, please ack the ARM changes if you are fine with it.

Matt, these patches may cause conflicts with your next patch set, so I
am fine with it if you are not rebasing your code. I will apply them
then to a different branch and solve the conflicts myself.

Ingo, this patch set is for review only. If there are no objections
with it I am going to apply it to my oprofile tree.

Thanks for review,

-Robert

[PATCH 1/5] oprofile, x86: Simplify init/exit functions

[Robert Richter]

Now, that we only call the exit function if init succeeds with commit:

 979048e oprofile: don't call arch exit code from init code on failure

we can simplify the x86 init/exit functions too. Variable using_nmi
becomes obsolete.

Signed-off-by: Robert Richter <robert.richter@amd.com>
---
 arch/x86/oprofile/nmi_int.c |    9 +--------
 1 files changed, 1 insertions(+), 8 deletions(-)

diff --git a/arch/x86/oprofile/nmi_int.c b/arch/x86/oprofile/nmi_int.c
index f1575c9..bd1489c 100644
--- a/arch/x86/oprofile/nmi_int.c
+++ b/arch/x86/oprofile/nmi_int.c
@@ -695,9 +695,6 @@ static int __init ppro_init(char **cpu_type)
 	return 1;
 }
 
-/* in order to get sysfs right */
-static int using_nmi;
-
 int __init op_nmi_init(struct oprofile_operations *ops)
 {
 	__u8 vendor = boot_cpu_data.x86_vendor;
@@ -705,8 +702,6 @@ int __init op_nmi_init(struct oprofile_operations *ops)
 	char *cpu_type = NULL;
 	int ret = 0;
 
-	using_nmi = 0;
-
 	if (!cpu_has_apic)
 		return -ENODEV;
 
@@ -790,13 +785,11 @@ int __init op_nmi_init(struct oprofile_operations *ops)
 	if (ret)
 		return ret;
 
-	using_nmi = 1;
 	printk(KERN_INFO "oprofile: using NMI interrupt.\n");
 	return 0;
 }
 
 void op_nmi_exit(void)
 {
-	if (using_nmi)
-		exit_sysfs();
+	exit_sysfs();
 }
-- 
1.7.2.2

[PATCH 2/5] oprofile, ARM: Use oprofile_arch_exit() to cleanup on failure

[Robert Richter]

There is duplicate cleanup code in the init and exit functions. Now,
oprofile_arch_exit() is also used if oprofile_arch_init() fails.

Cc: Will Deacon <will.deacon@arm.com>
Signed-off-by: Robert Richter <robert.richter@amd.com>
---
 arch/arm/oprofile/common.c |   53 +++++++++++++++++++++----------------------
 1 files changed, 26 insertions(+), 27 deletions(-)

diff --git a/arch/arm/oprofile/common.c b/arch/arm/oprofile/common.c
index bd7426f..7ae9eeb 100644
--- a/arch/arm/oprofile/common.c
+++ b/arch/arm/oprofile/common.c
@@ -348,10 +348,33 @@ static void arm_backtrace(struct pt_regs * const regs, unsigned int depth)
 		tail = user_backtrace(tail);
 }
 
+void oprofile_arch_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();
+}
+
 int __init oprofile_arch_init(struct oprofile_operations *ops)
 {
 	int cpu, ret = 0;
 
+	ret = init_driverfs();
+	if (ret)
+		return ret;
+
 	memset(&perf_events, 0, sizeof(perf_events));
 
 	perf_num_counters = armpmu_get_max_events();
@@ -363,13 +386,10 @@ int __init oprofile_arch_init(struct oprofile_operations *ops)
 		pr_info("oprofile: failed to allocate %d "
 				"counters\n", perf_num_counters);
 		ret = -ENOMEM;
+		perf_num_counters = 0;
 		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);
@@ -395,33 +415,12 @@ int __init oprofile_arch_init(struct oprofile_operations *ops)
 		pr_info("oprofile: using %s\n", ops->cpu_type);
 
 out:
-	if (ret) {
-		for_each_possible_cpu(cpu)
-			kfree(perf_events[cpu]);
-		kfree(counter_config);
-	}
+	if (ret)
+		oprofile_arch_exit();
 
 	return ret;
 }
 
-void __exit oprofile_arch_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();
-}
 #else
 int __init oprofile_arch_init(struct oprofile_operations *ops)
 {
-- 
1.7.2.2

[PATCH 3/5] oprofile, ARM: Rework op_create_counter()

[Robert Richter]

This patch simplifies op_create_counter(). Removing if/else if paths
and return code variable by direct returning from function.

Cc: Will Deacon <will.deacon@arm.com>
Signed-off-by: Robert Richter <robert.richter@amd.com>
---
 arch/arm/oprofile/common.c |   20 ++++++++++----------
 1 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/arch/arm/oprofile/common.c b/arch/arm/oprofile/common.c
index 7ae9eeb..cec9305 100644
--- a/arch/arm/oprofile/common.c
+++ b/arch/arm/oprofile/common.c
@@ -89,28 +89,28 @@ static void op_perf_setup(void)
 
 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;
+	if (!counter_config[event].enabled || perf_events[cpu][event])
+		return 0;
 
 	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) {
+	if (IS_ERR(pevent))
+		return PTR_ERR(pevent);
+
+	if (pevent->state != PERF_EVENT_STATE_ACTIVE) {
 		perf_event_release_kernel(pevent);
 		pr_warning("oprofile: failed to enable event %d "
 				"on CPU %d\n", event, cpu);
-		ret = -EBUSY;
-	} else {
-		perf_events[cpu][event] = pevent;
+		return -EBUSY;
 	}
 
-	return ret;
+	perf_events[cpu][event] = pevent;
+
+	return 0;
 }
 
 static void op_destroy_counter(int cpu, int event)
-- 
1.7.2.2

[PATCH 4/5] oprofile, ARM: Remove some goto statements

[Robert Richter]

This patch removes some unnecessary goto statements.

Cc: Will Deacon <will.deacon@arm.com>
Signed-off-by: Robert Richter <robert.richter@amd.com>
---
 arch/arm/oprofile/common.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/arch/arm/oprofile/common.c b/arch/arm/oprofile/common.c
index cec9305..ab875f3 100644
--- a/arch/arm/oprofile/common.c
+++ b/arch/arm/oprofile/common.c
@@ -135,11 +135,10 @@ static int op_perf_start(void)
 		for (event = 0; event < perf_num_counters; ++event) {
 			ret = op_create_counter(cpu, event);
 			if (ret)
-				goto out;
+				return ret;
 		}
 	}
 
-out:
 	return ret;
 }
 
@@ -263,7 +262,7 @@ static int __init init_driverfs(void)
 
 	ret = platform_driver_register(&oprofile_driver);
 	if (ret)
-		goto out;
+		return ret;
 
 	oprofile_pdev =	platform_device_register_simple(
 				oprofile_driver.driver.name, 0, NULL, 0);
@@ -272,7 +271,6 @@ static int __init init_driverfs(void)
 		platform_driver_unregister(&oprofile_driver);
 	}
 
-out:
 	return ret;
 }
 
-- 
1.7.2.2

[PATCH 5/5] oprofile: Remove duplicate code around __oprofilefs_create_file()

[Robert Richter]

Removing duplicate code by assigning the inodes private data pointer
in __oprofilefs_create_file(). Extending the function interface to
pass the pointer.

Signed-off-by: Robert Richter <robert.richter@amd.com>
---
 drivers/oprofile/oprofilefs.c |   46 ++++++++++++----------------------------
 1 files changed, 14 insertions(+), 32 deletions(-)

diff --git a/drivers/oprofile/oprofilefs.c b/drivers/oprofile/oprofilefs.c
index 2766a6d..789a1a8 100644
--- a/drivers/oprofile/oprofilefs.c
+++ b/drivers/oprofile/oprofilefs.c
@@ -126,50 +126,41 @@ static const struct file_operations ulong_ro_fops = {
 };
 
 
-static struct dentry *__oprofilefs_create_file(struct super_block *sb,
+static int __oprofilefs_create_file(struct super_block *sb,
 	struct dentry *root, char const *name, const struct file_operations *fops,
-	int perm)
+	int perm, void *priv)
 {
 	struct dentry *dentry;
 	struct inode *inode;
 
 	dentry = d_alloc_name(root, name);
 	if (!dentry)
-		return NULL;
+		return -ENOMEM;
 	inode = oprofilefs_get_inode(sb, S_IFREG | perm);
 	if (!inode) {
 		dput(dentry);
-		return NULL;
+		return -ENOMEM;
 	}
 	inode->i_fop = fops;
 	d_add(dentry, inode);
-	return dentry;
+	dentry->d_inode->i_private = priv;
+	return 0;
 }
 
 
 int oprofilefs_create_ulong(struct super_block *sb, struct dentry *root,
 	char const *name, unsigned long *val)
 {
-	struct dentry *d = __oprofilefs_create_file(sb, root, name,
-						     &ulong_fops, 0644);
-	if (!d)
-		return -EFAULT;
-
-	d->d_inode->i_private = val;
-	return 0;
+	return __oprofilefs_create_file(sb, root, name,
+					&ulong_fops, 0644, val);
 }
 
 
 int oprofilefs_create_ro_ulong(struct super_block *sb, struct dentry *root,
 	char const *name, unsigned long *val)
 {
-	struct dentry *d = __oprofilefs_create_file(sb, root, name,
-						     &ulong_ro_fops, 0444);
-	if (!d)
-		return -EFAULT;
-
-	d->d_inode->i_private = val;
-	return 0;
+	return __oprofilefs_create_file(sb, root, name,
+					&ulong_ro_fops, 0444, val);
 }
 
 
@@ -189,31 +180,22 @@ static const struct file_operations atomic_ro_fops = {
 int oprofilefs_create_ro_atomic(struct super_block *sb, struct dentry *root,
 	char const *name, atomic_t *val)
 {
-	struct dentry *d = __oprofilefs_create_file(sb, root, name,
-						     &atomic_ro_fops, 0444);
-	if (!d)
-		return -EFAULT;
-
-	d->d_inode->i_private = val;
-	return 0;
+	return __oprofilefs_create_file(sb, root, name,
+					&atomic_ro_fops, 0444, val);
 }
 
 
 int oprofilefs_create_file(struct super_block *sb, struct dentry *root,
 	char const *name, const struct file_operations *fops)
 {
-	if (!__oprofilefs_create_file(sb, root, name, fops, 0644))
-		return -EFAULT;
-	return 0;
+	return __oprofilefs_create_file(sb, root, name, fops, 0644, NULL);
 }
 
 
 int oprofilefs_create_file_perm(struct super_block *sb, struct dentry *root,
 	char const *name, const struct file_operations *fops, int perm)
 {
-	if (!__oprofilefs_create_file(sb, root, name, fops, perm))
-		return -EFAULT;
-	return 0;
+	return __oprofilefs_create_file(sb, root, name, fops, perm, NULL);
 }
 
 
-- 
1.7.2.2

Re: [PATCH 0/5] oprofile updates for v2.6.37

[Will Deacon]

Hello Robert,

On Fri, 2010-10-01 at 16:56 +0100, Robert Richter wrote:
> This patch set contains some rework and cleanups of the oprofile code.
> It also contains changes of the oprofile perf backend that is
> currently implemented for the ARM architecture, but will be ported to
> oprofile generic code.
> 
> Will, please ack the ARM changes if you are fine with it.
> 

I've been through patches 2, 3 and 4 and they look fine. I've also
tested them and can't see any regressions. Thanks for sticking with
this!

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

Have a good weekend,

Will

Re: [PATCH 0/5] oprofile updates for v2.6.37

[Robert Richter]

On 01.10.10 17:56:22, Robert Richter wrote:
> This patch set contains some rework and cleanups of the oprofile code.
> It also contains changes of the oprofile perf backend that is
> currently implemented for the ARM architecture, but will be ported to
> oprofile generic code.
> 
> Will, please ack the ARM changes if you are fine with it.

Will, thanks for your review.

I have applied the patches to oprofile/core.

-Robert

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