[PATCH] PM / Hibernation: Fix *massive* memory leak at early exits in hibernation

[PATCH] PM / Hibernation: Fix *massive* memory leak at early exits in hibernation

[Srivatsa S. Bhat]

At some of the early exit points during hibernation (exiting either due
to failure or after a successful hibernation test, the memory pre-allocated
for hibernation is not freed up. And this is *very* serious, because, during
pre-allocation, it could have allocated upto a few *gigabytes* of memory!
And hence, if a hibernation fails or even if we run some hibernation tests
using the 'pm_test' framework, the system is rendered unstable due to memory
becoming signifantly lower. Fix this bug.

Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
---

 kernel/power/hibernate.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index 196c0126..dfe07fd 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -346,8 +346,10 @@ int hibernation_snapshot(int platform_mode)
 		goto Close;
 
 	error = freeze_kernel_threads();
-	if (error)
+	if (error) {
+		swsusp_free();
 		goto Close;
+	}
 
 	if (hibernation_test(TEST_FREEZER) ||
 		hibernation_testmode(HIBERNATION_TESTPROC)) {
@@ -357,12 +359,15 @@ int hibernation_snapshot(int platform_mode)
 		 * successful freezer test.
 		 */
 		freezer_test_done = true;
+		swsusp_free();
 		goto Close;
 	}
 
 	error = dpm_prepare(PMSG_FREEZE);
-	if (error)
+	if (error) {
+		swsusp_free();
 		goto Complete_devices;
+	}
 
 	suspend_console();
 	pm_restrict_gfp_mask();

Re: [PATCH] PM / Hibernation: Fix *massive* memory leak at early exits in hibernation

[Rafael J. Wysocki]

On Monday, November 21, 2011, Srivatsa S. Bhat wrote:
> At some of the early exit points during hibernation (exiting either due
> to failure or after a successful hibernation test, the memory pre-allocated
> for hibernation is not freed up. And this is *very* serious, because, during
> pre-allocation, it could have allocated upto a few *gigabytes* of memory!
> And hence, if a hibernation fails or even if we run some hibernation tests
> using the 'pm_test' framework, the system is rendered unstable due to memory
> becoming signifantly lower. Fix this bug.

While the observation is valid, I'd prefer to do something like the patch
below.

Thanks,
Rafael

---
From: Rafael J. Wysocki <rjw@sisk.pl>
Subject: PM / Hibernate: Do not leak memory in error/test code paths

The hibernation core code forgets to release memory preallocated
for hibernation if there's an error in its early stages or if test
modes causing hibernation_snapshot() to return early are used.  This
causes the system to be hardly usable, because the amount of
preallocated memory is usually huge.  Fix this problem.

Reported-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 kernel/power/hibernate.c |   16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

Index: linux/kernel/power/hibernate.c
===================================================================
--- linux.orig/kernel/power/hibernate.c
+++ linux/kernel/power/hibernate.c
@@ -347,7 +347,7 @@ int hibernation_snapshot(int platform_mo
 
 	error = freeze_kernel_threads();
 	if (error)
-		goto Close;
+		goto Cleanup;
 
 	if (hibernation_test(TEST_FREEZER) ||
 		hibernation_testmode(HIBERNATION_TESTPROC)) {
@@ -357,12 +357,14 @@ int hibernation_snapshot(int platform_mo
 		 * successful freezer test.
 		 */
 		freezer_test_done = true;
-		goto Close;
+		goto Cleanup;
 	}
 
 	error = dpm_prepare(PMSG_FREEZE);
-	if (error)
-		goto Complete_devices;
+	if (error) {
+		dpm_complete(msg);
+		goto Cleanup;
+	}
 
 	suspend_console();
 	pm_restrict_gfp_mask();
@@ -391,8 +393,6 @@ int hibernation_snapshot(int platform_mo
 		pm_restore_gfp_mask();
 
 	resume_console();
-
- Complete_devices:
 	dpm_complete(msg);
 
  Close:
@@ -402,6 +402,10 @@ int hibernation_snapshot(int platform_mo
  Recover_platform:
 	platform_recover(platform_mode);
 	goto Resume_devices;
+
+ Cleanup:
+	swsusp_free();
+	goto Close;
 }
 
 /**

Re: [PATCH] PM / Hibernation: Fix *massive* memory leak at early exits in hibernation

[Pavel Machek]

On Mon 2011-11-21 23:25:39, Rafael J. Wysocki wrote:
> On Monday, November 21, 2011, Srivatsa S. Bhat wrote:
> > At some of the early exit points during hibernation (exiting either due
> > to failure or after a successful hibernation test, the memory pre-allocated
> > for hibernation is not freed up. And this is *very* serious, because, during
> > pre-allocation, it could have allocated upto a few *gigabytes* of memory!
> > And hence, if a hibernation fails or even if we run some hibernation tests
> > using the 'pm_test' framework, the system is rendered unstable due to memory
> > becoming signifantly lower. Fix this bug.
> 
> While the observation is valid, I'd prefer to do something like the patch
> below.

The code slowly becomes goto maze :-(.

> @@ -357,12 +357,14 @@ int hibernation_snapshot(int platform_mo
>  		 * successful freezer test.
>  		 */
>  		freezer_test_done = true;
> -		goto Close;
> +		goto Cleanup;
>  	}
>  
>  	error = dpm_prepare(PMSG_FREEZE);
> -	if (error)
> -		goto Complete_devices;
> +	if (error) {
> +		dpm_complete(msg);
> +		goto Cleanup;
> +	}

Perhaps dpm_prepare should be changed to clean after itself in the
error case? That is the normal convention AFAICT....

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] PM / Hibernation: Fix *massive* memory leak at early exits in hibernation

[Srivatsa S. Bhat]

On 11/22/2011 05:15 PM, Pavel Machek wrote:
> On Mon 2011-11-21 23:25:39, Rafael J. Wysocki wrote:
>> On Monday, November 21, 2011, Srivatsa S. Bhat wrote:
>>> At some of the early exit points during hibernation (exiting either due
>>> to failure or after a successful hibernation test, the memory pre-allocated
>>> for hibernation is not freed up. And this is *very* serious, because, during
>>> pre-allocation, it could have allocated upto a few *gigabytes* of memory!
>>> And hence, if a hibernation fails or even if we run some hibernation tests
>>> using the 'pm_test' framework, the system is rendered unstable due to memory
>>> becoming signifantly lower. Fix this bug.
>>
>> While the observation is valid, I'd prefer to do something like the patch
>> below.
> 
> The code slowly becomes goto maze :-(.
> 

I agree.. It is already quite a mess.

>> @@ -357,12 +357,14 @@ int hibernation_snapshot(int platform_mo
>>  		 * successful freezer test.
>>  		 */
>>  		freezer_test_done = true;
>> -		goto Close;
>> +		goto Cleanup;
>>  	}
>>  
>>  	error = dpm_prepare(PMSG_FREEZE);
>> -	if (error)
>> -		goto Complete_devices;
>> +	if (error) {
>> +		dpm_complete(msg);
>> +		goto Cleanup;
>> +	}
> 
> Perhaps dpm_prepare should be changed to clean after itself in the
> error case? That is the normal convention AFAICT....
> 

If the intention here is to merely clean up hibernation_snapshot() code,
I would not prefer to change the behaviour of dpm_prepare(), considering
things like, what parameter should we pass to dpm_complete(); is the
resultant behaviour change in dpm_suspend_start() correct or not; what
happens to all the code that uses the nice pair: dpm_suspend_start() and
dpm_resume_end() and so on.

Perhaps there are bigger issues involved there, since I observed on a brief
look that the current code doesn't seem to strictly follow the above
convention that whoever called dpm_prepare() should call dpm_complete()
upon failure. Or may be its doing the right thing.. I don't know.

But anyway, the good news is, even without changing dpm_prepare()'s
behaviour, we can clean up quite a bit of code in hibernation_snapshot(),
as it is.

The first patch below does the cleanup, the second patch fixes the memory
leak and applies on top of the first patch.

---
From: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
Subject: PM / Hibernation: Refactor and simplify hibernation_snapshot() code

The goto statements in hibernation_snapshot() are a bit complex.
Refactor the code to remove some of them, thereby simplifying the
implementation.

Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
---

 kernel/power/hibernate.c |   28 ++++++++++++----------------
 1 files changed, 12 insertions(+), 16 deletions(-)

diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index 196c0126..fe7e83e 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -333,7 +333,7 @@ static int create_image(int platform_mode)
  */
 int hibernation_snapshot(int platform_mode)
 {
-	pm_message_t msg = PMSG_RECOVER;
+	pm_message_t msg;
 	int error;
 
 	error = platform_begin(platform_mode);
@@ -361,25 +361,27 @@ int hibernation_snapshot(int platform_mode)
 	}
 
 	error = dpm_prepare(PMSG_FREEZE);
-	if (error)
-		goto Complete_devices;
+	if (error) {
+		dpm_complete(PMSG_RECOVER);
+		goto Close;
+	}
 
 	suspend_console();
 	pm_restrict_gfp_mask();
+
 	error = dpm_suspend(PMSG_FREEZE);
-	if (error)
-		goto Recover_platform;
 
-	if (hibernation_test(TEST_DEVICES))
-		goto Recover_platform;
+	if (error || hibernation_test(TEST_DEVICES))
+		platform_recover(platform_mode);
+	else
+		error = create_image(platform_mode);
 
-	error = create_image(platform_mode);
 	/*
-	 * Control returns here (1) after the image has been created or the
+	 * In the case that we call create_image() above, the control
+	 * returns here (1) after the image has been created or the
 	 * image creation has failed and (2) after a successful restore.
 	 */
 
- Resume_devices:
 	/* We may need to release the preallocated image pages here. */
 	if (error || !in_suspend)
 		swsusp_free();
@@ -392,16 +394,10 @@ int hibernation_snapshot(int platform_mode)
 
 	resume_console();
 
- Complete_devices:
-	dpm_complete(msg);
-
  Close:
 	platform_end(platform_mode);
 	return error;
 
- Recover_platform:
-	platform_recover(platform_mode);
-	goto Resume_devices;
 }
 
 /**



==============================================
The second patch:

---
From: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
Subject: PM / Hibernation: Fix memory leak in hibernation at error/test paths

At some points in the hibernation code, if we exit early either due to an
error or a successful hibernation test, the memory pre-allocated for
hibernation is not freed up, which might be quite significant. Fix this
memory leak.

Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
---

 kernel/power/hibernate.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index fe7e83e..7aea7e5 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -347,7 +347,7 @@ int hibernation_snapshot(int platform_mode)
 
 	error = freeze_kernel_threads();
 	if (error)
-		goto Close;
+		goto Free_mem;
 
 	if (hibernation_test(TEST_FREEZER) ||
 		hibernation_testmode(HIBERNATION_TESTPROC)) {
@@ -357,13 +357,13 @@ int hibernation_snapshot(int platform_mode)
 		 * successful freezer test.
 		 */
 		freezer_test_done = true;
-		goto Close;
+		goto Free_mem;
 	}
 
 	error = dpm_prepare(PMSG_FREEZE);
 	if (error) {
 		dpm_complete(PMSG_RECOVER);
-		goto Close;
+		goto Free_mem;
 	}
 
 	suspend_console();
@@ -398,6 +398,9 @@ int hibernation_snapshot(int platform_mode)
 	platform_end(platform_mode);
 	return error;
 
+ Free_mem:
+	swsusp_free();
+	goto Close;
 }
 
 /**

Re: [PATCH] PM / Hibernation: Fix *massive* memory leak at early exits in hibernation

[Rafael J. Wysocki]

On Tuesday, November 22, 2011, Srivatsa S. Bhat wrote:
> On 11/22/2011 05:15 PM, Pavel Machek wrote:
> > On Mon 2011-11-21 23:25:39, Rafael J. Wysocki wrote:
> >> On Monday, November 21, 2011, Srivatsa S. Bhat wrote:
> >>> At some of the early exit points during hibernation (exiting either due
> >>> to failure or after a successful hibernation test, the memory pre-allocated
> >>> for hibernation is not freed up. And this is *very* serious, because, during
> >>> pre-allocation, it could have allocated upto a few *gigabytes* of memory!
> >>> And hence, if a hibernation fails or even if we run some hibernation tests
> >>> using the 'pm_test' framework, the system is rendered unstable due to memory
> >>> becoming signifantly lower. Fix this bug.
> >>
> >> While the observation is valid, I'd prefer to do something like the patch
> >> below.
> > 
> > The code slowly becomes goto maze :-(.
> > 
> 
> I agree.. It is already quite a mess.
> 
> >> @@ -357,12 +357,14 @@ int hibernation_snapshot(int platform_mo
> >>  		 * successful freezer test.
> >>  		 */
> >>  		freezer_test_done = true;
> >> -		goto Close;
> >> +		goto Cleanup;
> >>  	}
> >>  
> >>  	error = dpm_prepare(PMSG_FREEZE);
> >> -	if (error)
> >> -		goto Complete_devices;
> >> +	if (error) {
> >> +		dpm_complete(msg);
> >> +		goto Cleanup;
> >> +	}
> > 
> > Perhaps dpm_prepare should be changed to clean after itself in the
> > error case? That is the normal convention AFAICT....
> > 
> 
> If the intention here is to merely clean up hibernation_snapshot() code,
> I would not prefer to change the behaviour of dpm_prepare(), considering
> things like, what parameter should we pass to dpm_complete(); is the
> resultant behaviour change in dpm_suspend_start() correct or not; what
> happens to all the code that uses the nice pair: dpm_suspend_start() and
> dpm_resume_end() and so on.
> 
> Perhaps there are bigger issues involved there, since I observed on a brief
> look that the current code doesn't seem to strictly follow the above
> convention that whoever called dpm_prepare() should call dpm_complete()
> upon failure. Or may be its doing the right thing.. I don't know.
> 
> But anyway, the good news is, even without changing dpm_prepare()'s
> behaviour, we can clean up quite a bit of code in hibernation_snapshot(),
> as it is.
> 
> The first patch below does the cleanup, the second patch fixes the memory
> leak and applies on top of the first patch.

Wait, wait.  These changes can be made in the 3.3 merge window, while I'd
like the fix the bug _now_.

Does anyone have any _technical_ problem with my patch posted previously
in this thread?

Rafael

Re: [PATCH] PM / Hibernation: Fix *massive* memory leak at early exits in hibernation

[Srivatsa S. Bhat]

On 11/23/2011 02:02 AM, Rafael J. Wysocki wrote:
> On Tuesday, November 22, 2011, Srivatsa S. Bhat wrote:
>> On 11/22/2011 05:15 PM, Pavel Machek wrote:
>>> On Mon 2011-11-21 23:25:39, Rafael J. Wysocki wrote:
>>>> On Monday, November 21, 2011, Srivatsa S. Bhat wrote:
>>>>> At some of the early exit points during hibernation (exiting either due
>>>>> to failure or after a successful hibernation test, the memory pre-allocated
>>>>> for hibernation is not freed up. And this is *very* serious, because, during
>>>>> pre-allocation, it could have allocated upto a few *gigabytes* of memory!
>>>>> And hence, if a hibernation fails or even if we run some hibernation tests
>>>>> using the 'pm_test' framework, the system is rendered unstable due to memory
>>>>> becoming signifantly lower. Fix this bug.
>>>>
>>>> While the observation is valid, I'd prefer to do something like the patch
>>>> below.
>>>
>>> The code slowly becomes goto maze :-(.
>>>
>>
>> I agree.. It is already quite a mess.
>>
>>>> @@ -357,12 +357,14 @@ int hibernation_snapshot(int platform_mo
>>>>  		 * successful freezer test.
>>>>  		 */
>>>>  		freezer_test_done = true;
>>>> -		goto Close;
>>>> +		goto Cleanup;
>>>>  	}
>>>>  
>>>>  	error = dpm_prepare(PMSG_FREEZE);
>>>> -	if (error)
>>>> -		goto Complete_devices;
>>>> +	if (error) {
>>>> +		dpm_complete(msg);
>>>> +		goto Cleanup;
>>>> +	}
>>>
>>> Perhaps dpm_prepare should be changed to clean after itself in the
>>> error case? That is the normal convention AFAICT....
>>>
>>
>> If the intention here is to merely clean up hibernation_snapshot() code,
>> I would not prefer to change the behaviour of dpm_prepare(), considering
>> things like, what parameter should we pass to dpm_complete(); is the
>> resultant behaviour change in dpm_suspend_start() correct or not; what
>> happens to all the code that uses the nice pair: dpm_suspend_start() and
>> dpm_resume_end() and so on.
>>
>> Perhaps there are bigger issues involved there, since I observed on a brief
>> look that the current code doesn't seem to strictly follow the above
>> convention that whoever called dpm_prepare() should call dpm_complete()
>> upon failure. Or may be its doing the right thing.. I don't know.
>>
>> But anyway, the good news is, even without changing dpm_prepare()'s
>> behaviour, we can clean up quite a bit of code in hibernation_snapshot(),
>> as it is.
>>
>> The first patch below does the cleanup, the second patch fixes the memory
>> leak and applies on top of the first patch.
> 
> Wait, wait.  These changes can be made in the 3.3 merge window, while I'd
> like the fix the bug _now_.
> 
> Does anyone have any _technical_ problem with my patch posted previously
> in this thread?
> 

Technically, your patch is fine :-)

Acked-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>

Thanks,
Srivatsa S. Bhat

Re: [PATCH] PM / Hibernation: Fix *massive* memory leak at early exits in hibernation

[Rafael J. Wysocki]

On Tuesday, November 22, 2011, Srivatsa S. Bhat wrote:
> On 11/23/2011 02:02 AM, Rafael J. Wysocki wrote:
> > On Tuesday, November 22, 2011, Srivatsa S. Bhat wrote:
> >> On 11/22/2011 05:15 PM, Pavel Machek wrote:
> >>> On Mon 2011-11-21 23:25:39, Rafael J. Wysocki wrote:
> >>>> On Monday, November 21, 2011, Srivatsa S. Bhat wrote:
> >>>>> At some of the early exit points during hibernation (exiting either due
> >>>>> to failure or after a successful hibernation test, the memory pre-allocated
> >>>>> for hibernation is not freed up. And this is *very* serious, because, during
> >>>>> pre-allocation, it could have allocated upto a few *gigabytes* of memory!
> >>>>> And hence, if a hibernation fails or even if we run some hibernation tests
> >>>>> using the 'pm_test' framework, the system is rendered unstable due to memory
> >>>>> becoming signifantly lower. Fix this bug.
> >>>>
> >>>> While the observation is valid, I'd prefer to do something like the patch
> >>>> below.
> >>>
> >>> The code slowly becomes goto maze :-(.
> >>>
> >>
> >> I agree.. It is already quite a mess.
> >>
> >>>> @@ -357,12 +357,14 @@ int hibernation_snapshot(int platform_mo
> >>>>  		 * successful freezer test.
> >>>>  		 */
> >>>>  		freezer_test_done = true;
> >>>> -		goto Close;
> >>>> +		goto Cleanup;
> >>>>  	}
> >>>>  
> >>>>  	error = dpm_prepare(PMSG_FREEZE);
> >>>> -	if (error)
> >>>> -		goto Complete_devices;
> >>>> +	if (error) {
> >>>> +		dpm_complete(msg);
> >>>> +		goto Cleanup;
> >>>> +	}
> >>>
> >>> Perhaps dpm_prepare should be changed to clean after itself in the
> >>> error case? That is the normal convention AFAICT....
> >>>
> >>
> >> If the intention here is to merely clean up hibernation_snapshot() code,
> >> I would not prefer to change the behaviour of dpm_prepare(), considering
> >> things like, what parameter should we pass to dpm_complete(); is the
> >> resultant behaviour change in dpm_suspend_start() correct or not; what
> >> happens to all the code that uses the nice pair: dpm_suspend_start() and
> >> dpm_resume_end() and so on.
> >>
> >> Perhaps there are bigger issues involved there, since I observed on a brief
> >> look that the current code doesn't seem to strictly follow the above
> >> convention that whoever called dpm_prepare() should call dpm_complete()
> >> upon failure. Or may be its doing the right thing.. I don't know.
> >>
> >> But anyway, the good news is, even without changing dpm_prepare()'s
> >> behaviour, we can clean up quite a bit of code in hibernation_snapshot(),
> >> as it is.
> >>
> >> The first patch below does the cleanup, the second patch fixes the memory
> >> leak and applies on top of the first patch.
> > 
> > Wait, wait.  These changes can be made in the 3.3 merge window, while I'd
> > like the fix the bug _now_.
> > 
> > Does anyone have any _technical_ problem with my patch posted previously
> > in this thread?
> > 
> 
> Technically, your patch is fine :-)
> 
> Acked-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>

Thanks!

Please feel free to make the cleanup on top of it.

Rafael

Re: [PATCH] PM / Hibernation: Fix *massive* memory leak at early exits in hibernation

[Pavel Machek]

Hi!

> > The first patch below does the cleanup, the second patch fixes the memory
> > leak and applies on top of the first patch.
> 
> Wait, wait.  These changes can be made in the 3.3 merge window, while I'd
> like the fix the bug _now_.
> 
> Does anyone have any _technical_ problem with my patch posted previously
> in this thread?

No, it was ok.

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] PM / Hibernation: Fix *massive* memory leak at early exits in hibernation

[Rafael J. Wysocki]

On Tuesday, November 22, 2011, Pavel Machek wrote:
> On Mon 2011-11-21 23:25:39, Rafael J. Wysocki wrote:
> > On Monday, November 21, 2011, Srivatsa S. Bhat wrote:
> > > At some of the early exit points during hibernation (exiting either due
> > > to failure or after a successful hibernation test, the memory pre-allocated
> > > for hibernation is not freed up. And this is *very* serious, because, during
> > > pre-allocation, it could have allocated upto a few *gigabytes* of memory!
> > > And hence, if a hibernation fails or even if we run some hibernation tests
> > > using the 'pm_test' framework, the system is rendered unstable due to memory
> > > becoming signifantly lower. Fix this bug.
> > 
> > While the observation is valid, I'd prefer to do something like the patch
> > below.
> 
> The code slowly becomes goto maze :-(.
> 
> > @@ -357,12 +357,14 @@ int hibernation_snapshot(int platform_mo
> >  		 * successful freezer test.
> >  		 */
> >  		freezer_test_done = true;
> > -		goto Close;
> > +		goto Cleanup;
> >  	}
> >  
> >  	error = dpm_prepare(PMSG_FREEZE);
> > -	if (error)
> > -		goto Complete_devices;
> > +	if (error) {
> > +		dpm_complete(msg);
> > +		goto Cleanup;
> > +	}
> 
> Perhaps dpm_prepare should be changed to clean after itself in the
> error case? That is the normal convention AFAICT....

Yes, in a future patch.

Thanks,
Rafael