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

[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

--

>From 6cb93a9063c579ec0183406933bfe07e8cc92c34 Mon Sep 17 00:00:00 2001
From: Robert Richter <robert.richter@amd.com>
Date: Wed, 29 Sep 2010 16:03:11 +0200
Subject: [PATCH] oprofile, arm: proper release resources on failure

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@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

Re: [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

Re: [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

Re: [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

Re: [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

Re: [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

[tip:perf/urgent] oprofile, ARM: Release resources on failure

[tip-bot for Robert Richter]

Commit-ID:  98d943b02f6f1b57787ff1aa6f34d019a407e3ee
Gitweb:     http://git.kernel.org/tip/98d943b02f6f1b57787ff1aa6f34d019a407e3ee
Author:     Robert Richter <robert.richter@amd.com>
AuthorDate: Wed, 29 Sep 2010 16:52:25 +0200
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Thu, 30 Sep 2010 09:14:25 +0200

oprofile, ARM: Release resources on failure

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

Signed-off-by: Robert Richter <robert.richter@amd.com>
Acked-by: Will Deacon <will.deacon@arm.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: <stable@kernel.org> # .35.x
LKML-Reference: <20100929145225.GJ13563@erda.amd.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 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)

[tip:perf/core] oprofile, ARM: Release resources on failure

[tip-bot for Robert Richter]

Commit-ID:  81771974ae49bf79aab60c42eac7a6d730a9ef2b
Gitweb:     http://git.kernel.org/tip/81771974ae49bf79aab60c42eac7a6d730a9ef2b
Author:     Robert Richter <robert.richter@amd.com>
AuthorDate: Wed, 29 Sep 2010 16:52:25 +0200
Committer:  Robert Richter <robert.richter@amd.com>
CommitDate: Mon, 11 Oct 2010 19:27:10 +0200

oprofile, ARM: Release resources on failure

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

Signed-off-by: Robert Richter <robert.richter@amd.com>
Acked-by: Will Deacon <will.deacon@arm.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: <stable@kernel.org> # .35.x
LKML-Reference: <20100929145225.GJ13563@erda.amd.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 drivers/oprofile/oprofile_perf.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/oprofile/oprofile_perf.c b/drivers/oprofile/oprofile_perf.c
index ebb40cb..f3d3df2 100644
--- a/drivers/oprofile/oprofile_perf.c
+++ b/drivers/oprofile/oprofile_perf.c
@@ -84,6 +84,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;