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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxx>
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@xxxxxxxxxxxxx>
Acked-by: Rik van Riel <riel@xxxxxxxxxx>
---
 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@xxxxxxxxxx  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]