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 <[email protected]>
Cc: Matt Fleming <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Ingo Molnar <[email protected]>
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(-)
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 <[email protected]>
Cc: Matt Fleming <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Ingo Molnar <[email protected]>
Signed-off-by: Will Deacon <[email protected]>
---
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
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 <[email protected]>
Cc: Matt Fleming <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Ingo Molnar <[email protected]>
Signed-off-by: Will Deacon <[email protected]>
---
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
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 <[email protected]>
Cc: Matt Fleming <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Ingo Molnar <[email protected]>
Signed-off-by: Will Deacon <[email protected]>
---
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
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 <[email protected]>
> Cc: Matt Fleming <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Signed-off-by: Will Deacon <[email protected]>
> ---
> 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
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 <[email protected]>
> > Cc: Matt Fleming <[email protected]>
> > Cc: Peter Zijlstra <[email protected]>
> > Cc: Ingo Molnar <[email protected]>
> > Signed-off-by: Will Deacon <[email protected]>
> > ---
> > 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
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
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
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
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 <[email protected]>
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: [email protected]
Signed-off-by: Robert Richter <[email protected]>
---
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
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 <[email protected]>
> Cc: Matt Fleming <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Ingo Molnar <[email protected]>
>
> 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
--
>From 4cbe75be5c6ae86bdc7daec864eeb2dfd66f48bb Mon Sep 17 00:00:00 2001
From: Robert Richter <[email protected]>
Date: Mon, 30 Aug 2010 18:21:55 +0200
Subject: [PATCH] oprofile, arm: initialize perf_event pointers with NULL
The pointers must be NULL'ed to avoid double-freeing the pointers in
rare cases during reinitialization.
Signed-off-by: Robert Richter <[email protected]>
---
arch/arm/oprofile/common.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/arch/arm/oprofile/common.c b/arch/arm/oprofile/common.c
index c3652f7..d660cb8 100644
--- a/arch/arm/oprofile/common.c
+++ b/arch/arm/oprofile/common.c
@@ -351,6 +351,8 @@ int __init oprofile_arch_init(struct oprofile_operations *ops)
{
int cpu, ret = 0;
+ memset(&perf_events, 0, sizeof(perf_events));
+
perf_num_counters = armpmu_get_max_events();
counter_config = kcalloc(perf_num_counters,
--
1.7.1.1
--
Advanced Micro Devices, Inc.
Operating System Research Center