| 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
| ||
|
Message-ID: <20100331112730.GB27389@csn.ul.ie>
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