On Tue, 18 Apr 2006 01:27:40 -0700 (PDT)
Christoph Lameter <[email protected]> wrote:
> On Tue, 18 Apr 2006, KAMEZAWA Hiroyuki wrote:
>
> > On Mon, 17 Apr 2006 23:58:41 -0700 (PDT)
> > Christoph Lameter <[email protected]> wrote:
> >
> > > Hmmm... Good ideas. I think it could be much simpler like the following
> > > patch.
> > >
> > > However, the problem here is how to know that we really took the anon_vma
> > > lock and what to do about a page being unmmapped while migrating. This
> > > could cause the anon_vma not to be unlocked.
> > >
> > lock dependency here is page_lock(page) -> page's anon_vma->lock.
> > So, I guess anon_vma->lock cannot be unlocked by other threads
> > if we have page_lock(page).
>
> No the problem is to know if the lock was really taken. SWAP_AGAIN could
> mean that page_lock_anon_vma failed.
>
Ah, I see. and understood what you did in http://lkml.org/lkml/2006/4/18/19
That will be happen when the migration takes the anon_vma->lock in
try_to_unmap().
> Also the page may be freed while it is being processes. In that case
> remove_migration_ptes may not find the mapping and may not unlock the
> anon_vma.
>
My patch in http://lkml.org/lkml/2006/4/17/180
This is a look when above patch is applied.
==
/*
* Common logic to directly migrate a single page suitable for
* pages that do not use PagePrivate.
*
* Pages are locked upon entry and exit.
*/
int migrate_page(struct page *newpage, struct page *page)
{
int rc;
struct anon_vma *anon_vma;
BUG_ON(PageWriteback(page)); /* Writeback must be complete */
if (PageAnon(page)) {
anon_vma = page_lock_anon_vma(page);
}
rc = migrate_page_remove_references(newpage, page,
page_mapping(page) ? 2 : 1);
if (rc) {
remove_migration_ptes(anon_vma, page, page);
goto unlock_out;
}
migrate_page_copy(newpage, page);
remove_migration_ptes(anon_vma, page, newpage);
unlock_out:
if (anon_vma)
spin_unlock(&anon_vma->lock);
return rc;
}
==
lock around anon_vma->lock does not depend on the result of
try_to_unmap() and remove_migration_ptes().
But I agree : 'taking anon_vma->lock before try_to_unmap() is ugly and complicated
and will make things insane.'
Will this attached one make things clearer ?
This anon_vma->lock is just an optimization (for now) but complicated.
I think restart discusstion against -mm3? will be better.
-Kame
==
Index: Christoph-NewMigrationV2/mm/rmap.c
===================================================================
--- Christoph-NewMigrationV2.orig/mm/rmap.c
+++ Christoph-NewMigrationV2/mm/rmap.c
@@ -711,29 +711,44 @@ static void try_to_unmap_cluster(unsigne
pte_unmap_unlock(pte - 1, ptl);
}
-static int try_to_unmap_anon(struct page *page, int migration)
+static int __try_to_unmap_anon(struct anon_vma *anon_vma,
+ struct page *page, int migration)
{
- struct anon_vma *anon_vma;
struct vm_area_struct *vma;
int ret = SWAP_AGAIN;
- if (migration) { /* anon_vma->lock is held under migration */
- unsigned long mapping;
- mapping = (unsigned long)page->mapping - PAGE_MAPPING_ANON;
- anon_vma = (struct anon_vma *)mapping;
- } else {
- anon_vma = page_lock_anon_vma(page);
- }
- if (!anon_vma)
- return ret;
-
list_for_each_entry(vma, &anon_vma->head, anon_vma_node) {
ret = try_to_unmap_one(page, vma, migration);
if (ret == SWAP_FAIL || !page_mapped(page))
break;
}
- if (!migration)
- spin_unlock(&anon_vma->lock);
+ return ret;
+}
+
+static int try_to_unmap_anon(struct page *page)
+{
+ struct anon_vma *anon_vma;
+ struct vm_area_struct *vma;
+ int ret = SWAP_AGAIN;
+
+ anon_vma = page_lock_anon_vma(page);
+ if (!anon_vma)
+ return ret;
+ ret = __try_to_unmap_anon(anon_vma, page, 0);
+ spin_unlock(&anon_vma->lock);
+ return ret;
+}
+
+static int try_to_unmap_anon_migrate(struct page *page)
+{
+ struct anon_vma *anon_vma;
+ unsigned long mapping;
+ int ret = SWAP_AGAIN;
+ if (PageAnon(page))
+ return ret;
+ mapping = page->mapping;
+ anon_vma = (struct anon_vma *)(mapping - PAGE_MAPPING_ANON);
+ ret = __try_to_unmap_anon_migrate(anon_vma, page, 1);
return ret;
}
@@ -851,9 +866,12 @@ int try_to_unmap(struct page *page, int
BUG_ON(!PageLocked(page));
- if (PageAnon(page))
- ret = try_to_unmap_anon(page, migration);
- else
+ if (PageAnon(page)) {
+ if (migration)
+ ret = try_to_unmap_anon_migrate(page);
+ else
+ ret = try_to_unmap_anon(page);
+ } else
ret = try_to_unmap_file(page, migration);
if (!page_mapped(page))
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
