fix oops after saving image
Subsystems (2)
Hi!
I was seeing strange oopsen after saving image. They are almost
harmless: anything that happens after writing final signature does not matter (as long as it does not write to disk). But that oops makes testing hard as you have to manually powerdown the machine.
free_page() at this point is unneccessary as machine is going down, anyway. Please apply,
Pavel
PS: For a test, I'm running while true; do echo 4 > /proc/acpi/sleep; sleep 30; done while making kernel. It seems to work so far.
kernel/power/swsusp.c renamed from linux/kernel/power/swsusp.c +1 -1
@@ -345,7 +348,7 @@printk ("|\n" );MDELAY (1000 ); -free_page ((unsigned long ) buffer); +/* Trying to free_page((unsigned long) buffer) here is bad idea, not sure why */ return 0 ; }
Signature · 2 lines
When do you have a heart between your knees? [Johanka's followup: and *two* hearts?]
kernel/power/swsusp.c line 353
Quoted 11 lines
kernel/power/swsusp.c renamed from linux/kernel/power/swsusp.c +1
-1@@ -345,7 +348,7 @@printk ("|\n" );MDELAY (1000 ); -free_page ((unsigned long ) buffer); +/* Trying to free_page((unsigned long) buffer) here is bad idea, not sure why */ return 0 ; }
Patches like this really do a disservice to anyone trying to read the code and figure out what is going on. I've spent a considerable amount of time deciphering and santizing the swsusp code, which is why pmdisk exists.
The patch is simply a band-aid, and completely meaningless without the context of the email. If I applied this, one would be able to ascertain the reason for the patch, if they manipulated the BK tools correctly. However, seeing that line solely in the context on the rest of the source makes one cock their head, squint their eyes and pray that they never have to look at that file again.
If you're seeing an Oops, please search more for the cause and submit a real fix for it.
Or, simply change the semantics of the code enough to eliminate the possibility of a problem. In the pmdisk code, I've statically declared the header, so we don't need that alloc/free. Please see that file for an example.
Pat
Hi!
Quoted 22 lines
kernel/power/swsusp.c line 353
Quoted 10 lines
kernel/power/swsusp.c renamed from linux/kernel/power/swsusp.c +1
-1@@ -345,7 +348,7 @@printk ("|\n" );MDELAY (1000 ); -free_page ((unsigned long ) buffer); +/* Trying to free_page((unsigned long) buffer) here is bad idea, not sure why */ return 0 ; }Patches like this really do a disservice to anyone trying to read the code and figure out what is going on. I've spent a considerable amount of time deciphering and santizing the swsusp code, which is why pmdisk exists.
The patch is simply a band-aid, and completely meaningless without the context of the email. If I applied this, one would be able to ascertain the reason for the patch, if they manipulated the BK tools correctly. However, seeing that line solely in the context on the rest of the source makes one cock their head, squint their eyes and pray that they never have to look at that file again.
Quoted 7 lines
If you're seeing an Oops, please search more for the cause and submit a real fix for it.
Or, simply change the semantics of the code enough to eliminate the possibility of a problem. In the pmdisk code, I've statically declared the header, so we don't need that alloc/free. Please see that file for an example.
I do not want to waste 4K, does this look better?
Pavel
kernel/power/swsusp.c renamed from linux/kernel/power/swsusp.c +4 -1
@@ -283,6 +283,9 @@unsigned long address;struct page *page; +if (!buffer) +return -ENOMEM ; +printk ("Writing data to swap (%d pages): " , nr_copy_pages );for (i=0 ; i<nr_copy_pages; i++) {if (!(i%100 ))
@@ -345,7 +348,7 @@printk ("|\n" );MDELAY (1000 ); -free_page ((unsigned long ) buffer); +/* No need to free anything, system is going down, anyway. */ return 0 ; }
Signature · 2 lines
When do you have a heart between your knees? [Johanka's followup: and *two* hearts?]
Quoted 7 lines
I do not want to waste 4K, does this look better?
Pavel
kernel/power/swsusp.c line 291
Quoted 9 lines
kernel/power/swsusp.c renamed from linux/kernel/power/swsusp.c +3
-0@@ -283,6 +283,9 @@unsigned long address;struct page *page; +if (!buffer) +return -ENOMEM ; +printk ("Writing data to swap (%d pages): " , nr_copy_pages );for (i=0 ; i<nr_copy_pages; i++) {if (!(i%100 ))
Argh! This bit was in the previous patch I applied. Please get it straight, or just keep adding to one patch and resending it (with an itemized list of changes).
kernel/power/swsusp.c line 353
Quoted 8 lines
kernel/power/swsusp.c renamed from linux/kernel/power/swsusp.c +1
-1@@ -345,7 +348,7 @@printk ("|\n" );MDELAY (1000 ); -free_page ((unsigned long ) buffer); +/* No need to free anything, system is going down, anyway. */ return 0 ; }
It's technically still incorrect, since you'd still be leaking memory if suspend failed. And, the comment is still in an unfortunate place. Something like this, before the function helps to provide understanding of the entire operation:
/**
* write_suspend_image - Write entire image to disk.
*
* ...
*
* Note: The buffer we allocate to use to write the suspend header is
* not freed because it otherwise causes a random Oops that I can't
* find. This is a band-aid that probably only masks the problem, and
* technically bad, since it means we leak memory unconditionally if
* suspend fails.
*/
static int write_suspend_image(void)
...
Pat
Hi!
Quoted 9 lines
Quoted 9 lines
I do not want to waste 4K, does this look better?
Pavelkernel/power/swsusp.c renamed from linux/kernel/power/swsusp.c +0
-0@@ -283,6 +283,9 @@unsigned long address;struct page *page;
Quoted 10 lines
kernel/power/swsusp.c line 291
Quoted 6 lines
kernel/power/swsusp.c renamed from linux/kernel/power/swsusp.c +3
-0+if (!buffer) +return -ENOMEM ; +printk ("Writing data to swap (%d pages): " , nr_copy_pages );for (i=0 ; i<nr_copy_pages; i++) {if (!(i%100 ))Argh! This bit was in the previous patch I applied. Please get it straight, or just keep adding to one patch and resending it (with an itemized list of changes).
I do not want to do that (other people would have problems reviewing). I'll try to be more carefull.
One thing would help me: could you reply with "Applied" when you apply some patch? Just now it seems to be silence==applied and reply==some problem, but that makes it "interesting" to guess what your tree looks like.
Quoted 13 lines
kernel/power/swsusp.c line 353
Quoted 8 lines
kernel/power/swsusp.c renamed from linux/kernel/power/swsusp.c +1
-1@@ -345,7 +348,7 @@printk ("|\n" );MDELAY (1000 ); -free_page ((unsigned long ) buffer); +/* No need to free anything, system is going down, anyway. */ return 0 ; }It's technically still incorrect, since you'd still be leaking memory if suspend failed. And, the comment is still in an unfortunate place. Something like this, before the function helps to provide understanding of the entire operation:
At this point, suspend may no longer fail: we have ready-to-run image in swap, and rollback would happen on next reboot -- corrupting data. Yep better docs is needed.
How about this one?
Pavel
kernel/power/swsusp.c renamed from linux/kernel/power/swsusp.c +11 -1
@@ -274,6 +274,17 @@swap_list_unlock (); } +/** +* write_suspend_image - Write entire image to disk. +* +* After writing suspend signature to the disk, suspend may no +* longer fail: we have ready-to-run image in swap, and rollback +* would happen on next reboot -- corrupting data. +* +* Note: The buffer we allocate to use to write the suspend header is +* not freed; its not needed since system is going down anyway +* (plus it causes oops and I'm lazy^H^H^H^Htoo busy). +*/ static int write_suspend_image (void ) {int i;
@@ -345,7 +359,6 @@printk ("|\n" );MDELAY (1000 ); -free_page ((unsigned long ) buffer);return 0 ; }
Signature · 2 lines
When do you have a heart between your knees? [Johanka's followup: and *two* hearts?]
Quoted 4 lines · Pavel Machek
+ * Note: The buffer we allocate to use to write the suspend header is + * not freed; its not needed since system is going down anyway + * (plus it causes oops and I'm lazy^H^H^H^Htoo busy).+ */
Too lazy to properly fix your comment as well.
-sb
Hi!
Quoted 6 lines
Quoted 4 lines
+ * Note: The buffer we allocate to use to write the suspend header is + * not freed; its not needed since system is going down anyway + * (plus it causes oops and I'm lazy^H^H^H^Htoo busy).+ */
Too lazy to properly fix your comment as well.
Can you elaborate?
Pavel
Signature · 2 lines
When do you have a heart between your knees? [Johanka's followup: and *two* hearts?]
I guess he means...
Quoted 1 line · Pavel Machek
Quoted 1 line
Quoted 1 line
+ * not freed; its not needed since system is going down anyway
should be
Quoted 1 line
Quoted 1 line
+ * not freed; it's not needed since the system is going down anyway
(it's = it is, its = belongs to it)
and
Quoted 1 line
Quoted 1 line
Quoted 1 line
+ * (plus it causes oops and I'm lazy^H^H^H^Htoo busy).
should be
Quoted 1 line
Quoted 1 line
+ * (plus it causes an oops and I'm too lazy^H^H^H^Hbusy).
Regards,
Nigel
Signature · 6 lines
Nigel Cunningham 495 St Georges Road South, Hastings 4201, New Zealand You see, at just the right time, when we were still powerless, Christ died for the ungodly. -- Romans 5:6, NIV.
Yeah that too :)
Quoted 30 lines · Nigel Cunningham
I guess he means...
Quoted 1 line · Pavel Machek
Quoted 1 line
Quoted 1 line
+ * not freed; its not needed since system is going down anyway
should be
Quoted 1 line
Quoted 1 line
+ * not freed; it's not needed since the system is going down anyway
(it's = it is, its = belongs to it)
and
Quoted 1 line
Quoted 1 line
Quoted 1 line
+ * (plus it causes oops and I'm lazy^H^H^H^Htoo busy).
should be
Quoted 1 line
Quoted 1 line
+ * (plus it causes an oops and I'm too lazy^H^H^H^Hbusy).
Regards,
Nigel
Quoted 8 lines · Pavel Machek
Hi!
Quoted 5 lines
Quoted 4 lines
+ * Note: The buffer we allocate to use to write the suspend header is + * not freed; its not needed since system is going down anyway + * (plus it causes oops and I'm lazy^H^H^H^Htoo busy).+ */
Quoted 5 lines
Quoted 1 line
Too lazy to properly fix your comment as well.
Can you elaborate?
Pavel
Pavel what mail client are you using? The last comment reads:
+ * (plus it causes oops and I'm lazy<CTRL-H><CTRL-H><CTRL-H><CTRL-H>too busy).
I spelled out the ^H so you can see them, that is how the comment looks to me. The word 'lazy' is still in there along with too busy, you never backspaced over the lazy in the comment. That's all :P
-sb
Hi!
Quoted 16 lines
Quoted 10 lines
Quoted 6 lines
Quoted 4 lines
+ * Note: The buffer we allocate to use to write the suspend header is + * not freed; its not needed since system is going down anyway + * (plus it causes oops and I'm lazy^H^H^H^Htoo busy).+ */
Too lazy to properly fix your comment as well.
Can you elaborate?
PavelPavel what mail client are you using? The last comment reads:
+ * (plus it causes oops and I'm lazy<CTRL-H><CTRL-H><CTRL-H><CTRL-H>too
busy).
Quoted 4 lines
I spelled out the ^H so you can see them, that is how the comment looks to me. The word 'lazy' is still in there along with too busy, you never backspaced over the lazy in the comment. That's all :P
:-))))))))))))))))))))))
No, my mail client is okay. I actually wrote ^ and H, and that should be a joke.
Pavel
Signature · 2 lines
When do you have a heart between your knees? [Johanka's followup: and *two* hearts?]
Quoted 6 lines · Pavel Machek
:-))))))))))))))))))))))
No, my mail client is okay. I actually wrote ^ and H, and that should be a joke.
Pavel
lol my b you da man :)
-sb
Patrick Mochel
AppliedQuoted 5 lines
One thing would help me: could you reply with "Applied" when you apply some patch? Just now it seems to be silence==applied and reply==some problem, but that makes it "interesting" to guess what your tree looks like.
Yes, I'll try to be better about this.
Quoted 5 lines
At this point, suspend may no longer fail: we have ready-to-run image in swap, and rollback would happen on next reboot -- corrupting data. Yep better docs is needed.
How about this one?
Applied.
Pat