[PATCH 0/3] stop OProfile calling arch_exit when arch_init fails

[PATCH 0/3] stop OProfile calling arch_exit when arch_init fails

[Will Deacon]

These patches remove the oprofile_arch_exit call from oprofile_init,
allowing architectures that perform memory allocation in their init
functions to be simplified. This requires some changes to the ARM and
x86 OProfile backends to ensure that their init functions clean up
after themselves if they fail.

This is required for Matt's combined OProfile/Perf driver which will
be shared between all architectures.

Patches taken against tip/master.

Cc: Robert Richter <robert.richter@amd.com>
Cc: Matt Fleming <matt@console-pimps.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@elte.hu>

Will Deacon (3):
  oprofile: don't call arch exit code from init code on failure
  ARM: oprofile: fix and simplify init/exit functions
  x86: oprofile: fix oprofile_arch_init behaviour on failure

 arch/arm/oprofile/common.c |   47 +++++++++++++++++++++++--------------------
 arch/x86/oprofile/init.c   |   26 ++++++++++++++----------
 drivers/oprofile/oprof.c   |   11 +--------
 3 files changed, 42 insertions(+), 42 deletions(-)

[PATCH 1/3] oprofile: don't call arch exit code from init code on failure

[Will Deacon]

oprofile_init calls oprofile_arch_init to initialise the architecture-specific
backend code. If this backend code returns failure, oprofile_arch_exit is
called immediately, making it difficult to allocate and free resources
correctly.

This patch removes the oprofile_arch_exit call from oprofile_init,
meaning that all architectures must ensure that oprofile_arch_init
cleans up any mess it's made before returning an error. As far as
I can tell, this only affects the code for ARM and x86.

Cc: Robert Richter <robert.richter@amd.com>
Cc: Matt Fleming <matt@console-pimps.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@elte.hu>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 drivers/oprofile/oprof.c |   11 ++---------
 1 files changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/oprofile/oprof.c b/drivers/oprofile/oprof.c
index b336cd9..b4a6857 100644
--- a/drivers/oprofile/oprof.c
+++ b/drivers/oprofile/oprof.c
@@ -257,16 +257,9 @@ static int __init oprofile_init(void)
 		printk(KERN_INFO "oprofile: using timer interrupt.\n");
 		err = oprofile_timer_init(&oprofile_ops);
 		if (err)
-			goto out_arch;
+			return err;
 	}
-	err = oprofilefs_register();
-	if (err)
-		goto out_arch;
-	return 0;
-
-out_arch:
-	oprofile_arch_exit();
-	return err;
+	return oprofilefs_register();
 }
 
 
-- 
1.6.3.3

[PATCH 2/3] ARM: oprofile: fix and simplify init/exit functions

[Will Deacon]

Now that oprofile_arch_exit is only called when the OProfile module
is unloaded, it can assume that init completed successfully and not
have to worry about double frees or releasing NULL perf events.

This patch ensures that oprofile_arch_init fails gracefully on ARM
and simplifies the exit code based on the above.

Cc: Robert Richter <robert.richter@amd.com>
Cc: Matt Fleming <matt@console-pimps.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@elte.hu>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/oprofile/common.c |   47 +++++++++++++++++++++++--------------------
 1 files changed, 25 insertions(+), 22 deletions(-)

diff --git a/arch/arm/oprofile/common.c b/arch/arm/oprofile/common.c
index 0691176..c2c4a2e 100644
--- a/arch/arm/oprofile/common.c
+++ b/arch/arm/oprofile/common.c
@@ -275,7 +275,7 @@ out:
 	return ret;
 }
 
-static void  exit_driverfs(void)
+static void __exit exit_driverfs(void)
 {
 	platform_device_unregister(oprofile_pdev);
 	platform_driver_unregister(&oprofile_driver);
@@ -359,14 +359,13 @@ int __init oprofile_arch_init(struct oprofile_operations *ops)
 	if (!counter_config) {
 		pr_info("oprofile: failed to allocate %d "
 				"counters\n", perf_num_counters);
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto out;
 	}
 
 	ret = init_driverfs();
-	if (ret) {
-		kfree(counter_config);
-		return ret;
-	}
+	if (ret)
+		goto out;
 
 	for_each_possible_cpu(cpu) {
 		perf_events[cpu] = kcalloc(perf_num_counters,
@@ -374,9 +373,8 @@ int __init oprofile_arch_init(struct oprofile_operations *ops)
 		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;
+			ret = -ENOMEM;
+			goto out;
 		}
 	}
 
@@ -393,28 +391,33 @@ int __init oprofile_arch_init(struct oprofile_operations *ops)
 	else
 		pr_info("oprofile: using %s\n", ops->cpu_type);
 
+out:
+	if (ret) {
+		kfree(counter_config);
+		for_each_possible_cpu(cpu)
+			kfree(perf_events[cpu]);
+	}
+
 	return ret;
 }
 
-void oprofile_arch_exit(void)
+void __exit oprofile_arch_exit(void)
 {
 	int cpu, id;
 	struct perf_event *event;
 
-	if (*perf_events) {
-		exit_driverfs();
-		for_each_possible_cpu(cpu) {
-			for (id = 0; id < perf_num_counters; ++id) {
-				event = perf_events[cpu][id];
-				if (event != NULL)
-					perf_event_release_kernel(event);
-			}
-			kfree(perf_events[cpu]);
+	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]);
 	}
 
-	if (counter_config)
-		kfree(counter_config);
+	kfree(counter_config);
+	exit_driverfs();
 }
 #else
 int __init oprofile_arch_init(struct oprofile_operations *ops)
@@ -422,5 +425,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 */
-- 
1.6.3.3

[PATCH 3/3] x86: oprofile: fix oprofile_arch_init behaviour on failure

[Will Deacon]

The OProfile driver no longer calls oprofile_arch_exit when
oprofile_arch_init return failure.

This patch fixes the x86 implementation of oprofile_arch_init
to ensure that op_nmi_exit is called if necessary.

Cc: Robert Richter <robert.richter@amd.com>
Cc: Matt Fleming <matt@console-pimps.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@elte.hu>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/x86/oprofile/init.c |   26 +++++++++++++++-----------
 1 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/arch/x86/oprofile/init.c b/arch/x86/oprofile/init.c
index cdfe4c5..886b2c4 100644
--- a/arch/x86/oprofile/init.c
+++ b/arch/x86/oprofile/init.c
@@ -16,34 +16,38 @@
  * with the NMI mode driver.
  */
 
+#ifdef CONFIG_X86_LOCAL_APIC
 extern int op_nmi_init(struct oprofile_operations *ops);
-extern int op_nmi_timer_init(struct oprofile_operations *ops);
 extern void op_nmi_exit(void);
-extern void x86_backtrace(struct pt_regs * const regs, unsigned int depth);
+#else
+static int op_nmi_init(struct oprofile_operations *ops) { return -ENODEV; }
+static void op_nmi_exit(void) {}
+#endif
 
+extern int op_nmi_timer_init(struct oprofile_operations *ops);
+extern void x86_backtrace(struct pt_regs * const regs, unsigned int depth);
 
 int __init oprofile_arch_init(struct oprofile_operations *ops)
 {
 	int ret;
 
-	ret = -ENODEV;
-
-#ifdef CONFIG_X86_LOCAL_APIC
 	ret = op_nmi_init(ops);
-#endif
-#ifdef CONFIG_X86_IO_APIC
 	if (ret < 0)
+#ifdef CONFIG_X86_IO_APIC
 		ret = op_nmi_timer_init(ops);
+#else
+		return ret;
 #endif
+
 	ops->backtrace = x86_backtrace;
 
+	if (ret < 0)
+		op_nmi_exit();
+
 	return ret;
 }
 
-
-void oprofile_arch_exit(void)
+void __exit oprofile_arch_exit(void)
 {
-#ifdef CONFIG_X86_LOCAL_APIC
 	op_nmi_exit();
-#endif
 }
-- 
1.6.3.3

[PATCH 3/3] x86: oprofile: fix oprofile_arch_init behaviour on failure

[Robert Richter]

On 29.08.10 14:52:01, Will Deacon wrote:
> The OProfile driver no longer calls oprofile_arch_exit when
> oprofile_arch_init return failure.
> 
> This patch fixes the x86 implementation of oprofile_arch_init
> to ensure that op_nmi_exit is called if necessary.
> 
> Cc: Robert Richter <robert.richter@amd.com>
> Cc: Matt Fleming <matt@console-pimps.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Ingo Molnar <mingo@elte.hu>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
>  arch/x86/oprofile/init.c |   26 +++++++++++++++-----------
>  1 files changed, 15 insertions(+), 11 deletions(-)
> 

>  int __init oprofile_arch_init(struct oprofile_operations *ops)
>  {
>  	int ret;
>  
> -	ret = -ENODEV;
> -
> -#ifdef CONFIG_X86_LOCAL_APIC
>  	ret = op_nmi_init(ops);
> -#endif
> -#ifdef CONFIG_X86_IO_APIC
>  	if (ret < 0)
> +#ifdef CONFIG_X86_IO_APIC
>  		ret = op_nmi_timer_init(ops);
> +#else
> +		return ret;
>  #endif
> +
>  	ops->backtrace = x86_backtrace;
>  
> +	if (ret < 0)
> +		op_nmi_exit();
> +

I don't see why we have to do this. All init functions above clean up
properly on failure. If op_nmi_init() succeeds we don't call
op_nmi_timer_init(), so we don't need to free it it either.

-Robert

>  	return ret;
>  }

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

[PATCH 3/3] x86: oprofile: fix oprofile_arch_init behaviour on failure

[Will Deacon]

Robert,

On Mon, 2010-08-30 at 10:09 +0100, Robert Richter wrote:
> On 29.08.10 14:52:01, Will Deacon wrote:
> > The OProfile driver no longer calls oprofile_arch_exit when
> > oprofile_arch_init return failure.
> >
> > This patch fixes the x86 implementation of oprofile_arch_init
> > to ensure that op_nmi_exit is called if necessary.
> >
> > Cc: Robert Richter <robert.richter@amd.com>
> > Cc: Matt Fleming <matt@console-pimps.org>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Ingo Molnar <mingo@elte.hu>
> > Signed-off-by: Will Deacon <will.deacon@arm.com>
> > ---
> >  arch/x86/oprofile/init.c |   26 +++++++++++++++-----------
> >  1 files changed, 15 insertions(+), 11 deletions(-)
> >
> 
> >  int __init oprofile_arch_init(struct oprofile_operations *ops)
> >  {
> >       int ret;
> > 
> > -     ret = -ENODEV;
> > -
> > -#ifdef CONFIG_X86_LOCAL_APIC
> >       ret = op_nmi_init(ops);
> > -#endif
> > -#ifdef CONFIG_X86_IO_APIC
> >       if (ret < 0)
> > +#ifdef CONFIG_X86_IO_APIC
> >               ret = op_nmi_timer_init(ops);
> > +#else
> > +             return ret;
> >  #endif
> > +
> >       ops->backtrace = x86_backtrace;
> > 
> > +     if (ret < 0)
> > +             op_nmi_exit();
> > +
> 
> I don't see why we have to do this. All init functions above clean up
> properly on failure. If op_nmi_init() succeeds we don't call
> op_nmi_timer_init(), so we don't need to free it it either.
> 

The original code called op_nmi_exit() from oprofile_arch_exit()
regardless of whether or not op_nmi_init() had succeeded. Actually, it
turns out that this is ok because of this guy:

/* in order to get sysfs right */
static int using_nmi;

which is set by the nmi_init function and checked by nmi_exit.

Do you think it would be better to rework this patch so that the static
using_nmi variable is set explicitly by init.c, or shall I just drop
this patch altogether (and resubmit the first two)?

Thanks,

Will

[PATCH 3/3] x86: oprofile: fix oprofile_arch_init behaviour on failure

[Robert Richter]

On 31.08.10 04:54:40, Will Deacon wrote:
> Do you think it would be better to rework this patch so that the static
> using_nmi variable is set explicitly by init.c, or shall I just drop
> this patch altogether (and resubmit the first two)?

For x86 wont change the code (actually I found a bug in the init_sysfs
error handler for which I will send a fix). Just wanted to get your
confirmation in case I was missing something that x86 is not affected.
I will apply the first 2 patches, no need to resubmit.

Thanks Will.

-Robert

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

[PATCH 3/3] x86: oprofile: fix oprofile_arch_init behaviour on failure

[Will Deacon]

On Tue, 2010-08-31 at 10:05 +0100, Robert Richter wrote:
> On 31.08.10 04:54:40, Will Deacon wrote:
> > Do you think it would be better to rework this patch so that the static
> > using_nmi variable is set explicitly by init.c, or shall I just drop
> > this patch altogether (and resubmit the first two)?
> 
> For x86 wont change the code (actually I found a bug in the init_sysfs
> error handler for which I will send a fix). Just wanted to get your
> confirmation in case I was missing something that x86 is not affected.
> I will apply the first 2 patches, no need to resubmit.
> 
Ok, great. The commit comment might need tweaking for the first
patch now that the x86 code remains unchanged.

Cheers,

Will

[PATCH 3/3] x86: oprofile: fix oprofile_arch_init behaviour on failure

[Robert Richter]

On 31.08.10 05:31:29, Will Deacon wrote:
> Ok, great. The commit comment might need tweaking for the first
> patch now that the x86 code remains unchanged.

Ok, will change this.

-Robert

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

[PATCH] oprofile, x86: fix init_sysfs error handling

[Robert Richter]

On 31.08.10 05:31:29, Will Deacon wrote:
> > For x86 wont change the code (actually I found a bug in the init_sysfs
> > error handler for which I will send a fix). Just wanted to get your
> > confirmation in case I was missing something that x86 is not affected.
> > I will apply the first 2 patches, no need to resubmit.

This is the patch, applied to:

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

-Robert

--

[PATCH] oprofile, x86: fix init_sysfs error handling

[Ingo Molnar]

* Robert Richter <robert.richter@amd.com> wrote:

> On 31.08.10 05:31:29, Will Deacon wrote:
> > > For x86 wont change the code (actually I found a bug in the init_sysfs
> > > error handler for which I will send a fix). Just wanted to get your
> > > confirmation in case I was missing something that x86 is not affected.
> > > I will apply the first 2 patches, no need to resubmit.
> 
> This is the patch, applied to:
> 
>  git://git.kernel.org/pub/scm/linux/kernel/git/rric/oprofile.git urgent
> 
> -Robert
> 
> --
> 
> >From 10f0412f57f2a76a90eff4376f59cbb0a39e4e18 Mon Sep 17 00:00:00 2001
> From: Robert Richter <robert.richter@amd.com>
> Date: Mon, 30 Aug 2010 10:56:18 +0200
> Subject: [PATCH] oprofile, x86: fix init_sysfs error handling
> 
> On failure init_sysfs() might not properly free resources. The error
> code of the function is not checked. And, when reinitializing the exit
> function might be called twice. This patch fixes all this.
> 
> Cc: stable at kernel.org
> Signed-off-by: Robert Richter <robert.richter@amd.com>
> ---
>  arch/x86/oprofile/nmi_int.c |   16 +++++++++++++---
>  1 files changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/oprofile/nmi_int.c b/arch/x86/oprofile/nmi_int.c
> index f6b48f6..73a41d3 100644
> --- a/arch/x86/oprofile/nmi_int.c
> +++ b/arch/x86/oprofile/nmi_int.c
> @@ -568,8 +568,13 @@ static int __init init_sysfs(void)
>  	int error;
>  
>  	error = sysdev_class_register(&oprofile_sysclass);
> -	if (!error)
> -		error = sysdev_register(&device_oprofile);
> +	if (error)
> +		return error;
> +
> +	error = sysdev_register(&device_oprofile);
> +	if (error)
> +		sysdev_class_unregister(&oprofile_sysclass);
> +
>  	return error;
>  }
>  
> @@ -695,6 +700,8 @@ 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;
>  
> @@ -774,7 +781,10 @@ int __init op_nmi_init(struct oprofile_operations *ops)
>  
>  	mux_init(ops);
>  
> -	init_sysfs();
> +	ret = init_sysfs();

FYI, this causes a build error if CONFIG_PM is off:

   arch/x86/oprofile/nmi_int.c:784: error: expected expression before ?do?

Due to this assymetric form of the wrapper:

  #define init_sysfs() do { } while (0)

The wrapper should be changed to return 0 i suspect, via something like 
this:

  static inline int init_sysfs(void) { return 0; }

(untested)

Thanks,

	Ingo

[PATCH] oprofile, x86: fix init_sysfs error handling

[Robert Richter]

On 01.09.10 05:51:17, Ingo Molnar wrote:
> > @@ -774,7 +781,10 @@ int __init op_nmi_init(struct oprofile_operations *ops)
> >  
> >  	mux_init(ops);
> >  
> > -	init_sysfs();
> > +	ret = init_sysfs();
> 
> FYI, this causes a build error if CONFIG_PM is off:
> 
>    arch/x86/oprofile/nmi_int.c:784: error: expected expression before ?do?
> 
> Due to this assymetric form of the wrapper:
> 
>   #define init_sysfs() do { } while (0)
> 
> The wrapper should be changed to return 0 i suspect, via something like 
> this:
> 
>   static inline int init_sysfs(void) { return 0; }
> 
> (untested)

Ingo, thanks for catching this, fix below.

-Robert

--

[PATCH 0/3] stop OProfile calling arch_exit when arch_init fails

[Robert Richter]

On 29.08.10 14:51:58, Will Deacon wrote:
> These patches remove the oprofile_arch_exit call from oprofile_init,
> allowing architectures that perform memory allocation in their init
> functions to be simplified. This requires some changes to the ARM and
> x86 OProfile backends to ensure that their init functions clean up
> after themselves if they fail.
> 
> This is required for Matt's combined OProfile/Perf driver which will
> be shared between all architectures.
> 
> Patches taken against tip/master.
> 
> Cc: Robert Richter <robert.richter@amd.com>
> Cc: Matt Fleming <matt@console-pimps.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Ingo Molnar <mingo@elte.hu>
> 
> Will Deacon (3):
>   oprofile: don't call arch exit code from init code on failure
>   ARM: oprofile: fix and simplify init/exit functions
>   x86: oprofile: fix oprofile_arch_init behaviour on failure
> 
>  arch/arm/oprofile/common.c |   47 +++++++++++++++++++++++--------------------
>  arch/x86/oprofile/init.c   |   26 ++++++++++++++----------
>  drivers/oprofile/oprof.c   |   11 +--------
>  3 files changed, 42 insertions(+), 42 deletions(-)

I have applied patch #1 and #2 to

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

with some small modifications:

Patch #1: Commit message, x86 removed.
Patch #2: Order of freeing memory:

+out:
+       if (ret) {
+               for_each_possible_cpu(cpu)
+                       kfree(perf_events[cpu]);
+               kfree(counter_config);
+       }
+

There is also the patch below on top of it.

Thanks Will,

-Robert

--

[PATCH] oprofile, x86: fix init_sysfs error handling

[Robert Richter]

On failure init_sysfs() might not properly free resources. The error
code of the function is not checked. And, when reinitializing the exit
function might be called twice. This patch fixes all this.

Cc: stable at kernel.org
Signed-off-by: Robert Richter <robert.richter@amd.com>
---
 arch/x86/oprofile/nmi_int.c |   16 +++++++++++++---
 1 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/arch/x86/oprofile/nmi_int.c b/arch/x86/oprofile/nmi_int.c
index f6b48f6..73a41d3 100644
--- a/arch/x86/oprofile/nmi_int.c
+++ b/arch/x86/oprofile/nmi_int.c
@@ -568,8 +568,13 @@ static int __init init_sysfs(void)
 	int error;
 
 	error = sysdev_class_register(&oprofile_sysclass);
-	if (!error)
-		error = sysdev_register(&device_oprofile);
+	if (error)
+		return error;
+
+	error = sysdev_register(&device_oprofile);
+	if (error)
+		sysdev_class_unregister(&oprofile_sysclass);
+
 	return error;
 }
 
@@ -695,6 +700,8 @@ 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;
 
@@ -774,7 +781,10 @@ int __init op_nmi_init(struct oprofile_operations *ops)
 
 	mux_init(ops);
 
-	init_sysfs();
+	ret = init_sysfs();
+	if (ret)
+		return ret;
+
 	using_nmi = 1;
 	printk(KERN_INFO "oprofile: using NMI interrupt.\n");
 	return 0;
-- 
1.7.1.1



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