shared page table for hugetlbpage memory causing leak.

RFC In review Larry Woodman 3 participants 6 messages Jan 16, 2008 – Jan 17, 2008

Subsystems (2)

Memory management HugeTLB

lore

Share & tools

lore

Larry Woodman

author

2008-01-16 17:27 UTC #0 lore raw

RFC 1/1 shared page table for hugetlbpage memory causing leak.

I think the shared page table code for hugetlb memory on x86 and x86_64 is causing a leak. When a user of hugepages exits using this code the system leaks some of the hugepages.

-------------------------------------------------------
Part of /proc/meminfo just before database startup:

HugePages_Total:  5500
HugePages_Free:   5500
HugePages_Rsvd:      0
Hugepagesize:     2048 kB

Just before shutdown:

HugePages_Total:  5500
HugePages_Free:   4475
HugePages_Rsvd:      0
Hugepagesize:     2048 kB

After shutdown:
HugePages_Total: 5500 HugePages_Free: 4988 HugePages_Rsvd: 0 Hugepagesize: 2048 kB
----------------------------------------------------------

I think the problem occurs durring a fork, in copy_hugetlb_page_range(). It locates the dst_pte using huge_pte_alloc(). Since huge_pte_alloc() calls huge_pmd_share() it will share the pmd page if can yet the main loop in copy_hugetlb_page_range() does a get_page() on every hugepage. This is a violation of the shared hugepmd pagetable protocol and creates additional referenced to the hugepages.

I think we can skip the entire replication of the ptes when the hugepage pagetables are shared. This patch skips copying the ptes and the get_page() calls if the hugetlbpage pagetable is shared.

mm/hugetlb.c renamed from mm/hugetlb.c.orig +15 ~~-11~~

@@ -377,18 +377,22 @@ int copy_hugetlb_page_range(struct mm_st
 		dst_pte = huge_pte_alloc(dst, addr);
 		if (!dst_pte)
 			goto nomem;
-		spin_lock(&dst->page_table_lock);
-		spin_lock(&src->page_table_lock);
-		if (!pte_none(*src_pte)) {
-			if (cow)
-				ptep_set_wrprotect(src, addr, src_pte);
-			entry = *src_pte;
-			ptepage = pte_page(entry);
-			get_page(ptepage);
-			set_huge_pte_at(dst, addr, dst_pte, entry);
+
+		/* if hugetlbpage pagetables are shared dont take additional references */
+		if(!(is_vm_hugtlb_page(vma) && dst_pte == src_pte)) {
+			spin_lock(&dst->page_table_lock);
+			spin_lock(&src->page_table_lock);
+			if (!pte_none(*src_pte)) {
+				if (cow)
+					ptep_set_wrprotect(src, addr, src_pte);
+				entry = *src_pte;
+				ptepage = pte_page(entry);
+				get_page(ptepage);
+				set_huge_pte_at(dst, addr, dst_pte, entry);
+			}
+			spin_unlock(&src->page_table_lock);
+			spin_unlock(&dst->page_table_lock);
 		}
-		spin_unlock(&src->page_table_lock);
-		spin_unlock(&dst->page_table_lock);
 	}
 	return 0;

Adam Litke

2008-01-16 18:50 UTC #1 lore raw

Since we know we are dealing with a hugetlb VMA, how about the following, simpler, _untested_ patch:

Signed-off-by Adam Litke <[email protected]>

mm/hugetlb.c +5 -0

index 6f97821..75b0e4f 100644
@@ -644,6 +644,11 @@ int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
 		dst_pte = huge_pte_alloc(dst, addr);
 		if (!dst_pte)
 			goto nomem;
+
+		/* If page table is shared do not copy or take references */
+		if (src_pte == dst_pte)
+			continue;
+
 		spin_lock(&dst->page_table_lock);
 		spin_lock(&src->page_table_lock);
 		if (!pte_none(*src_pte)) {

Signature · 8 lines

Adam Litke - (agl at us.ibm.com)
IBM Linux Technology Center

--
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>

Larry Woodman

author

2008-01-16 18:55 UTC #2 lore raw

Quoted 15 lines · Adam Litke

Since we know we are dealing with a hugetlb VMA, how about the following, simpler, _untested_ patch:
Signed-off-by Adam Litke <[email protected]>
mm/hugetlb.c +0 -0
index 6f97821..75b0e4f 100644
@@ -644,6 +644,11 @@ int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
 		dst_pte = huge_pte_alloc(dst, addr);

mm/hugetlb.c int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src, · line 654

Quoted 10 lines

mm/hugetlb.c +5 -0

 		if (!dst_pte)
 			goto nomem;
+
+		/* If page table is shared do not copy or take references */
+		if (src_pte == dst_pte)
+			continue;
+
 		spin_lock(&dst->page_table_lock);
 		spin_lock(&src->page_table_lock);
 		if (!pte_none(*src_pte)) {

Agreed.

--
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>

Balbir Singh

2008-01-17 10:19 UTC #3 lore raw

In reply to Adam Litke

Quoted 10 lines · Adam Litke

Since we know we are dealing with a hugetlb VMA, how about the following, simpler, _untested_ patch:
Signed-off-by Adam Litke <[email protected]>
mm/hugetlb.c +0 -0
index 6f97821..75b0e4f 100644

mm/hugetlb.c int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src, · line 651

Quoted 8 lines

mm/hugetlb.c +5 -0

@@ -644,6 +644,11 @@ int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
 		dst_pte = huge_pte_alloc(dst, addr);
 		if (!dst_pte)
 			goto nomem;
+
+		/* If page table is shared do not copy or take references */
+		if (src_pte == dst_pte)
+			continue;
+

Shouldn't you be checking the PTE contents rather than the pointers? Shouldn't the check be

                if (unlikely(pte_same(*src_pte, *dst_pte))
                        continue;

Quoted 4 lines

mm/hugetlb.c +0 -0

 		spin_lock(&dst->page_table_lock);
 		spin_lock(&src->page_table_lock);
 		if (!pte_none(*src_pte)) {

Signature · 10 lines

	Warm Regards,
	Balbir Singh
	Linux Technology Center
	IBM, ISTL

--
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>

Larry Woodman

author

2008-01-17 11:53 UTC #4 lore raw

Quoted 7 lines · Balbir Singh

Quoted 5 lines · Adam Litke
Since we know we are dealing with a hugetlb VMA, how about the following, simpler, _untested_ patch:
Signed-off-by Adam Litke <[email protected]>

Quoted 15 lines

mm/hugetlb.c int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src, · line 651
Quoted 13 lines
mm/hugetlb.c +5 -0
index 6f97821..75b0e4f 100644
@@ -644,6 +644,11 @@ int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
 		dst_pte = huge_pte_alloc(dst, addr);
 		if (!dst_pte)
 			goto nomem;
+
+		/* If page table is shared do not copy or take references */
+		if (src_pte == dst_pte)
+			continue;
+
Shouldn't you be checking the PTE contents rather than the pointers?

No, this is chacking for shared page tables not shared pages.

Quoted 11 lines

Shouldn't the check be

                if (unlikely(pte_same(*src_pte, *dst_pte))
                        continue;

Quoted 4 lines

mm/hugetlb.c +0 -0

 		spin_lock(&dst->page_table_lock);
 		spin_lock(&src->page_table_lock);
 		if (!pte_none(*src_pte)) {

Balbir Singh

2008-01-17 12:12 UTC #5 lore raw

Quoted 23 lines · Larry Woodman

Quoted 7 lines · Balbir Singh
Quoted 5 lines · Adam Litke
Since we know we are dealing with a hugetlb VMA, how about the following, simpler, _untested_ patch:
Signed-off-by Adam Litke <[email protected]>
Quoted 15 lines
mm/hugetlb.c int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src, · line 651
Quoted 13 lines
mm/hugetlb.c +5 -0
index 6f97821..75b0e4f 100644
@@ -644,6 +644,11 @@ int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
 		dst_pte = huge_pte_alloc(dst, addr);
 		if (!dst_pte)
 			goto nomem;
+
+		/* If page table is shared do not copy or take references */
+		if (src_pte == dst_pte)
+			continue;
+
Shouldn't you be checking the PTE contents rather than the pointers?

Quoted 1 line

No, this is chacking for shared page tables not shared pages.

Aah.. I see.

Thanks for clarifying!

Signature · 9 lines

	Balbir Singh
	Linux Technology Center
	IBM, ISTL

--
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>