lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Wed, 31 Mar 2010 12:27:30 +0100
From:	Mel Gorman <mel@....ul.ie>
To:	KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Andrea Arcangeli <aarcange@...hat.com>,
	Christoph Lameter <cl@...ux-foundation.org>,
	Adam Litke <agl@...ibm.com>, Avi Kivity <avi@...hat.com>,
	David Rientjes <rientjes@...gle.com>,
	Minchan Kim <minchan.kim@...il.com>,
	KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>,
	Rik van Riel <riel@...hat.com>, linux-kernel@...r.kernel.org,
	linux-mm@...ck.org
Subject: Re: [PATCH 14/14] mm,migration: Allow the migration of
	PageSwapCache pages

On Wed, Mar 31, 2010 at 02:26:23PM +0900, KAMEZAWA Hiroyuki wrote:
> On Tue, 30 Mar 2010 10:14:49 +0100
> Mel Gorman <mel@....ul.ie> wrote:
> 
> > PageAnon pages that are unmapped may or may not have an anon_vma so
> > are not currently migrated. However, a swap cache page can be migrated
> > and fits this description. This patch identifies page swap caches and
> > allows them to be migrated.
> > 
> 
> Some comments.
> 
> > Signed-off-by: Mel Gorman <mel@....ul.ie>
> > ---
> >  mm/migrate.c |   15 ++++++++++-----
> >  mm/rmap.c    |    6 ++++--
> >  2 files changed, 14 insertions(+), 7 deletions(-)
> > 
> > diff --git a/mm/migrate.c b/mm/migrate.c
> > index 35aad2a..f9bf37e 100644
> > --- a/mm/migrate.c
> > +++ b/mm/migrate.c
> > @@ -203,6 +203,9 @@ static int migrate_page_move_mapping(struct address_space *mapping,
> >  	void **pslot;
> >  
> >  	if (!mapping) {
> > +		if (PageSwapCache(page))
> > +			SetPageSwapCache(newpage);
> > +
> 
> Migration of SwapCache requires radix-tree replacement, IOW, 
>  mapping == NULL && PageSwapCache is BUG.
> 
> So, this never happens.
> 

Correct. In the initial attempt to allow PageSwapCache pages to move, I
encountered a number of bugs. This was a "it can't be happening but something
is odd" change that I meant to drop in the final version but forgot. Thanks

> 
> >  		/* Anonymous page without mapping */
> >  		if (page_count(page) != 1)
> >  			return -EAGAIN;
> > @@ -607,11 +610,13 @@ static int unmap_and_move(new_page_t get_new_page, unsigned long private,
> >  		 * the page was isolated and when we reached here while
> >  		 * the RCU lock was not held
> >  		 */
> > -		if (!page_mapped(page))
> > -			goto rcu_unlock;
> > -
> > -		anon_vma = page_anon_vma(page);
> > -		atomic_inc(&anon_vma->external_refcount);
> > +		if (!page_mapped(page)) {
> > +			if (!PageSwapCache(page))
> > +				goto rcu_unlock;
> > +		} else {
> > +			anon_vma = page_anon_vma(page);
> > +			atomic_inc(&anon_vma->external_refcount);
> > +		}
> >  	}
> >  
> >  	/*
> > diff --git a/mm/rmap.c b/mm/rmap.c
> > index af35b75..d5ea1f2 100644
> > --- a/mm/rmap.c
> > +++ b/mm/rmap.c
> > @@ -1394,9 +1394,11 @@ int rmap_walk(struct page *page, int (*rmap_one)(struct page *,
> >  
> >  	if (unlikely(PageKsm(page)))
> >  		return rmap_walk_ksm(page, rmap_one, arg);
> > -	else if (PageAnon(page))
> > +	else if (PageAnon(page)) {
> > +		if (PageSwapCache(page))
> > +			return SWAP_AGAIN;
> >  		return rmap_walk_anon(page, rmap_one, arg);
> 
> SwapCache has a condition as (PageSwapCache(page) && page_mapped(page) == true.
> 
> Please see do_swap_page(), PageSwapCache bit is cleared only when
> 
> do_swap_page()...
>        swap_free(entry);
>         if (vm_swap_full() || (vma->vm_flags & VM_LOCKED) || PageMlocked(page))
>                 try_to_free_swap(page);
> 
> Then, PageSwapCache is cleared only when swap is freeable even if mapped.
> 
> rmap_walk_anon() should be called and the check is not necessary.
> 

I follow your reasoning. What caught me is the following comment;

         * Corner case handling:
         * 1. When a new swap-cache page is read into, it is added to the LRU
         * and treated as swapcache but it has no rmap yet.
         * Calling try_to_unmap() against a page->mapping==NULL page will
         * trigger a BUG.  So handle it here.

and the fact that without the check the following bug is triggered;

[  476.541321] BUG: unable to handle kernel NULL pointer dereference at (null)
[  476.590328] IP: [<ffffffff81072162>] __bfs+0x1a1/0x1d7
[  476.590328] PGD 3781c067 PUD 371b2067 PMD 0 
[  476.590328] Oops: 0000 [#1] PREEMPT SMP 
[  476.590328] last sysfs file: /sys/block/sr0/capability
[  476.590328] CPU 0 
[  476.590328] Modules linked in: highalloc trace_allocmap buddyinfo vmregress_core oprofile dm_crypt loop i2c_piix4 evdev i2c_core processor serio_raw tpm_tis tpm tpm_bios shpchp pci_hotplug button ext3 jbd mbcache dm_mirror dm_region_hash dm_log dm_snapshot dm_mod sg sr_mod cdrom sd_mod ata_generic ahci libahci ide_pci_generic libata ide_core scsi_mod ohci_hcd r8169 mii ehci_hcd floppy thermal fan thermal_sys
[  477.296405] 
[  477.296405] Pid: 4343, comm: bench-stresshig Not tainted 2.6.34-rc2-mm1-compaction-v7r3 #1 GA-MA790GP-UD4H/GA-MA790GP-UD4H
[  477.296405] RIP: 0010:[<ffffffff81072162>]  [<ffffffff81072162>] __bfs+0x1a1/0x1d7
[  477.296405] RSP: 0000:ffff88007817d4c8  EFLAGS: 00010046
[  477.296405] RAX: ffffffff81c80170 RBX: ffff88007817d528 RCX: 0000000000000000
[  477.296405] RDX: ffff88007817d528 RSI: 0000000000000000 RDI: ffff88007817d528
[  477.296405] RBP: ffff88007817d518 R08: 0000000000000000 R09: 0000000000000000
[  477.296405] R10: ffffffff816a1d08 R11: 0000000000000046 R12: 0000000000000000
[  477.922839] R13: ffffffff81513887 R14: ffffffff81c80170 R15: 0000000000000000
[  477.922839] FS:  00007f8d853d26e0(0000) GS:ffff880002200000(0000) knlGS:0000000000000000
[  478.123091] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[  478.123091] CR2: 0000000000000000 CR3: 0000000037a0e000 CR4: 00000000000006f0
[  478.123091] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[  478.123091] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
[  478.123091] Process bench-stresshig (pid: 4343, threadinfo ffff88007817c000, task ffff88007ebea000)
[  478.566030] Stack:
[  478.566030]  ffff88007817d570 ffffffff81070f92 0000000000000000 0000000000000002
[  478.566030] <0> ffff88007817d528 ffff88007817d528 ffff88007ebea668 ffffffff81513887
[  478.566030] <0> ffff88007ebea000 ffff88007ebea000 ffff88007817d598 ffffffff81074201
[  478.566030] Call Trace:
[  478.566030]  [<ffffffff81070f92>] ? usage_match+0x0/0x17
[  478.566030]  [<ffffffff81074201>] check_usage_backwards+0x93/0xca
[  478.566030]  [<ffffffff8107416e>] ? check_usage_backwards+0x0/0xca
[  478.566030]  [<ffffffff81073544>] mark_lock+0x31d/0x52f
[  478.566030]  [<ffffffff8107515a>] __lock_acquire+0x801/0x1776
[  478.566030]  [<ffffffff810761c5>] lock_acquire+0xf6/0x122
[  478.566030]  [<ffffffff810ef121>] ? rmap_walk+0x5c/0x16d
[  478.566030]  [<ffffffff812fcfeb>] _raw_spin_lock+0x3b/0x47
[  478.566030]  [<ffffffff810ef121>] ? rmap_walk+0x5c/0x16d
[  478.566030]  [<ffffffff810ef121>] rmap_walk+0x5c/0x16d
[  478.566030]  [<ffffffff81106396>] ? remove_migration_pte+0x0/0x234
[  478.566030]  [<ffffffff81300dc1>] ? sub_preempt_count+0x9/0x83
[  478.566030]  [<ffffffff81106914>] ? migrate_page_copy+0xa0/0x1ed
[  478.566030]  [<ffffffff81106ea4>] migrate_pages+0x3fc/0x5d3
[  478.566030]  [<ffffffff81106c56>] ? migrate_pages+0x1ae/0x5d3
[  478.566030]  [<ffffffff81073a24>] ? trace_hardirqs_on_caller+0x110/0x134
[  478.566030]  [<ffffffff81107e11>] ? compaction_alloc+0x0/0x283
[  478.566030]  [<ffffffff811079b0>] ? compact_zone+0x14e/0x4bd

Granted, what I should have spotted was that a more relevant check was for
the specific corner case like in the revised patch below. Please note that
if I put a WARN_ON in the check in rmap.c, it can and does trigger. If this
situation really is never meant to occur, there is a race that needs to be
closed before PageSwapCache can be migrated.

==== CUT HERE ====

mm,migration: Allow the migration of PageSwapCache pages

PageAnon pages that are unmapped may or may not have an anon_vma so
are not currently migrated. However, a swap cache page can be migrated
and fits this description. This patch identifies page swap caches and
allows them to be migrated.

Signed-off-by: Mel Gorman <mel@....ul.ie>
---
 mm/migrate.c |   12 +++++++-----
 mm/rmap.c    |    7 +++++--
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/mm/migrate.c b/mm/migrate.c
index 35aad2a..2284d79 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -607,11 +607,13 @@ static int unmap_and_move(new_page_t get_new_page, unsigned long private,
 		 * the page was isolated and when we reached here while
 		 * the RCU lock was not held
 		 */
-		if (!page_mapped(page))
-			goto rcu_unlock;
-
-		anon_vma = page_anon_vma(page);
-		atomic_inc(&anon_vma->external_refcount);
+		if (!page_mapped(page)) {
+			if (!PageSwapCache(page))
+				goto rcu_unlock;
+		} else {
+			anon_vma = page_anon_vma(page);
+			atomic_inc(&anon_vma->external_refcount);
+		}
 	}
 
 	/*
diff --git a/mm/rmap.c b/mm/rmap.c
index af35b75..7d63c68 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -1394,9 +1394,12 @@ int rmap_walk(struct page *page, int (*rmap_one)(struct page *,
 
 	if (unlikely(PageKsm(page)))
 		return rmap_walk_ksm(page, rmap_one, arg);
-	else if (PageAnon(page))
+	else if (PageAnon(page)) {
+		/* See comment on corner case handling in mm/migrate.c */
+		if (PageSwapCache(page) && !page_mapped(page))
+			return SWAP_AGAIN;
 		return rmap_walk_anon(page, rmap_one, arg);
-	else
+	} else
 		return rmap_walk_file(page, rmap_one, arg);
 }
 #endif /* CONFIG_MIGRATION */
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists