[PATCH 0/4] hugetlb, rmap: fixes and cleanups

[PATCH 0/4] hugetlb, rmap: fixes and cleanups

[Naoya Horiguchi]

Hi,

These are fix and cleanup patches for hugepage rmapping.
All these were pointed out in the following thread (last 4 messages.)

  http://thread.gmane.org/gmane.linux.kernel.mm/52334

Summary:

 [PATCH 1/4] hugetlb, rmap: always use anon_vma root pointer
 [PATCH 2/4] hugetlb, rmap: use hugepage_add_new_anon_rmap() in hugetlb_cow()
 [PATCH 3/4] hugetlb, rmap: fix confusing page locking in hugetlb_cow()
 [PATCH 4/4] hugetlb, rmap: add BUG_ON(!PageLocked) in hugetlb_add_anon_rmap()

Thanks,
Naoya Horiguchi

[PATCH 0/4] hugetlb, rmap: fixes and cleanups

[Naoya Horiguchi]

Hi,

These are fix and cleanup patches for hugepage rmapping.
All these were pointed out in the following thread (last 4 messages.)

  http://thread.gmane.org/gmane.linux.kernel.mm/52334

Summary:

 [PATCH 1/4] hugetlb, rmap: always use anon_vma root pointer
 [PATCH 2/4] hugetlb, rmap: use hugepage_add_new_anon_rmap() in hugetlb_cow()
 [PATCH 3/4] hugetlb, rmap: fix confusing page locking in hugetlb_cow()
 [PATCH 4/4] hugetlb, rmap: add BUG_ON(!PageLocked) in hugetlb_add_anon_rmap()

Thanks,
Naoya Horiguchi

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 1/4] hugetlb, rmap: always use anon_vma root pointer

[Naoya Horiguchi]

This patch applies Andrea's fix given by the following patch into hugepage
rmapping code:

  commit 288468c334e98aacbb7e2fb8bde6bc1adcd55e05
  Author: Andrea Arcangeli <aarcange@redhat.com>
  Date:   Mon Aug 9 17:19:09 2010 -0700

This patch uses anon_vma->root and avoids unnecessary overwriting when
anon_vma is already set up.

Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
---
 mm/rmap.c |   13 +++++++------
 1 files changed, 7 insertions(+), 6 deletions(-)

diff --git v2.6.36-rc3/mm/rmap.c v2.6.36-rc3/mm/rmap.c
index f6f0d2d..2854857 100644
--- v2.6.36-rc3/mm/rmap.c
+++ v2.6.36-rc3/mm/rmap.c
@@ -1564,13 +1564,14 @@ static void __hugepage_set_anon_rmap(struct page *page,
 	struct vm_area_struct *vma, unsigned long address, int exclusive)
 {
 	struct anon_vma *anon_vma = vma->anon_vma;
+
 	BUG_ON(!anon_vma);
-	if (!exclusive) {
-		struct anon_vma_chain *avc;
-		avc = list_entry(vma->anon_vma_chain.prev,
-				 struct anon_vma_chain, same_vma);
-		anon_vma = avc->anon_vma;
-	}
+
+	if (PageAnon(page))
+		return;
+	if (!exclusive)
+		anon_vma = anon_vma->root;
+
 	anon_vma = (void *) anon_vma + PAGE_MAPPING_ANON;
 	page->mapping = (struct address_space *) anon_vma;
 	page->index = linear_page_index(vma, address);
-- 
1.7.2.2

[PATCH 1/4] hugetlb, rmap: always use anon_vma root pointer

[Naoya Horiguchi]

This patch applies Andrea's fix given by the following patch into hugepage
rmapping code:

  commit 288468c334e98aacbb7e2fb8bde6bc1adcd55e05
  Author: Andrea Arcangeli <aarcange@redhat.com>
  Date:   Mon Aug 9 17:19:09 2010 -0700

This patch uses anon_vma->root and avoids unnecessary overwriting when
anon_vma is already set up.

Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
---
 mm/rmap.c |   13 +++++++------
 1 files changed, 7 insertions(+), 6 deletions(-)

diff --git v2.6.36-rc3/mm/rmap.c v2.6.36-rc3/mm/rmap.c
index f6f0d2d..2854857 100644
--- v2.6.36-rc3/mm/rmap.c
+++ v2.6.36-rc3/mm/rmap.c
@@ -1564,13 +1564,14 @@ static void __hugepage_set_anon_rmap(struct page *page,
 	struct vm_area_struct *vma, unsigned long address, int exclusive)
 {
 	struct anon_vma *anon_vma = vma->anon_vma;
+
 	BUG_ON(!anon_vma);
-	if (!exclusive) {
-		struct anon_vma_chain *avc;
-		avc = list_entry(vma->anon_vma_chain.prev,
-				 struct anon_vma_chain, same_vma);
-		anon_vma = avc->anon_vma;
-	}
+
+	if (PageAnon(page))
+		return;
+	if (!exclusive)
+		anon_vma = anon_vma->root;
+
 	anon_vma = (void *) anon_vma + PAGE_MAPPING_ANON;
 	page->mapping = (struct address_space *) anon_vma;
 	page->index = linear_page_index(vma, address);
-- 
1.7.2.2

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 2/4] hugetlb, rmap: use hugepage_add_new_anon_rmap() in hugetlb_cow()

[Naoya Horiguchi]

Obviously, setting anon_vma for COWed hugepage should be done
by hugepage_add_new_anon_rmap() to scan vmas faster.
This patch fixes it.

Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
---
 mm/hugetlb.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git v2.6.36-rc3/mm/hugetlb.c v2.6.36-rc3/mm/hugetlb.c
index cc5be78..9519f3f 100644
--- v2.6.36-rc3/mm/hugetlb.c
+++ v2.6.36-rc3/mm/hugetlb.c
@@ -2404,7 +2404,7 @@ retry_avoidcopy:
 		set_huge_pte_at(mm, address, ptep,
 				make_huge_pte(vma, new_page, 1));
 		page_remove_rmap(old_page);
-		hugepage_add_anon_rmap(new_page, vma, address);
+		hugepage_add_new_anon_rmap(new_page, vma, address);
 		/* Make the old page be freed below */
 		new_page = old_page;
 		mmu_notifier_invalidate_range_end(mm,
-- 
1.7.2.2

[PATCH 2/4] hugetlb, rmap: use hugepage_add_new_anon_rmap() in hugetlb_cow()

[Naoya Horiguchi]

Obviously, setting anon_vma for COWed hugepage should be done
by hugepage_add_new_anon_rmap() to scan vmas faster.
This patch fixes it.

Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
---
 mm/hugetlb.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git v2.6.36-rc3/mm/hugetlb.c v2.6.36-rc3/mm/hugetlb.c
index cc5be78..9519f3f 100644
--- v2.6.36-rc3/mm/hugetlb.c
+++ v2.6.36-rc3/mm/hugetlb.c
@@ -2404,7 +2404,7 @@ retry_avoidcopy:
 		set_huge_pte_at(mm, address, ptep,
 				make_huge_pte(vma, new_page, 1));
 		page_remove_rmap(old_page);
-		hugepage_add_anon_rmap(new_page, vma, address);
+		hugepage_add_new_anon_rmap(new_page, vma, address);
 		/* Make the old page be freed below */
 		new_page = old_page;
 		mmu_notifier_invalidate_range_end(mm,
-- 
1.7.2.2

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 3/4] hugetlb, rmap: fix confusing page locking in hugetlb_cow()

[Naoya Horiguchi]

if(!trylock_page) block in avoidcopy path of hugetlb_cow() looks confusing
and is buggy.  Originally this trylock_page() is intended to make sure
that old_page is locked even when old_page != pagecache_page, because then
only pagecache_page is locked.
This patch fixes it by moving page locking into hugetlb_fault().

Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
---
 mm/hugetlb.c |   20 ++++++++++----------
 1 files changed, 10 insertions(+), 10 deletions(-)

diff --git v2.6.36-rc3/mm/hugetlb.c v2.6.36-rc3/mm/hugetlb.c
index 9519f3f..2e17e0e 100644
--- v2.6.36-rc3/mm/hugetlb.c
+++ v2.6.36-rc3/mm/hugetlb.c
@@ -2324,11 +2324,8 @@ retry_avoidcopy:
 	 * and just make the page writable */
 	avoidcopy = (page_mapcount(old_page) == 1);
 	if (avoidcopy) {
-		if (!trylock_page(old_page)) {
-			if (PageAnon(old_page))
-				page_move_anon_rmap(old_page, vma, address);
-		} else
-			unlock_page(old_page);
+		if (PageAnon(old_page))
+			page_move_anon_rmap(old_page, vma, address);
 		set_huge_ptep_writable(vma, address, ptep);
 		return 0;
 	}
@@ -2631,10 +2628,14 @@ int hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma,
 								vma, address);
 	}
 
-	if (!pagecache_page) {
-		page = pte_page(entry);
+	/*
+	 * hugetlb_cow() requires page locks of pte_page(entry) and
+	 * pagecache_page, so here we need take the former one
+	 * when page != pagecache_page or !pagecache_page.
+	 */
+	page = pte_page(entry);
+	if (page != pagecache_page)
 		lock_page(page);
-	}
 
 	spin_lock(&mm->page_table_lock);
 	/* Check for a racing update before calling hugetlb_cow */
@@ -2661,9 +2662,8 @@ out_page_table_lock:
 	if (pagecache_page) {
 		unlock_page(pagecache_page);
 		put_page(pagecache_page);
-	} else {
-		unlock_page(page);
 	}
+	unlock_page(page);
 
 out_mutex:
 	mutex_unlock(&hugetlb_instantiation_mutex);
-- 
1.7.2.2

[PATCH 3/4] hugetlb, rmap: fix confusing page locking in hugetlb_cow()

[Naoya Horiguchi]

if(!trylock_page) block in avoidcopy path of hugetlb_cow() looks confusing
and is buggy.  Originally this trylock_page() is intended to make sure
that old_page is locked even when old_page != pagecache_page, because then
only pagecache_page is locked.
This patch fixes it by moving page locking into hugetlb_fault().

Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
---
 mm/hugetlb.c |   20 ++++++++++----------
 1 files changed, 10 insertions(+), 10 deletions(-)

diff --git v2.6.36-rc3/mm/hugetlb.c v2.6.36-rc3/mm/hugetlb.c
index 9519f3f..2e17e0e 100644
--- v2.6.36-rc3/mm/hugetlb.c
+++ v2.6.36-rc3/mm/hugetlb.c
@@ -2324,11 +2324,8 @@ retry_avoidcopy:
 	 * and just make the page writable */
 	avoidcopy = (page_mapcount(old_page) == 1);
 	if (avoidcopy) {
-		if (!trylock_page(old_page)) {
-			if (PageAnon(old_page))
-				page_move_anon_rmap(old_page, vma, address);
-		} else
-			unlock_page(old_page);
+		if (PageAnon(old_page))
+			page_move_anon_rmap(old_page, vma, address);
 		set_huge_ptep_writable(vma, address, ptep);
 		return 0;
 	}
@@ -2631,10 +2628,14 @@ int hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma,
 								vma, address);
 	}
 
-	if (!pagecache_page) {
-		page = pte_page(entry);
+	/*
+	 * hugetlb_cow() requires page locks of pte_page(entry) and
+	 * pagecache_page, so here we need take the former one
+	 * when page != pagecache_page or !pagecache_page.
+	 */
+	page = pte_page(entry);
+	if (page != pagecache_page)
 		lock_page(page);
-	}
 
 	spin_lock(&mm->page_table_lock);
 	/* Check for a racing update before calling hugetlb_cow */
@@ -2661,9 +2662,8 @@ out_page_table_lock:
 	if (pagecache_page) {
 		unlock_page(pagecache_page);
 		put_page(pagecache_page);
-	} else {
-		unlock_page(page);
 	}
+	unlock_page(page);
 
 out_mutex:
 	mutex_unlock(&hugetlb_instantiation_mutex);
-- 
1.7.2.2

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 4/4] hugetlb, rmap: add BUG_ON(!PageLocked) in hugetlb_add_anon_rmap()

[Naoya Horiguchi]

Confirming page lock is held in hugetlb_add_anon_rmap() may be useful
to detect possible future problems.

Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
---
 mm/rmap.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git v2.6.36-rc3/mm/rmap.c v2.6.36-rc3/mm/rmap.c
index 2854857..9d2ba01 100644
--- v2.6.36-rc3/mm/rmap.c
+++ v2.6.36-rc3/mm/rmap.c
@@ -1582,6 +1582,8 @@ void hugepage_add_anon_rmap(struct page *page,
 {
 	struct anon_vma *anon_vma = vma->anon_vma;
 	int first;
+
+	BUG_ON(!PageLocked(page));
 	BUG_ON(!anon_vma);
 	BUG_ON(address < vma->vm_start || address >= vma->vm_end);
 	first = atomic_inc_and_test(&page->_mapcount);
-- 
1.7.2.2

[PATCH 4/4] hugetlb, rmap: add BUG_ON(!PageLocked) in hugetlb_add_anon_rmap()

[Naoya Horiguchi]

Confirming page lock is held in hugetlb_add_anon_rmap() may be useful
to detect possible future problems.

Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
---
 mm/rmap.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git v2.6.36-rc3/mm/rmap.c v2.6.36-rc3/mm/rmap.c
index 2854857..9d2ba01 100644
--- v2.6.36-rc3/mm/rmap.c
+++ v2.6.36-rc3/mm/rmap.c
@@ -1582,6 +1582,8 @@ void hugepage_add_anon_rmap(struct page *page,
 {
 	struct anon_vma *anon_vma = vma->anon_vma;
 	int first;
+
+	BUG_ON(!PageLocked(page));
 	BUG_ON(!anon_vma);
 	BUG_ON(address < vma->vm_start || address >= vma->vm_end);
 	first = atomic_inc_and_test(&page->_mapcount);
-- 
1.7.2.2

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 0/4] hugetlb, rmap: fixes and cleanups

[Andi Kleen]

On Fri, 10 Sep 2010 13:23:02 +0900
Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> wrote:

> Hi,
> 
> These are fix and cleanup patches for hugepage rmapping.
> All these were pointed out in the following thread (last 4 messages.)
> 
>   http://thread.gmane.org/gmane.linux.kernel.mm/52334

Looks all good to me. It's not strictly hwpoison related
though, so I assume they are better with Andrew than my tree.

I assume they do not depend on the earlier patchkit?
 
-Andi

Re: [PATCH 0/4] hugetlb, rmap: fixes and cleanups

[Andi Kleen]

On Fri, 10 Sep 2010 13:23:02 +0900
Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> wrote:

> Hi,
> 
> These are fix and cleanup patches for hugepage rmapping.
> All these were pointed out in the following thread (last 4 messages.)
> 
>   http://thread.gmane.org/gmane.linux.kernel.mm/52334

Looks all good to me. It's not strictly hwpoison related
though, so I assume they are better with Andrew than my tree.

I assume they do not depend on the earlier patchkit?
 
-Andi

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 0/4] hugetlb, rmap: fixes and cleanups

[Naoya Horiguchi]

On Fri, Sep 10, 2010 at 11:04:38AM +0200, Andi Kleen wrote:
> On Fri, 10 Sep 2010 13:23:02 +0900
> Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> wrote:
> 
> > Hi,
> > 
> > These are fix and cleanup patches for hugepage rmapping.
> > All these were pointed out in the following thread (last 4 messages.)
> > 
> >   http://thread.gmane.org/gmane.linux.kernel.mm/52334
> 
> Looks all good to me. It's not strictly hwpoison related
> though, so I assume they are better with Andrew than my tree.

Agreed.

> I assume they do not depend on the earlier patchkit?

No, all changes on this patchset update code merged with
"HWPOISON for hugepage" patchset.

Thanks,
Naoya Horiguchi

Re: [PATCH 0/4] hugetlb, rmap: fixes and cleanups

[Naoya Horiguchi]

On Fri, Sep 10, 2010 at 11:04:38AM +0200, Andi Kleen wrote:
> On Fri, 10 Sep 2010 13:23:02 +0900
> Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> wrote:
> 
> > Hi,
> > 
> > These are fix and cleanup patches for hugepage rmapping.
> > All these were pointed out in the following thread (last 4 messages.)
> > 
> >   http://thread.gmane.org/gmane.linux.kernel.mm/52334
> 
> Looks all good to me. It's not strictly hwpoison related
> though, so I assume they are better with Andrew than my tree.

Agreed.

> I assume they do not depend on the earlier patchkit?

No, all changes on this patchset update code merged with
"HWPOISON for hugepage" patchset.

Thanks,
Naoya Horiguchi

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 0/4] hugetlb, rmap: fixes and cleanups

[Andi Kleen]

On Fri, 10 Sep 2010 20:47:59 +0900
Naoya Horiguchi <nao.horiguchi@gmail.com> wrote:

> On Fri, Sep 10, 2010 at 11:04:38AM +0200, Andi Kleen wrote:
> > On Fri, 10 Sep 2010 13:23:02 +0900
> > Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> wrote:
> > 
> > > Hi,
> > > 
> > > These are fix and cleanup patches for hugepage rmapping.
> > > All these were pointed out in the following thread (last 4
> > > messages.)
> > > 
> > >   http://thread.gmane.org/gmane.linux.kernel.mm/52334
> > 
> > Looks all good to me. It's not strictly hwpoison related
> > though, so I assume they are better with Andrew than my tree.
> 
> Agreed.
> 
> > I assume they do not depend on the earlier patchkit?
> 
> No, all changes on this patchset update code merged with
> "HWPOISON for hugepage" patchset.

Ok then it's better for me to carry it anyways, otherwise Andrew
will be in dependency hell.

I will need some acks from MM hackers though. Anyone?

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [PATCH 0/4] hugetlb, rmap: fixes and cleanups

[Andi Kleen]

On Fri, 10 Sep 2010 20:47:59 +0900
Naoya Horiguchi <nao.horiguchi@gmail.com> wrote:

> On Fri, Sep 10, 2010 at 11:04:38AM +0200, Andi Kleen wrote:
> > On Fri, 10 Sep 2010 13:23:02 +0900
> > Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> wrote:
> > 
> > > Hi,
> > > 
> > > These are fix and cleanup patches for hugepage rmapping.
> > > All these were pointed out in the following thread (last 4
> > > messages.)
> > > 
> > >   http://thread.gmane.org/gmane.linux.kernel.mm/52334
> > 
> > Looks all good to me. It's not strictly hwpoison related
> > though, so I assume they are better with Andrew than my tree.
> 
> Agreed.
> 
> > I assume they do not depend on the earlier patchkit?
> 
> No, all changes on this patchset update code merged with
> "HWPOISON for hugepage" patchset.

Ok then it's better for me to carry it anyways, otherwise Andrew
will be in dependency hell.

I will need some acks from MM hackers though. Anyone?

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/4] hugetlb, rmap: always use anon_vma root pointer

[Andrea Arcangeli]

On Fri, Sep 10, 2010 at 01:23:03PM +0900, Naoya Horiguchi wrote:
> This patch applies Andrea's fix given by the following patch into hugepage
> rmapping code:
> 
>   commit 288468c334e98aacbb7e2fb8bde6bc1adcd55e05
>   Author: Andrea Arcangeli <aarcange@redhat.com>
>   Date:   Mon Aug 9 17:19:09 2010 -0700
> 
> This patch uses anon_vma->root and avoids unnecessary overwriting when
> anon_vma is already set up.
> 
> Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> ---
>  mm/rmap.c |   13 +++++++------
>  1 files changed, 7 insertions(+), 6 deletions(-)

Acked-by: Andrea Arcangeli <aarcange@redhat.com>

Re: [PATCH 1/4] hugetlb, rmap: always use anon_vma root pointer

[Andrea Arcangeli]

On Fri, Sep 10, 2010 at 01:23:03PM +0900, Naoya Horiguchi wrote:
> This patch applies Andrea's fix given by the following patch into hugepage
> rmapping code:
> 
>   commit 288468c334e98aacbb7e2fb8bde6bc1adcd55e05
>   Author: Andrea Arcangeli <aarcange@redhat.com>
>   Date:   Mon Aug 9 17:19:09 2010 -0700
> 
> This patch uses anon_vma->root and avoids unnecessary overwriting when
> anon_vma is already set up.
> 
> Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> ---
>  mm/rmap.c |   13 +++++++------
>  1 files changed, 7 insertions(+), 6 deletions(-)

Acked-by: Andrea Arcangeli <aarcange@redhat.com>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/4] hugetlb, rmap: use hugepage_add_new_anon_rmap() in hugetlb_cow()

[Andrea Arcangeli]

On Fri, Sep 10, 2010 at 01:23:04PM +0900, Naoya Horiguchi wrote:
> Obviously, setting anon_vma for COWed hugepage should be done
> by hugepage_add_new_anon_rmap() to scan vmas faster.
> This patch fixes it.
> 
> Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> ---
>  mm/hugetlb.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)

Acked-by: Andrea Arcangeli <aarcange@redhat.com>

Re: [PATCH 2/4] hugetlb, rmap: use hugepage_add_new_anon_rmap() in hugetlb_cow()

[Andrea Arcangeli]

On Fri, Sep 10, 2010 at 01:23:04PM +0900, Naoya Horiguchi wrote:
> Obviously, setting anon_vma for COWed hugepage should be done
> by hugepage_add_new_anon_rmap() to scan vmas faster.
> This patch fixes it.
> 
> Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> ---
>  mm/hugetlb.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)

Acked-by: Andrea Arcangeli <aarcange@redhat.com>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/4] hugetlb, rmap: always use anon_vma root pointer

[Rik van Riel]

On 09/10/2010 12:23 AM, Naoya Horiguchi wrote:
> This patch applies Andrea's fix given by the following patch into hugepage
> rmapping code:
>
>    commit 288468c334e98aacbb7e2fb8bde6bc1adcd55e05
>    Author: Andrea Arcangeli<aarcange@redhat.com>
>    Date:   Mon Aug 9 17:19:09 2010 -0700
>
> This patch uses anon_vma->root and avoids unnecessary overwriting when
> anon_vma is already set up.
>
> Signed-off-by: Naoya Horiguchi<n-horiguchi@ah.jp.nec.com>

Reviewed-by: Rik van Riel <riel@redhat.com>

-- 
All rights reversed

Re: [PATCH 1/4] hugetlb, rmap: always use anon_vma root pointer

[Rik van Riel]

On 09/10/2010 12:23 AM, Naoya Horiguchi wrote:
> This patch applies Andrea's fix given by the following patch into hugepage
> rmapping code:
>
>    commit 288468c334e98aacbb7e2fb8bde6bc1adcd55e05
>    Author: Andrea Arcangeli<aarcange@redhat.com>
>    Date:   Mon Aug 9 17:19:09 2010 -0700
>
> This patch uses anon_vma->root and avoids unnecessary overwriting when
> anon_vma is already set up.
>
> Signed-off-by: Naoya Horiguchi<n-horiguchi@ah.jp.nec.com>

Reviewed-by: Rik van Riel <riel@redhat.com>

-- 
All rights reversed

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/4] hugetlb, rmap: use hugepage_add_new_anon_rmap() in hugetlb_cow()

[Rik van Riel]

On 09/10/2010 12:23 AM, Naoya Horiguchi wrote:
> Obviously, setting anon_vma for COWed hugepage should be done
> by hugepage_add_new_anon_rmap() to scan vmas faster.
> This patch fixes it.
>
> Signed-off-by: Naoya Horiguchi<n-horiguchi@ah.jp.nec.com>
> ---
>   mm/hugetlb.c |    2 +-
>   1 files changed, 1 insertions(+), 1 deletions(-)

Reviewed-by: Rik van Riel <riel@redhat.com>

-- 
All rights reversed

Re: [PATCH 2/4] hugetlb, rmap: use hugepage_add_new_anon_rmap() in hugetlb_cow()

[Rik van Riel]

On 09/10/2010 12:23 AM, Naoya Horiguchi wrote:
> Obviously, setting anon_vma for COWed hugepage should be done
> by hugepage_add_new_anon_rmap() to scan vmas faster.
> This patch fixes it.
>
> Signed-off-by: Naoya Horiguchi<n-horiguchi@ah.jp.nec.com>
> ---
>   mm/hugetlb.c |    2 +-
>   1 files changed, 1 insertions(+), 1 deletions(-)

Reviewed-by: Rik van Riel <riel@redhat.com>

-- 
All rights reversed

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 3/4] hugetlb, rmap: fix confusing page locking in hugetlb_cow()

[Rik van Riel]

On 09/10/2010 12:23 AM, Naoya Horiguchi wrote:
> if(!trylock_page) block in avoidcopy path of hugetlb_cow() looks confusing
> and is buggy.  Originally this trylock_page() is intended to make sure
> that old_page is locked even when old_page != pagecache_page, because then
> only pagecache_page is locked.
> This patch fixes it by moving page locking into hugetlb_fault().
>
> Signed-off-by: Naoya Horiguchi<n-horiguchi@ah.jp.nec.com>

Acked-by: Rik van Riel <riel@redhat.com>

-- 
All rights reversed

Re: [PATCH 3/4] hugetlb, rmap: fix confusing page locking in hugetlb_cow()

[Rik van Riel]

On 09/10/2010 12:23 AM, Naoya Horiguchi wrote:
> if(!trylock_page) block in avoidcopy path of hugetlb_cow() looks confusing
> and is buggy.  Originally this trylock_page() is intended to make sure
> that old_page is locked even when old_page != pagecache_page, because then
> only pagecache_page is locked.
> This patch fixes it by moving page locking into hugetlb_fault().
>
> Signed-off-by: Naoya Horiguchi<n-horiguchi@ah.jp.nec.com>

Acked-by: Rik van Riel <riel@redhat.com>

-- 
All rights reversed

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 4/4] hugetlb, rmap: add BUG_ON(!PageLocked) in hugetlb_add_anon_rmap()

[Rik van Riel]

On 09/10/2010 12:23 AM, Naoya Horiguchi wrote:
> Confirming page lock is held in hugetlb_add_anon_rmap() may be useful
> to detect possible future problems.
>
> Signed-off-by: Naoya Horiguchi<n-horiguchi@ah.jp.nec.com>

Acked-by: Rik van Riel <riel@redhat.com>

-- 
All rights reversed

Re: [PATCH 4/4] hugetlb, rmap: add BUG_ON(!PageLocked) in hugetlb_add_anon_rmap()

[Rik van Riel]

On 09/10/2010 12:23 AM, Naoya Horiguchi wrote:
> Confirming page lock is held in hugetlb_add_anon_rmap() may be useful
> to detect possible future problems.
>
> Signed-off-by: Naoya Horiguchi<n-horiguchi@ah.jp.nec.com>

Acked-by: Rik van Riel <riel@redhat.com>

-- 
All rights reversed

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 4/4] hugetlb, rmap: add BUG_ON(!PageLocked) in hugetlb_add_anon_rmap()

[Andrea Arcangeli]

On Fri, Sep 10, 2010 at 01:23:06PM +0900, Naoya Horiguchi wrote:
> Confirming page lock is held in hugetlb_add_anon_rmap() may be useful
> to detect possible future problems.
> 
> Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> ---
>  mm/rmap.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)

Acked-by: Andrea Arcangeli <aarcange@redhat.com>

It should probably be a VM_BUG_ON like do_page_add_anon_rmap, but
given the tiny hugetlbfs userbase it's ok.

Re: [PATCH 4/4] hugetlb, rmap: add BUG_ON(!PageLocked) in hugetlb_add_anon_rmap()

[Andrea Arcangeli]

On Fri, Sep 10, 2010 at 01:23:06PM +0900, Naoya Horiguchi wrote:
> Confirming page lock is held in hugetlb_add_anon_rmap() may be useful
> to detect possible future problems.
> 
> Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> ---
>  mm/rmap.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)

Acked-by: Andrea Arcangeli <aarcange@redhat.com>

It should probably be a VM_BUG_ON like do_page_add_anon_rmap, but
given the tiny hugetlbfs userbase it's ok.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 3/4] hugetlb, rmap: fix confusing page locking in hugetlb_cow()

[Linus Torvalds]

On Fri, 10 Sep 2010, Naoya Horiguchi wrote:
>  
> -	if (!pagecache_page) {
> -		page = pte_page(entry);
> +	/*
> +	 * hugetlb_cow() requires page locks of pte_page(entry) and
> +	 * pagecache_page, so here we need take the former one
> +	 * when page != pagecache_page or !pagecache_page.
> +	 */
> +	page = pte_page(entry);
> +	if (page != pagecache_page)
>  		lock_page(page);

Why isn't this a potential deadlock? You have two pages, and lock them 
both. Is there some ordering guarantee that says that 'pagecache_page' and 
'page' will always be in a certain relationship so that you cannot get 
A->B and B->A lock ordering?

Please document that ordering rule if so.

			Linus

Re: [PATCH 3/4] hugetlb, rmap: fix confusing page locking in hugetlb_cow()

[Linus Torvalds]

On Fri, 10 Sep 2010, Naoya Horiguchi wrote:
>  
> -	if (!pagecache_page) {
> -		page = pte_page(entry);
> +	/*
> +	 * hugetlb_cow() requires page locks of pte_page(entry) and
> +	 * pagecache_page, so here we need take the former one
> +	 * when page != pagecache_page or !pagecache_page.
> +	 */
> +	page = pte_page(entry);
> +	if (page != pagecache_page)
>  		lock_page(page);

Why isn't this a potential deadlock? You have two pages, and lock them 
both. Is there some ordering guarantee that says that 'pagecache_page' and 
'page' will always be in a certain relationship so that you cannot get 
A->B and B->A lock ordering?

Please document that ordering rule if so.

			Linus

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/4] hugetlb, rmap: always use anon_vma root pointer

[Linus Torvalds]

On Thu, Sep 9, 2010 at 9:23 PM, Naoya Horiguchi
<n-horiguchi@ah.jp.nec.com> wrote:
> This patch applies Andrea's fix given by the following patch into hugepage
> rmapping code:
>
>  commit 288468c334e98aacbb7e2fb8bde6bc1adcd55e05
>  Author: Andrea Arcangeli <aarcange@redhat.com>
>  Date:   Mon Aug 9 17:19:09 2010 -0700
>
> This patch uses anon_vma->root and avoids unnecessary overwriting when
> anon_vma is already set up.

Btw, why isn't the code in __page_set_anon_rmap() also doing this
cleaner version (ie a single "if (PageAnon(page)) return;" up front)?

The comments in that function are also some alien language translated
to english by some broken automatic translation service. Could
somebody clean up that function and come up with a comment that
actually parses as English and makes sense?

                                        Linus

Re: [PATCH 1/4] hugetlb, rmap: always use anon_vma root pointer

[Linus Torvalds]

On Thu, Sep 9, 2010 at 9:23 PM, Naoya Horiguchi
<n-horiguchi@ah.jp.nec.com> wrote:
> This patch applies Andrea's fix given by the following patch into hugepage
> rmapping code:
>
>  commit 288468c334e98aacbb7e2fb8bde6bc1adcd55e05
>  Author: Andrea Arcangeli <aarcange@redhat.com>
>  Date:   Mon Aug 9 17:19:09 2010 -0700
>
> This patch uses anon_vma->root and avoids unnecessary overwriting when
> anon_vma is already set up.

Btw, why isn't the code in __page_set_anon_rmap() also doing this
cleaner version (ie a single "if (PageAnon(page)) return;" up front)?

The comments in that function are also some alien language translated
to english by some broken automatic translation service. Could
somebody clean up that function and come up with a comment that
actually parses as English and makes sense?

                                        Linus

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/4] hugetlb, rmap: always use anon_vma root pointer

[Andi Kleen]

On Fri, 10 Sep 2010 10:19:24 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Thu, Sep 9, 2010 at 9:23 PM, Naoya Horiguchi
> <n-horiguchi@ah.jp.nec.com> wrote:
> > This patch applies Andrea's fix given by the following patch into
> > hugepage rmapping code:
> >
> >  commit 288468c334e98aacbb7e2fb8bde6bc1adcd55e05
> >  Author: Andrea Arcangeli <aarcange@redhat.com>
> >  Date:   Mon Aug 9 17:19:09 2010 -0700
> >
> > This patch uses anon_vma->root and avoids unnecessary overwriting
> > when anon_vma is already set up.
> 
> Btw, why isn't the code in __page_set_anon_rmap() also doing this
> cleaner version (ie a single "if (PageAnon(page)) return;" up front)?

Perhaps I misunderstand the question, but __page_set_anon_rmap
should handle Anon pages, shouldn't it?

> 
> The comments in that function are also some alien language translated
> to english by some broken automatic translation service. Could
> somebody clean up that function and come up with a comment that
> actually parses as English and makes sense?

I'll do that.

-Andi


-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [PATCH 1/4] hugetlb, rmap: always use anon_vma root pointer

[Andi Kleen]

On Fri, 10 Sep 2010 10:19:24 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Thu, Sep 9, 2010 at 9:23 PM, Naoya Horiguchi
> <n-horiguchi@ah.jp.nec.com> wrote:
> > This patch applies Andrea's fix given by the following patch into
> > hugepage rmapping code:
> >
> >  commit 288468c334e98aacbb7e2fb8bde6bc1adcd55e05
> >  Author: Andrea Arcangeli <aarcange@redhat.com>
> >  Date:   Mon Aug 9 17:19:09 2010 -0700
> >
> > This patch uses anon_vma->root and avoids unnecessary overwriting
> > when anon_vma is already set up.
> 
> Btw, why isn't the code in __page_set_anon_rmap() also doing this
> cleaner version (ie a single "if (PageAnon(page)) return;" up front)?

Perhaps I misunderstand the question, but __page_set_anon_rmap
should handle Anon pages, shouldn't it?

> 
> The comments in that function are also some alien language translated
> to english by some broken automatic translation service. Could
> somebody clean up that function and come up with a comment that
> actually parses as English and makes sense?

I'll do that.

-Andi


-- 
ak@linux.intel.com -- Speaking for myself only.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/4] hugetlb, rmap: always use anon_vma root pointer

[Linus Torvalds]

On Fri, Sep 10, 2010 at 2:50 PM, Andi Kleen <andi@firstfloor.org> wrote:
>> Btw, why isn't the code in __page_set_anon_rmap() also doing this
>> cleaner version (ie a single "if (PageAnon(page)) return;" up front)?
>
> Perhaps I misunderstand the question, but __page_set_anon_rmap
> should handle Anon pages, shouldn't it?

I'm talking about this:

    if (!exclusive) {
        if (PageAnon(page))
            return;
        anon_vma = anon_vma->root;
    } else {
        .. big bad comment ...
        if (PageAnon(page))
            return;
    }

where both sides of the if-statement start off doing the same thing.

It would be much cleaner to just do

    ... big _comprehensible_ comment ...
    if (PageAnon(page))
        return;

    if (!exclusive)
        anon_vma = anon_vma->root;

which avoids that silly else that just does something that we always do.

The reason I reacted is that Naoya-san's patch did that cleaner
version for the hugetlb case. So when I compared it to the non-hugetlb
case I just went "Ewww..."

                             Linus

Re: [PATCH 1/4] hugetlb, rmap: always use anon_vma root pointer

[Linus Torvalds]

On Fri, Sep 10, 2010 at 2:50 PM, Andi Kleen <andi@firstfloor.org> wrote:
>> Btw, why isn't the code in __page_set_anon_rmap() also doing this
>> cleaner version (ie a single "if (PageAnon(page)) return;" up front)?
>
> Perhaps I misunderstand the question, but __page_set_anon_rmap
> should handle Anon pages, shouldn't it?

I'm talking about this:

    if (!exclusive) {
        if (PageAnon(page))
            return;
        anon_vma = anon_vma->root;
    } else {
        .. big bad comment ...
        if (PageAnon(page))
            return;
    }

where both sides of the if-statement start off doing the same thing.

It would be much cleaner to just do

    ... big _comprehensible_ comment ...
    if (PageAnon(page))
        return;

    if (!exclusive)
        anon_vma = anon_vma->root;

which avoids that silly else that just does something that we always do.

The reason I reacted is that Naoya-san's patch did that cleaner
version for the hugetlb case. So when I compared it to the non-hugetlb
case I just went "Ewww..."

                             Linus

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/4] hugetlb, rmap: always use anon_vma root pointer

[Rik van Riel]

On 09/10/2010 05:50 PM, Andi Kleen wrote:
> On Fri, 10 Sep 2010 10:19:24 -0700
> Linus Torvalds<torvalds@linux-foundation.org>  wrote:
>
>> On Thu, Sep 9, 2010 at 9:23 PM, Naoya Horiguchi
>> <n-horiguchi@ah.jp.nec.com>  wrote:
>>> This patch applies Andrea's fix given by the following patch into
>>> hugepage rmapping code:
>>>
>>>   commit 288468c334e98aacbb7e2fb8bde6bc1adcd55e05
>>>   Author: Andrea Arcangeli<aarcange@redhat.com>
>>>   Date:   Mon Aug 9 17:19:09 2010 -0700
>>>
>>> This patch uses anon_vma->root and avoids unnecessary overwriting
>>> when anon_vma is already set up.
>>
>> Btw, why isn't the code in __page_set_anon_rmap() also doing this
>> cleaner version (ie a single "if (PageAnon(page)) return;" up front)?
>
> Perhaps I misunderstand the question, but __page_set_anon_rmap
> should handle Anon pages, shouldn't it?

__page_set_anon_rmap sets the page->mapping to be
a pointer to the anon_vma & PAGE_MAPPING_ANON.

PageAnon tests for page->mapping & PAGE_MAPPING_ANON,
ie. whether page->mapping is already pointing to an
anon_vma.

If it is, __page_set_anon_rmap should leave the page
mapping pointer alone.

-- 
All rights reversed

Re: [PATCH 1/4] hugetlb, rmap: always use anon_vma root pointer

[Rik van Riel]

On 09/10/2010 05:50 PM, Andi Kleen wrote:
> On Fri, 10 Sep 2010 10:19:24 -0700
> Linus Torvalds<torvalds@linux-foundation.org>  wrote:
>
>> On Thu, Sep 9, 2010 at 9:23 PM, Naoya Horiguchi
>> <n-horiguchi@ah.jp.nec.com>  wrote:
>>> This patch applies Andrea's fix given by the following patch into
>>> hugepage rmapping code:
>>>
>>>   commit 288468c334e98aacbb7e2fb8bde6bc1adcd55e05
>>>   Author: Andrea Arcangeli<aarcange@redhat.com>
>>>   Date:   Mon Aug 9 17:19:09 2010 -0700
>>>
>>> This patch uses anon_vma->root and avoids unnecessary overwriting
>>> when anon_vma is already set up.
>>
>> Btw, why isn't the code in __page_set_anon_rmap() also doing this
>> cleaner version (ie a single "if (PageAnon(page)) return;" up front)?
>
> Perhaps I misunderstand the question, but __page_set_anon_rmap
> should handle Anon pages, shouldn't it?

__page_set_anon_rmap sets the page->mapping to be
a pointer to the anon_vma & PAGE_MAPPING_ANON.

PageAnon tests for page->mapping & PAGE_MAPPING_ANON,
ie. whether page->mapping is already pointing to an
anon_vma.

If it is, __page_set_anon_rmap should leave the page
mapping pointer alone.

-- 
All rights reversed

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 3/4] hugetlb, rmap: fix confusing page locking in hugetlb_cow()

[Naoya Horiguchi]

Very sorry for late reply.
(Thank you for letting me know, Andi-san.)

On Fri, Sep 10, 2010 at 10:15:46AM -0700, Linus Torvalds wrote:
> 
> 
> On Fri, 10 Sep 2010, Naoya Horiguchi wrote:
> >  
> > -	if (!pagecache_page) {
> > -		page = pte_page(entry);
> > +	/*
> > +	 * hugetlb_cow() requires page locks of pte_page(entry) and
> > +	 * pagecache_page, so here we need take the former one
> > +	 * when page != pagecache_page or !pagecache_page.
> > +	 */
> > +	page = pte_page(entry);
> > +	if (page != pagecache_page)
> >  		lock_page(page);
> 
> Why isn't this a potential deadlock? You have two pages, and lock them 
> both. Is there some ordering guarantee that says that 'pagecache_page' and 
> 'page' will always be in a certain relationship so that you cannot get 
> A->B and B->A lock ordering?

Locking order is always pagecache -> page, so we are free from deadlock.

> Please document that ordering rule if so.

Yes. I comment it.

Please replace this patch by the following one.

Thanks,
Naoya Horiguchi

---
From: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
Date: Thu, 9 Sep 2010 16:39:33 +0900
Subject: [PATCH 3/4] hugetlb, rmap: fix confusing page locking in hugetlb_cow()

if(!trylock_page) block in avoidcopy path of hugetlb_cow() looks confusing
and is buggy.  Originally this trylock_page() is intended to make sure
that old_page is locked even when old_page != pagecache_page, because then
only pagecache_page is locked.
This patch fixes it by moving page locking into hugetlb_fault().

ChangeLog:
- add comment about deadlock.

Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
Acked-by: Rik van Riel <riel@redhat.com>
---
 mm/hugetlb.c |   22 ++++++++++++----------
 1 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 9519f3f..c032738 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -2324,11 +2324,8 @@ retry_avoidcopy:
 	 * and just make the page writable */
 	avoidcopy = (page_mapcount(old_page) == 1);
 	if (avoidcopy) {
-		if (!trylock_page(old_page)) {
-			if (PageAnon(old_page))
-				page_move_anon_rmap(old_page, vma, address);
-		} else
-			unlock_page(old_page);
+		if (PageAnon(old_page))
+			page_move_anon_rmap(old_page, vma, address);
 		set_huge_ptep_writable(vma, address, ptep);
 		return 0;
 	}
@@ -2631,10 +2628,16 @@ int hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma,
 								vma, address);
 	}
 
-	if (!pagecache_page) {
-		page = pte_page(entry);
+	/*
+	 * hugetlb_cow() requires page locks of pte_page(entry) and
+	 * pagecache_page, so here we need take the former one
+	 * when page != pagecache_page or !pagecache_page.
+	 * Note that locking order is always pagecache_page -> page,
+	 * so no worry about deadlock.
+	 */
+	page = pte_page(entry);
+	if (page != pagecache_page)
 		lock_page(page);
-	}
 
 	spin_lock(&mm->page_table_lock);
 	/* Check for a racing update before calling hugetlb_cow */
@@ -2661,9 +2664,8 @@ out_page_table_lock:
 	if (pagecache_page) {
 		unlock_page(pagecache_page);
 		put_page(pagecache_page);
-	} else {
-		unlock_page(page);
 	}
+	unlock_page(page);
 
 out_mutex:
 	mutex_unlock(&hugetlb_instantiation_mutex);
-- 
1.7.2.3

Re: [PATCH 3/4] hugetlb, rmap: fix confusing page locking in hugetlb_cow()

[Naoya Horiguchi]

Very sorry for late reply.
(Thank you for letting me know, Andi-san.)

On Fri, Sep 10, 2010 at 10:15:46AM -0700, Linus Torvalds wrote:
> 
> 
> On Fri, 10 Sep 2010, Naoya Horiguchi wrote:
> >  
> > -	if (!pagecache_page) {
> > -		page = pte_page(entry);
> > +	/*
> > +	 * hugetlb_cow() requires page locks of pte_page(entry) and
> > +	 * pagecache_page, so here we need take the former one
> > +	 * when page != pagecache_page or !pagecache_page.
> > +	 */
> > +	page = pte_page(entry);
> > +	if (page != pagecache_page)
> >  		lock_page(page);
> 
> Why isn't this a potential deadlock? You have two pages, and lock them 
> both. Is there some ordering guarantee that says that 'pagecache_page' and 
> 'page' will always be in a certain relationship so that you cannot get 
> A->B and B->A lock ordering?

Locking order is always pagecache -> page, so we are free from deadlock.

> Please document that ordering rule if so.

Yes. I comment it.

Please replace this patch by the following one.

Thanks,
Naoya Horiguchi

---
From: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
Date: Thu, 9 Sep 2010 16:39:33 +0900
Subject: [PATCH 3/4] hugetlb, rmap: fix confusing page locking in hugetlb_cow()

if(!trylock_page) block in avoidcopy path of hugetlb_cow() looks confusing
and is buggy.  Originally this trylock_page() is intended to make sure
that old_page is locked even when old_page != pagecache_page, because then
only pagecache_page is locked.
This patch fixes it by moving page locking into hugetlb_fault().

ChangeLog:
- add comment about deadlock.

Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
Acked-by: Rik van Riel <riel@redhat.com>
---
 mm/hugetlb.c |   22 ++++++++++++----------
 1 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 9519f3f..c032738 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -2324,11 +2324,8 @@ retry_avoidcopy:
 	 * and just make the page writable */
 	avoidcopy = (page_mapcount(old_page) == 1);
 	if (avoidcopy) {
-		if (!trylock_page(old_page)) {
-			if (PageAnon(old_page))
-				page_move_anon_rmap(old_page, vma, address);
-		} else
-			unlock_page(old_page);
+		if (PageAnon(old_page))
+			page_move_anon_rmap(old_page, vma, address);
 		set_huge_ptep_writable(vma, address, ptep);
 		return 0;
 	}
@@ -2631,10 +2628,16 @@ int hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma,
 								vma, address);
 	}
 
-	if (!pagecache_page) {
-		page = pte_page(entry);
+	/*
+	 * hugetlb_cow() requires page locks of pte_page(entry) and
+	 * pagecache_page, so here we need take the former one
+	 * when page != pagecache_page or !pagecache_page.
+	 * Note that locking order is always pagecache_page -> page,
+	 * so no worry about deadlock.
+	 */
+	page = pte_page(entry);
+	if (page != pagecache_page)
 		lock_page(page);
-	}
 
 	spin_lock(&mm->page_table_lock);
 	/* Check for a racing update before calling hugetlb_cow */
@@ -2661,9 +2664,8 @@ out_page_table_lock:
 	if (pagecache_page) {
 		unlock_page(pagecache_page);
 		put_page(pagecache_page);
-	} else {
-		unlock_page(page);
 	}
+	unlock_page(page);
 
 out_mutex:
 	mutex_unlock(&hugetlb_instantiation_mutex);
-- 
1.7.2.3

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>