thp: tidy and fix khugepaged_prealloc_page

PATCH In review Xiao Guangrong 2 participants 6 messages Sep 12, 2012 – Sep 13, 2012

Subsystems (2)

Memory management THP

lore

Share & tools

lore

Xiao Guangrong

author

2012-09-12 12:55 UTC #0 lore raw

cover thp: tidy and fix khugepaged_prealloc_page

There has a bug in khugepaged_prealloc_page, the page-alloc indicator is not reset if the previous page request is failed, then it will trigger the VM_BUG_ON in khugepaged_alloc_page. It is fixed by the first patch which need not be back port for it was introduced by recent commit. (sorry for that)

As Hugh pointed out, this are some ugly portions:
- releasing mmap_sem lock is hidden in khugepaged_alloc_page
- page is freed in khugepaged_prealloc_page
The later two patches try to fix these issues.

Hugh,

If any point i missed, please let me know, and sorry to waste your time on my broken patch.

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

Xiao Guangrong

author

2012-09-12 12:56 UTC #1 lore raw

PATCH 1/3 thp: fix forgetting to reset the page alloc indicator

If NUMA is enabled, the indicator is not reset if the previous page request is failed, then it will trigger the VM_BUG_ON in khugepaged_alloc_page

Signed-off-by Xiao Guangrong <[email protected]>

---
 mm/huge_memory.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

mm/huge_memory.c +1 -0

index e366ca5..66d2bc6 100644
@@ -1825,6 +1825,7 @@ static bool khugepaged_prealloc_page(struct page **hpage, bool *wait)
 			return false;
 
 		*wait = false;
+		*hpage = NULL;
 		khugepaged_alloc_sleep();
 	} else if (*hpage) {
 		put_page(*hpage);

Signature · 1 line

1.7.7.6

Xiao Guangrong

author

2012-09-12 12:56 UTC #2 lore raw

PATCH 2/3 thp: move release mmap_sem lock out of khugepaged_alloc_page

In reply to Xiao Guangrong

To make the code more clear, move release the lock out of khugepaged_alloc_page

Signed-off-by Xiao Guangrong <[email protected]>

---
 mm/huge_memory.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

mm/huge_memory.c +7 -7

index 66d2bc6..5622347 100644
@@ -1854,11 +1854,6 @@ static struct page
 	*hpage  = alloc_hugepage_vma(khugepaged_defrag(), vma, address,
 				      node, __GFP_OTHER_NODE);
 
-	/*
-	 * After allocating the hugepage, release the mmap_sem read lock in
-	 * preparation for taking it in write mode.
-	 */
-	up_read(&mm->mmap_sem);
 	if (unlikely(!*hpage)) {
 		count_vm_event(THP_COLLAPSE_ALLOC_FAILED);
 		*hpage = ERR_PTR(-ENOMEM);

@@ -1905,7 +1900,6 @@ static struct page
 		       struct vm_area_struct *vma, unsigned long address,
 		       int node)
 {
-	up_read(&mm->mmap_sem);
 	VM_BUG_ON(!*hpage);
 	return  *hpage;
 }

@@ -1931,8 +1925,14 @@ static void collapse_huge_page(struct mm_struct *mm,
 
 	VM_BUG_ON(address & ~HPAGE_PMD_MASK);
 
-	/* release the mmap_sem read lock. */
 	new_page = khugepaged_alloc_page(hpage, mm, vma, address, node);
+
+	/*
+	 * After allocating the hugepage, release the mmap_sem read lock in
+	 * preparation for taking it in write mode.
+	 */
+	up_read(&mm->mmap_sem);
+
 	if (!new_page)
 		return;

Signature · 7 lines

1.7.7.6

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

Andrew Morton

maintainer

2012-09-12 22:18 UTC #3 lore raw

Re: [PATCH 2/3] thp: move release mmap_sem lock out of khugepaged_alloc_page

Quoted 31 lines · On Wed, 12 Sep 2012 20:56:41 +0800 Xiao Guangrong…

To make the code more clear, move release the lock out of khugepaged_alloc_page

...

mm/huge_memory.c +0 -7

@@ -1854,11 +1854,6 @@ static struct page
 	*hpage  = alloc_hugepage_vma(khugepaged_defrag(), vma, address,
 				      node, __GFP_OTHER_NODE);
 
-	/*
-	 * After allocating the hugepage, release the mmap_sem read lock in
-	 * preparation for taking it in write mode.
-	 */
-	up_read(&mm->mmap_sem);
 	if (unlikely(!*hpage)) {
 		count_vm_event(THP_COLLAPSE_ALLOC_FAILED);
 		*hpage = ERR_PTR(-ENOMEM);

@@ -1905,7 +1900,6 @@ static struct page
 		       struct vm_area_struct *vma, unsigned long address,
 		       int node)
 {
-	up_read(&mm->mmap_sem);
 	VM_BUG_ON(!*hpage);
 	return  *hpage;
 }

@@ -1931,8 +1925,14 @@ static void collapse_huge_page(struct mm_struct *mm,
 
 	VM_BUG_ON(address & ~HPAGE_PMD_MASK);
 
-	/* release the mmap_sem read lock. */

mm/huge_memory.c static void collapse_huge_page(struct mm_struct *mm, · line 1937

Quoted 10 lines

mm/huge_memory.c +7 -0

 	new_page = khugepaged_alloc_page(hpage, mm, vma, address, node);
+
+	/*
+	 * After allocating the hugepage, release the mmap_sem read lock in
+	 * preparation for taking it in write mode.
+	 */
+	up_read(&mm->mmap_sem);
+
 	if (!new_page)
 		return;

Well that's a pretty minor improvement: one still has to go off on a big hunt to locate the matching down_read().

And the patch will increase mmap_sem hold times by a teeny amount. Do we really want to do this?

Xiao Guangrong

author

2012-09-13 09:17 UTC #4 lore raw

Re: [PATCH 2/3] thp: move release mmap_sem lock out of khugepaged_alloc_page

Quoted 48 lines · Andrew Morton

Quoted 31 lines · On Wed, 12 Sep 2012 20:56:41 +0800 Xiao Guangrong…

To make the code more clear, move release the lock out of khugepaged_alloc_page

...

mm/huge_memory.c +0 -7

@@ -1854,11 +1854,6 @@ static struct page
 	*hpage  = alloc_hugepage_vma(khugepaged_defrag(), vma, address,
 				      node, __GFP_OTHER_NODE);
 
-	/*
-	 * After allocating the hugepage, release the mmap_sem read lock in
-	 * preparation for taking it in write mode.
-	 */
-	up_read(&mm->mmap_sem);
 	if (unlikely(!*hpage)) {
 		count_vm_event(THP_COLLAPSE_ALLOC_FAILED);
 		*hpage = ERR_PTR(-ENOMEM);

@@ -1905,7 +1900,6 @@ static struct page
 		       struct vm_area_struct *vma, unsigned long address,
 		       int node)
 {
-	up_read(&mm->mmap_sem);
 	VM_BUG_ON(!*hpage);
 	return  *hpage;
 }

@@ -1931,8 +1925,14 @@ static void collapse_huge_page(struct mm_struct *mm,
 
 	VM_BUG_ON(address & ~HPAGE_PMD_MASK);
 
-	/* release the mmap_sem read lock. */

mm/huge_memory.c static void collapse_huge_page(struct mm_struct *mm, · line 1937

Quoted 10 lines

mm/huge_memory.c +7 -0

 	new_page = khugepaged_alloc_page(hpage, mm, vma, address, node);
+
+	/*
+	 * After allocating the hugepage, release the mmap_sem read lock in
+	 * preparation for taking it in write mode.
+	 */
+	up_read(&mm->mmap_sem);
+
 	if (!new_page)
 		return;

Well that's a pretty minor improvement: one still has to go off on a big hunt to locate the matching down_read().

Quoted 2 lines

And the patch will increase mmap_sem hold times by a teeny amount. Do we really want to do this?

Andrew,

This is why i did in the previous patch (the lock is released in alloc function), but as you noticed, this is really teeny overload after this patch - only increase the load of count_vm_event() which operates a cpu-local variable. And, before i posted this patch, i did kerbench test, no regression was found.

The another approach is, let the function name indicate the lock will be released, how about just change the function name to khugepaged_alloc_page_release_lock?

Xiao Guangrong

author

2012-09-12 12:57 UTC #5 lore raw

PATCH 3/3 thp: introduce khugepaged_cleanup_page

In reply to Xiao Guangrong

It is used to release the page on the fail path, then the page need not be cleaned up in khugepaged_prealloc_page anymore

Signed-off-by Xiao Guangrong <[email protected]>

---
 mm/huge_memory.c |   19 +++++++++++++++----
 1 files changed, 15 insertions(+), 4 deletions(-)

mm/huge_memory.c +15 -4

index 5622347..de0a028 100644
@@ -1827,9 +1827,6 @@ static bool khugepaged_prealloc_page(struct page **hpage, bool *wait)
 		*wait = false;
 		*hpage = NULL;
 		khugepaged_alloc_sleep();
-	} else if (*hpage) {
-		put_page(*hpage);
-		*hpage = NULL;
 	}
 
 	return true;

@@ -1863,6 +1860,13 @@ static struct page
 	count_vm_event(THP_COLLAPSE_ALLOC);
 	return *hpage;
 }
+
+static void khugepaged_cleanup_page(struct page **hpage)
+{
+	VM_BUG_ON(!*hpage);
+	put_page(*hpage);
+	*hpage = NULL;
+}
 #else
 static struct page *khugepaged_alloc_hugepage(bool *wait)
 {

@@ -1903,6 +1907,10 @@ static struct page
 	VM_BUG_ON(!*hpage);
 	return  *hpage;
 }
+
+static void khugepaged_cleanup_page(struct page **hpage)
+{
+}
 #endif
 
 static void collapse_huge_page(struct mm_struct *mm,

@@ -1936,8 +1944,10 @@ static void collapse_huge_page(struct mm_struct *mm,
 	if (!new_page)
 		return;
 
-	if (unlikely(mem_cgroup_newpage_charge(new_page, mm, GFP_KERNEL)))
+	if (unlikely(mem_cgroup_newpage_charge(new_page, mm, GFP_KERNEL))) {
+		khugepaged_cleanup_page(hpage);
 		return;
+	}
 
 	/*
 	 * Prevent all access to pagetables with the exception of

@@ -2048,6 +2058,7 @@ out_up_write:
 	return;
 
 out:
+	khugepaged_cleanup_page(hpage);
 	mem_cgroup_uncharge_page(new_page);
 	goto out_up_write;
 }

Signature · 1 line

1.7.7.6