fix mlocked page counter mismatch

PATCH Superseded MinChan Kim 2 participants 5 messages Feb 2, 2009 – Feb 3, 2009

Subsystems (2)

Memory management MM rmap

lore

Share & tools

lore

MinChan Kim

author

2009-02-02 06:16 UTC #0 lore raw

PATCH 1/1 fix mlocked page counter mismatch

When I tested following program, I found that mlocked counter is strange.
It couldn't free some mlocked pages of test program.

It is caused that try_to_unmap_file don't check real
page mapping in vmas.
That's because goal of address_space for file is to find all processes into which the file's specific interval is mapped.
What I mean is that it's not related page but file's interval.

Even if the page isn't really mapping at the vma, it returns SWAP_MLOCK since the vma have VM_LOCKED, then calls try_to_mlock_page. After all, mlocked counter is increased again.

This patch is based on 2.6.28-rc2-mm1.

-- my test program --

#include <stdio.h>
#include <sys/mman.h>
int main()

{
        mlockall(MCL_CURRENT);
        return 0;
}

-- before --

root@barrios-target-linux:~# cat /proc/meminfo | egrep 'Mlo|Unev'
Unevictable:           0 kB
Mlocked:               0 kB

-- after --

root@barrios-target-linux:~# cat /proc/meminfo | egrep 'Mlo|Unev'
Unevictable:           8 kB
Mlocked:               8 kB

mm/rmap.c +10 -0

index 1099394..9ba1fdf 100644
@@ -1073,6 +1073,9 @@ static int try_to_unmap_file(struct page *page, int unlock, int migration)
 	unsigned long max_nl_size = 0;
 	unsigned int mapcount;
 	unsigned int mlocked = 0;
+	unsigned long address;
+	pte_t *pte;
+	spinlock_t *ptl;
 
 	if (MLOCK_PAGES && unlikely(unlock))
 		ret = SWAP_SUCCESS;	/* default for try_to_munlock() */

@@ -1089,6 +1092,13 @@ static int try_to_unmap_file(struct page *page, int unlock, int migration)
 				goto out;
 		}
 		if (ret == SWAP_MLOCK) {
+     address = vma_address(page, vma);
+     if (address != -EFAULT) {
+       pte = page_check_address(page, vma->vm_mm, address, &ptl, 0);
+       if (!pte)
+            continue; 
+       pte_unmap_unlock(pte, ptl);
+     } 
 			mlocked = try_to_mlock_page(page, vma);
 			if (mlocked)
 				break;  /* stop if actually mlocked page */

Signature · 8 lines

Kinds Regards
MinChan Kim

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

Lee Schermerhorn

2009-02-02 17:16 UTC #1 lore raw

Quoted 59 lines · MinChan Kim

When I tested following program, I found that mlocked counter is strange.
It couldn't free some mlocked pages of test program.
It is caused that try_to_unmap_file don't check real
page mapping in vmas.
That's because goal of address_space for file is to find all processes into which the file's specific interval is mapped.
What I mean is that it's not related page but file's interval.
Even if the page isn't really mapping at the vma, it returns SWAP_MLOCK since the vma have VM_LOCKED, then calls try_to_mlock_page. After all, mlocked counter is increased again.
This patch is based on 2.6.28-rc2-mm1.
-- my test program --
#include <stdio.h>
#include <sys/mman.h>
int main()
{
        mlockall(MCL_CURRENT);
        return 0;
}
-- before --
root@barrios-target-linux:~# cat /proc/meminfo | egrep 'Mlo|Unev'
Unevictable:           0 kB
Mlocked:               0 kB
-- after --
root@barrios-target-linux:~# cat /proc/meminfo | egrep 'Mlo|Unev'
Unevictable:           8 kB
Mlocked:               8 kB
--
mm/rmap.c +3 -0
index 1099394..9ba1fdf 100644
@@ -1073,6 +1073,9 @@ static int try_to_unmap_file(struct page *page, int unlock, int migration)
 	unsigned long max_nl_size = 0;
 	unsigned int mapcount;
 	unsigned int mlocked = 0;
+	unsigned long address;
+	pte_t *pte;
+	spinlock_t *ptl;
 
 	if (MLOCK_PAGES && unlikely(unlock))
 		ret = SWAP_SUCCESS;	/* default for try_to_munlock() */
@@ -1089,6 +1092,13 @@ static int try_to_unmap_file(struct page *page, int unlock, int migration)
 				goto out;
 		}
 		if (ret == SWAP_MLOCK) {

mm/rmap.c static int try_to_unmap_file(struct page *page, int unlock, int migration) · line 1104

Quoted 10 lines

mm/rmap.c +7 -0

+     address = vma_address(page, vma);
+     if (address != -EFAULT) {
+       pte = page_check_address(page, vma->vm_mm, address, &ptl, 0);
+       if (!pte)
+            continue; 
+       pte_unmap_unlock(pte, ptl);
+     } 
 			mlocked = try_to_mlock_page(page, vma);
 			if (mlocked)
 				break;  /* stop if actually mlocked page */

Hi, MinChan:

Interestingly, Rik had addressed this [simpler patch below] way back
when he added the page_mapped_in_vma() function. I asked him whether the rb tree shouldn't have filtered any vmas that didn't have the page mapped. He agreed and removed the check from try_to_unmap_file(). Guess I can be very convincing, even when I'm wrong [happening a lot lately]. Of course, in this instance, the rb-tree filtering only works for shared, page-cache pages. The problem uncovered by your test case is with a COWed anon page in a file-backed vma. Yes, the vma 'maps' the virtual address range containing the page in question, but since it's a private COWed anon page, it isn't necessarily "mapped" in the VM_LOCKED vma's mm's page table. We need the check...

I've added the variant below [CURRENTLY UNTESTED] to my test tree.

Lee

[intentionally omitted sign off, until tested.]

mm/rmap.c +2 -1

Index: linux-2.6.29-rc3/mm/rmap.c
===================================================================
@@ -1072,7 +1072,8 @@ static int try_to_unmap_file(struct page
 	spin_lock(&mapping->i_mmap_lock);
 	vma_prio_tree_foreach(vma, &iter, &mapping->i_mmap, pgoff, pgoff) {
 		if (MLOCK_PAGES && unlikely(unlock)) {
-			if (!(vma->vm_flags & VM_LOCKED))
+			if (!((vma->vm_flags & VM_LOCKED) &&
+			      page_mapped_in_vma(page, vma)))
 				continue;	/* must visit all vmas */
 			ret = SWAP_MLOCK;
 		} else {

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

MinChan Kim

author

2009-02-02 23:27 UTC #2 lore raw

Quoted 77 lines · Lee Schermerhorn

Quoted 59 lines · MinChan Kim
When I tested following program, I found that mlocked counter is strange.
It couldn't free some mlocked pages of test program.
It is caused that try_to_unmap_file don't check real
page mapping in vmas.
That's because goal of address_space for file is to find all processes into which the file's specific interval is mapped.
What I mean is that it's not related page but file's interval.
Even if the page isn't really mapping at the vma, it returns SWAP_MLOCK since the vma have VM_LOCKED, then calls try_to_mlock_page. After all, mlocked counter is increased again.
This patch is based on 2.6.28-rc2-mm1.
-- my test program --
#include <stdio.h>
#include <sys/mman.h>
int main()
{
        mlockall(MCL_CURRENT);
        return 0;
}
-- before --
root@barrios-target-linux:~# cat /proc/meminfo | egrep 'Mlo|Unev'
Unevictable:           0 kB
Mlocked:               0 kB
-- after --
root@barrios-target-linux:~# cat /proc/meminfo | egrep 'Mlo|Unev'
Unevictable:           8 kB
Mlocked:               8 kB
--
mm/rmap.c +3 -0
index 1099394..9ba1fdf 100644
@@ -1073,6 +1073,9 @@ static int try_to_unmap_file(struct page *page, int unlock, int migration)
 	unsigned long max_nl_size = 0;
 	unsigned int mapcount;
 	unsigned int mlocked = 0;
+	unsigned long address;
+	pte_t *pte;
+	spinlock_t *ptl;
 
 	if (MLOCK_PAGES && unlikely(unlock))
 		ret = SWAP_SUCCESS;	/* default for try_to_munlock() */
@@ -1089,6 +1092,13 @@ static int try_to_unmap_file(struct page *page, int unlock, int migration)
 				goto out;
 		}
 		if (ret == SWAP_MLOCK) {
mm/rmap.c static int try_to_unmap_file(struct page *page, int unlock, int migration) · line 1104
Quoted 10 lines
mm/rmap.c +7 -0
+     address = vma_address(page, vma);
+     if (address != -EFAULT) {
+       pte = page_check_address(page, vma->vm_mm, address, &ptl, 0);
+       if (!pte)
+            continue; 
+       pte_unmap_unlock(pte, ptl);
+     } 
 			mlocked = try_to_mlock_page(page, vma);
 			if (mlocked)
 				break;  /* stop if actually mlocked page */
Hi, MinChan:
Interestingly, Rik had addressed this [simpler patch below] way back
when he added the page_mapped_in_vma() function. I asked him whether

Quoted 5 lines

the rb tree shouldn't have filtered any vmas that didn't have the page mapped. He agreed and removed the check from try_to_unmap_file(). Guess I can be very convincing, even when I'm wrong [happening a lot lately]. Of course, in this instance, the rb-tree filtering only works for shared, page-cache pages. The problem uncovered by your test case

It's not rb-tree but priority tree. ;-)

Quoted 4 lines

is with a COWed anon page in a file-backed vma. Yes, the vma 'maps' the virtual address range containing the page in question, but since it's a private COWed anon page, it isn't necessarily "mapped" in the VM_LOCKED vma's mm's page table. We need the check...

Indeed!

Quoted 6 lines

I've added the variant below [CURRENTLY UNTESTED] to my test tree.
Lee
[intentionally omitted sign off, until tested.]

I shouldn't forgot page_mapped_in_vma.
However, It looks good to me.
Thank you for testing.

Quoted 17 lines

Index: linux-2.6.29-rc3/mm/rmap.c
===================================================================

mm/rmap.c +2 -1

@@ -1072,7 +1072,8 @@ static int try_to_unmap_file(struct page
 	spin_lock(&mapping->i_mmap_lock);
 	vma_prio_tree_foreach(vma, &iter, &mapping->i_mmap, pgoff, pgoff) {
 		if (MLOCK_PAGES && unlikely(unlock)) {
-			if (!(vma->vm_flags & VM_LOCKED))
+			if (!((vma->vm_flags & VM_LOCKED) &&
+			      page_mapped_in_vma(page, vma)))
 				continue;	/* must visit all vmas */
 			ret = SWAP_MLOCK;
 		} else {

Signature · 8 lines

Kinds Regards
MinChan Kim

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

Lee Schermerhorn

2009-02-03 01:47 UTC #3 lore raw

Quoted 78 lines · MinChan Kim

Quoted 75 lines · Lee Schermerhorn
Quoted 59 lines · MinChan Kim
When I tested following program, I found that mlocked counter is strange.
It couldn't free some mlocked pages of test program.
It is caused that try_to_unmap_file don't check real
page mapping in vmas.
That's because goal of address_space for file is to find all processes into which the file's specific interval is mapped.
What I mean is that it's not related page but file's interval.
Even if the page isn't really mapping at the vma, it returns SWAP_MLOCK since the vma have VM_LOCKED, then calls try_to_mlock_page. After all, mlocked counter is increased again.
This patch is based on 2.6.28-rc2-mm1.
-- my test program --
#include <stdio.h>
#include <sys/mman.h>
int main()
{
        mlockall(MCL_CURRENT);
        return 0;
}
-- before --
root@barrios-target-linux:~# cat /proc/meminfo | egrep 'Mlo|Unev'
Unevictable:           0 kB
Mlocked:               0 kB
-- after --
root@barrios-target-linux:~# cat /proc/meminfo | egrep 'Mlo|Unev'
Unevictable:           8 kB
Mlocked:               8 kB
--
mm/rmap.c +3 -0
index 1099394..9ba1fdf 100644
@@ -1073,6 +1073,9 @@ static int try_to_unmap_file(struct page *page, int unlock, int migration)
 	unsigned long max_nl_size = 0;
 	unsigned int mapcount;
 	unsigned int mlocked = 0;
+	unsigned long address;
+	pte_t *pte;
+	spinlock_t *ptl;
 
 	if (MLOCK_PAGES && unlikely(unlock))
 		ret = SWAP_SUCCESS;	/* default for try_to_munlock() */
@@ -1089,6 +1092,13 @@ static int try_to_unmap_file(struct page *page, int unlock, int migration)
 				goto out;
 		}
 		if (ret == SWAP_MLOCK) {
mm/rmap.c static int try_to_unmap_file(struct page *page, int unlock, int migration) · line 1104
Quoted 10 lines
mm/rmap.c +7 -0
+     address = vma_address(page, vma);
+     if (address != -EFAULT) {
+       pte = page_check_address(page, vma->vm_mm, address, &ptl, 0);
+       if (!pte)
+            continue; 
+       pte_unmap_unlock(pte, ptl);
+     } 
 			mlocked = try_to_mlock_page(page, vma);
 			if (mlocked)
 				break;  /* stop if actually mlocked page */
Hi, MinChan:
Interestingly, Rik had addressed this [simpler patch below] way back
when he added the page_mapped_in_vma() function. I asked him whether

Quoted 7 lines

Quoted 5 lines
the rb tree shouldn't have filtered any vmas that didn't have the page mapped. He agreed and removed the check from try_to_unmap_file(). Guess I can be very convincing, even when I'm wrong [happening a lot lately]. Of course, in this instance, the rb-tree filtering only works for shared, page-cache pages. The problem uncovered by your test case
It's not rb-tree but priority tree. ;-)

Yeah, that one :).

Quoted 11 lines

Quoted 5 lines
is with a COWed anon page in a file-backed vma. Yes, the vma 'maps' the virtual address range containing the page in question, but since it's a private COWed anon page, it isn't necessarily "mapped" in the VM_LOCKED vma's mm's page table. We need the check...
Indeed!
Quoted 3 lines
I've added the variant below [CURRENTLY UNTESTED] to my test tree.

Quoted 7 lines

Quoted 3 lines
Lee
[intentionally omitted sign off, until tested.]
I shouldn't forgot page_mapped_in_vma.
However, It looks good to me.
Thank you for testing.

I did test this on 29-rc3 on a 4 socket x dual core x86_64 platform and it seems to resolve the statistics miscount. How do you want to proceed. Do you want to repost with this version of patch? Or shall I?

Regards,
Lee

Quoted 18 lines

Quoted 8 lines

Index: linux-2.6.29-rc3/mm/rmap.c
===================================================================

mm/rmap.c static int try_to_unmap_file(struct page · line 1079

Quoted 9 lines

mm/rmap.c +2 -1

@@ -1072,7 +1072,8 @@ static int try_to_unmap_file(struct page
 	spin_lock(&mapping->i_mmap_lock);
 	vma_prio_tree_foreach(vma, &iter, &mapping->i_mmap, pgoff, pgoff) {
 		if (MLOCK_PAGES && unlikely(unlock)) {
-			if (!(vma->vm_flags & VM_LOCKED))
+			if (!((vma->vm_flags & VM_LOCKED) &&
+			      page_mapped_in_vma(page, vma)))
 				continue;	/* must visit all vmas */
 			ret = SWAP_MLOCK;
 		} else {

MinChan Kim

author

2009-02-03 01:58 UTC #4 lore raw

Quoted 5 lines

Quoted 1 line
Thank you for testing.
I did test this on 29-rc3 on a 4 socket x dual core x86_64 platform and it seems to resolve the statistics miscount. How do you want to proceed. Do you want to repost with this version of patch? Or shall I?

I will repost this patch with your ACK and tested-by if you allow it.

Quoted 22 lines

Regards,
Lee

Quoted 18 lines

Quoted 17 lines

Index: linux-2.6.29-rc3/mm/rmap.c
===================================================================

mm/rmap.c +2 -1

@@ -1072,7 +1072,8 @@ static int try_to_unmap_file(struct page
 	spin_lock(&mapping->i_mmap_lock);
 	vma_prio_tree_foreach(vma, &iter, &mapping->i_mmap, pgoff, pgoff) {
 		if (MLOCK_PAGES && unlikely(unlock)) {
-			if (!(vma->vm_flags & VM_LOCKED))
+			if (!((vma->vm_flags & VM_LOCKED) &&
+			      page_mapped_in_vma(page, vma)))
 				continue;	/* must visit all vmas */
 			ret = SWAP_MLOCK;
 		} else {

Signature · 8 lines

Kinds Regards
MinChan Kim

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