Messages in this thread Patches in this message | | | Date | Wed, 23 Nov 2011 00:35:21 +0530 | From | "Srivatsa S. Bhat" <> | Subject | Re: [PATCH] PM / Hibernation: Fix *massive* memory leak at early exits in hibernation |
| |
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; } /**
| |