[PATCH] oprofile, arm: proper release resources on failure

[PATCH] oprofile, arm: proper release resources on failure

[Robert Richter]

This patch fixes a resource leak on failure, where the oprofilefs and
some counters may not released properly.

Fix is also for 2.6.35-stable.

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

diff --git a/arch/arm/oprofile/common.c b/arch/arm/oprofile/common.c
index 0691176..72e09eb 100644
--- a/arch/arm/oprofile/common.c
+++ b/arch/arm/oprofile/common.c
@@ -102,6 +102,7 @@ static int op_create_counter(int cpu, int event)
 	if (IS_ERR(pevent)) {
 		ret = PTR_ERR(pevent);
 	} else 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;
@@ -365,6 +366,7 @@ int __init oprofile_arch_init(struct oprofile_operations *ops)
 	ret = init_driverfs();
 	if (ret) {
 		kfree(counter_config);
+		counter_config = NULL;
 		return ret;
 	}
 
@@ -402,7 +404,6 @@ void oprofile_arch_exit(void)
 	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];
@@ -413,8 +414,10 @@ void oprofile_arch_exit(void)
 		}
 	}
 
-	if (counter_config)
+	if (counter_config) {
 		kfree(counter_config);
+		exit_driverfs();
+	}
 }
 #else
 int __init oprofile_arch_init(struct oprofile_operations *ops)
-- 
1.7.2.2



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

[PATCH] oprofile, arm: proper release resources on failure

[Robert Richter]

Will,

the patch below fixes a resource leak I found during code review. Can
you please review and test it (I don't have an ARM environment for
this available). If you are fine with the change, please ack. I want
to send it upstream via tip/perf/urgent.

Though you reworked the code already, parts of the fix are still valid
for latest oprofile code in oprofile/core.

Ingo,

if Will agrees with it, please apply it directly to tip/perf/urgent.

Thanks,

-Robert

--

[PATCH] oprofile, arm: proper release resources on failure

[Will Deacon]

On Wed, 2010-09-29 at 15:52 +0100, Robert Richter wrote:
> Will,
> 

Hi Robert,


> the patch below fixes a resource leak I found during code review. Can
> you please review and test it (I don't have an ARM environment for
> this available). If you are fine with the change, please ack. I want
> to send it upstream via tip/perf/urgent.
> 

I have a few minor clarifications:
> 

> diff --git a/arch/arm/oprofile/common.c b/arch/arm/oprofile/common.c
> index 0691176..72e09eb 100644
> --- a/arch/arm/oprofile/common.c
> +++ b/arch/arm/oprofile/common.c
> @@ -102,6 +102,7 @@ static int op_create_counter(int cpu, int event)
>         if (IS_ERR(pevent)) {
>                 ret = PTR_ERR(pevent);
>         } else 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;


Yup, this is needed. Thanks! It would be nice to do away with the
else statement altogether but I think adding a pinned event and
then failing to activate it still succeeds in
perf_event_create_kernel_counter.


> @@ -365,6 +366,7 @@ int __init oprofile_arch_init(struct oprofile_operations *ops)
>         ret = init_driverfs();
>         if (ret) {
>                 kfree(counter_config);
> +               counter_config = NULL;
>                 return ret;
>         }
> 
> @@ -402,7 +404,6 @@ void oprofile_arch_exit(void)
>         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];
> @@ -413,8 +414,10 @@ void oprofile_arch_exit(void)
>                 }
>         }
> 
> -       if (counter_config)
> +       if (counter_config) {
>                 kfree(counter_config);
> +               exit_driverfs();
> +       }
>  }
>  #else
>  int __init oprofile_arch_init(struct oprofile_operations *ops)
> --
> 1.7.2.2
> 

Hmm, these three hunks conflict with the patches I posted last
month to fix the resource allocation and freeing. Can't we
merge those patches instead? I have versions against -rc6 here:

git://linux-arm.org/linux-2.6-wd.git oprofile-mm

Cheers,

Will

[PATCH] oprofile, arm: proper release resources on failure

[Robert Richter]

On 29.09.10 11:39:44, Will Deacon wrote:

> I have a few minor clarifications:
> > 
> 
> > diff --git a/arch/arm/oprofile/common.c b/arch/arm/oprofile/common.c
> > index 0691176..72e09eb 100644
> > --- a/arch/arm/oprofile/common.c
> > +++ b/arch/arm/oprofile/common.c
> > @@ -102,6 +102,7 @@ static int op_create_counter(int cpu, int event)
> >         if (IS_ERR(pevent)) {
> >                 ret = PTR_ERR(pevent);
> >         } else 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;
> 
> 
> Yup, this is needed. Thanks! It would be nice to do away with the
> else statement altogether but I think adding a pinned event and
> then failing to activate it still succeeds in
> perf_event_create_kernel_counter.

Yes, I have cleanup code already for this. But as this is an -rc and
stable fix, I didn't want to change too much here. But I will send a
follow-on patch with the cleanup I made.

> 
> 
> > @@ -365,6 +366,7 @@ int __init oprofile_arch_init(struct oprofile_operations *ops)
> >         ret = init_driverfs();
> >         if (ret) {
> >                 kfree(counter_config);
> > +               counter_config = NULL;
> >                 return ret;
> >         }
> > 
> > @@ -402,7 +404,6 @@ void oprofile_arch_exit(void)
> >         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];
> > @@ -413,8 +414,10 @@ void oprofile_arch_exit(void)
> >                 }
> >         }
> > 
> > -       if (counter_config)
> > +       if (counter_config) {
> >                 kfree(counter_config);
> > +               exit_driverfs();
> > +       }
> >  }
> >  #else
> >  int __init oprofile_arch_init(struct oprofile_operations *ops)
> > --
> > 1.7.2.2
> > 
> 
> Hmm, these three hunks conflict with the patches I posted last
> month to fix the resource allocation and freeing. Can't we
> merge those patches instead? I have versions against -rc6 here:

Yes, these patches are also in my oprofile/core branch and its changes
conflict, but are scheduled for the next merge window. This fix is for
urgent and for 2.6.36. We will then merge back rc7 or 2.6.36 to
oprofile/core and solve the conflicts. But if you don't have something
else for -rc7/.36 that conflicts, this should be ok.

-Robert

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

[PATCH] oprofile, arm: proper release resources on failure

[Robert Richter]

On 29.09.10 18:50:27, Robert Richter wrote:

> > Hmm, these three hunks conflict with the patches I posted last
> > month to fix the resource allocation and freeing. Can't we
> > merge those patches instead? I have versions against -rc6 here:
> 
> Yes, these patches are also in my oprofile/core branch and its changes
> conflict, but are scheduled for the next merge window. This fix is for
> urgent and for 2.6.36. We will then merge back rc7 or 2.6.36 to
> oprofile/core and solve the conflicts. But if you don't have something
> else for -rc7/.36 that conflicts, this should be ok.

So, it would be ok then to test only this fix on -rc6 without your
other patches.

Thanks,

-Robert

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

[PATCH] oprofile, arm: proper release resources on failure

[Will Deacon]

Robert,

On Wed, 2010-09-29 at 17:59 +0100, Robert Richter wrote:
> On 29.09.10 18:50:27, Robert Richter wrote:
> 
> > > Hmm, these three hunks conflict with the patches I posted last
> > > month to fix the resource allocation and freeing. Can't we
> > > merge those patches instead? I have versions against -rc6 here:
> >
> > Yes, these patches are also in my oprofile/core branch and its changes
> > conflict, but are scheduled for the next merge window. This fix is for
> > urgent and for 2.6.36. We will then merge back rc7 or 2.6.36 to
> > oprofile/core and solve the conflicts. But if you don't have something
> > else for -rc7/.36 that conflicts, this should be ok.
> 
> So, it would be ok then to test only this fix on -rc6 without your
> other patches.
> 

Gotcha - this is a minimal fixup for -stable which we'll tackle properly
in the merge window with the stuff in oprofile/core. In that case I'm
happy for this to go upstream now:

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


I also built an -rc6 kernel with this change on top and OProfile worked
happily across module loads/unloads.

As for oprofile/core; will you handle the conflicts there or do I need
to resend my previous patches?

Cheers,

Will

[PATCH] oprofile, arm: proper release resources on failure

[Robert Richter]

On 29.09.10 13:32:58, Will Deacon wrote:
> Robert,
> 
> On Wed, 2010-09-29 at 17:59 +0100, Robert Richter wrote:
> > On 29.09.10 18:50:27, Robert Richter wrote:
> > 
> > > > Hmm, these three hunks conflict with the patches I posted last
> > > > month to fix the resource allocation and freeing. Can't we
> > > > merge those patches instead? I have versions against -rc6 here:
> > >
> > > Yes, these patches are also in my oprofile/core branch and its changes
> > > conflict, but are scheduled for the next merge window. This fix is for
> > > urgent and for 2.6.36. We will then merge back rc7 or 2.6.36 to
> > > oprofile/core and solve the conflicts. But if you don't have something
> > > else for -rc7/.36 that conflicts, this should be ok.
> > 
> > So, it would be ok then to test only this fix on -rc6 without your
> > other patches.
> > 
> 
> Gotcha - this is a minimal fixup for -stable which we'll tackle properly
> in the merge window with the stuff in oprofile/core. In that case I'm
> happy for this to go upstream now:
> 
> Acked-by: Will Deacon <will.deacon@arm.com>
> 
> 
> I also built an -rc6 kernel with this change on top and OProfile worked
> happily across module loads/unloads.
> 
> As for oprofile/core; will you handle the conflicts there or do I need
> to resend my previous patches?

Yes, I will merge it after it went upstream and let you know.

-Robert

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