Memory corruption during hibernation since 2.6.31
in [Kernel]
|
Prev: [PATCH 0/5] use idr kernel library instead of handles
Next: [PATCH v2] cpuidle: extend cpuidle and menu governor to handle dynamic states
From: KAMEZAWA Hiroyuki on 29 Jul 2010 00:30 On Wed, 28 Jul 2010 23:34:07 +0200 "Rafael J. Wysocki" <rjw(a)sisk.pl> wrote: > On Wednesday, July 28, 2010, Ondrej Zary wrote: > > Hello, > > after very long bisection, I finally found what's causing memory corruption > > during hibernation on my machine sice 2.6.31: > > https://bugzilla.kernel.org/show_bug.cgi?id=15753 > > > > It's commit c9e444103b5e7a5a3519f9913f59767f92e33baf (mm: reuse unused swap > > entry if necessary). > > > > I don't know anything about swapping in Linux so I don't have a clue what's > > wrong with that commit. > > Thanks for bisecting! > > This looks rather serious. I'd be grateful from any clues from the mm guys > involved (CCed). > Considering possible cases...and here is a patch. but I'm not fully sure. Could you clarify ? But hmm...status of swap_map[] to be recovered at resume() seems to be just based on luck. or hibernation has some tricks on swap_map[] ? == At hibernation, all pages-should-be-saved are written into a image (here, swap). Then, swap_map[], memmap etcs are also saved into disks. But, swap allocation happens one by one. So, the final image of swap_map[] is different from saved one and the commit c9e444103b5e7a5a3519f9913f59767f92e33baf changes page's state while assiging swap. Because memory can be modified in hibernation is only not-to-be-save memory. it's a breakage. This patch fixes it by disabling swap entry reuse at hibernation. Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu(a)jp.fujitsu.com> --- mm/swapfile.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) Index: linux-2.6.34.org/mm/swapfile.c =================================================================== --- linux-2.6.34.org.orig/mm/swapfile.c +++ linux-2.6.34.org/mm/swapfile.c @@ -316,7 +316,9 @@ checks: scan_base = offset = si->lowest_bit; /* reuse swap entry of cache-only swap if not busy. */ - if (vm_swap_full() && si->swap_map[offset] == SWAP_HAS_CACHE) { + if (vm_swap_full() + && usage == SWAP_HAS_CACHE + && si->swap_map[offset] == SWAP_HAS_CACHE) { int swap_was_freed; spin_unlock(&swap_lock); swap_was_freed = __try_to_reclaim_swap(si, offset); -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: KOSAKI Motohiro on 29 Jul 2010 01:30 > Index: linux-2.6.34.org/mm/swapfile.c > =================================================================== > --- linux-2.6.34.org.orig/mm/swapfile.c > +++ linux-2.6.34.org/mm/swapfile.c > @@ -316,7 +316,9 @@ checks: > scan_base = offset = si->lowest_bit; > > /* reuse swap entry of cache-only swap if not busy. */ > - if (vm_swap_full() && si->swap_map[offset] == SWAP_HAS_CACHE) { > + if (vm_swap_full() > + && usage == SWAP_HAS_CACHE > + && si->swap_map[offset] == SWAP_HAS_CACHE) { > int swap_was_freed; > spin_unlock(&swap_lock); > swap_was_freed = __try_to_reclaim_swap(si, offset); Can you please add explicit commenting in the code? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: KAMEZAWA Hiroyuki on 29 Jul 2010 01:30 On Thu, 29 Jul 2010 14:23:33 +0900 (JST) KOSAKI Motohiro <kosaki.motohiro(a)jp.fujitsu.com> wrote: > Can you please add explicit commenting in the code? > How about this ? == From: KAMEZAWA Hiroyuki <kamezawa.hiroyu(a)jp.fujitsu.com> At hibernation, all pages-should-be-saved are written into a image (here, swap). Then, swap_map[], memmap etcs are also saved into disks. But, swap allocation happens one by one. So, the final image of swap_map[] is different from saved one and the commit c9e444103b5e7a5a3519f9913f59767f92e33baf changes page's state while assiging swap. Because memory can be modified in hibernation is only not-to-be-save memory. it's a breakage. This patch fixes it by disabling swap entry reuse at hibernation. Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu(a)jp.fujitsu.com> --- mm/swapfile.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) Index: linux-2.6.34.org/mm/swapfile.c =================================================================== --- linux-2.6.34.org.orig/mm/swapfile.c +++ linux-2.6.34.org/mm/swapfile.c @@ -315,8 +315,15 @@ checks: if (offset > si->highest_bit) scan_base = offset = si->lowest_bit; - /* reuse swap entry of cache-only swap if not busy. */ - if (vm_swap_full() && si->swap_map[offset] == SWAP_HAS_CACHE) { + /* + * reuse swap entry of cache-only swap if not busy && + * when we're called via pageout(). At hibernation, swap-reuse + * is harmful because it changes memory status...which may + * be saved already. + */ + if (vm_swap_full() + && usage == SWAP_HAS_CACHE + && si->swap_map[offset] == SWAP_HAS_CACHE) { int swap_was_freed; spin_unlock(&swap_lock); swap_was_freed = __try_to_reclaim_swap(si, offset); -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: KOSAKI Motohiro on 29 Jul 2010 01:40 > On Thu, 29 Jul 2010 14:23:33 +0900 (JST) > KOSAKI Motohiro <kosaki.motohiro(a)jp.fujitsu.com> wrote: > > > Can you please add explicit commenting in the code? > > > How about this ? Looks good to me. thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Ondrej Zary on 29 Jul 2010 13:40
On Thursday 29 July 2010 07:24:29 KAMEZAWA Hiroyuki wrote: > On Thu, 29 Jul 2010 14:23:33 +0900 (JST) > > KOSAKI Motohiro <kosaki.motohiro(a)jp.fujitsu.com> wrote: > > Can you please add explicit commenting in the code? > > How about this ? > == > From: KAMEZAWA Hiroyuki <kamezawa.hiroyu(a)jp.fujitsu.com> > > At hibernation, all pages-should-be-saved are written into a image (here, > swap). Then, swap_map[], memmap etcs are also saved into disks. > > But, swap allocation happens one by one. So, the final image of swap_map[] > is different from saved one and the commit > c9e444103b5e7a5a3519f9913f59767f92e33baf changes page's state while > assiging swap. Because memory can be modified in hibernation is only > not-to-be-save memory. it's a breakage. > > This patch fixes it by disabling swap entry reuse at hibernation. Thanks for the patch, I'm going to test it for a few days. It didn't compile with 2.6.32 (because of missing "usage" parameter) so I tried 2.6.35-rc6 only to find that there's regression since 2.6.34 that causes console font corruption with matroxfb. Almost every new kernel means new regressions on this machine. Looks like something is wrong with the development process... So at least the patch compiles with 2.6.33 where the console works - so that's going to be tested. > Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu(a)jp.fujitsu.com> > --- > mm/swapfile.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > Index: linux-2.6.34.org/mm/swapfile.c > =================================================================== > --- linux-2.6.34.org.orig/mm/swapfile.c > +++ linux-2.6.34.org/mm/swapfile.c > @@ -315,8 +315,15 @@ checks: > if (offset > si->highest_bit) > scan_base = offset = si->lowest_bit; > > - /* reuse swap entry of cache-only swap if not busy. */ > - if (vm_swap_full() && si->swap_map[offset] == SWAP_HAS_CACHE) { > + /* > + * reuse swap entry of cache-only swap if not busy && > + * when we're called via pageout(). At hibernation, swap-reuse > + * is harmful because it changes memory status...which may > + * be saved already. > + */ > + if (vm_swap_full() > + && usage == SWAP_HAS_CACHE > + && si->swap_map[offset] == SWAP_HAS_CACHE) { > int swap_was_freed; > spin_unlock(&swap_lock); > swap_was_freed = __try_to_reclaim_swap(si, offset); > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo(a)vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- Ondrej Zary -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |