mbind() fails to fail with EIO

DISCUSSION Cyril Hrubis 5 participants 12 messages Mar 15, 2019 – Mar 19, 2019

Subsystems (2)

Memory management MM NUMA placement

lore

Share & tools

lore

Cyril Hrubis

author

2019-03-15 16:01 UTC #0 lore raw

Hi!
I've started to write tests for mbind() and found out that mbind() does not work as described in manual page in a case that page has been faulted on different node that we are asking it to bind to. Looks like this is working fine on older kernels. On my testing machine with 3.0 mbind() fails correctly with EIO but succeeds unexpectedly on newer kernels such as 4.12.

What the test does is:

* mmap() private mapping
* fault it
* find out on which node it is faulted on
* mbind() it to a different node with MPOL_BIND and MPOL_MF_STRICT and expects to get EIO

The test code can be seen and compiled from:

https://github.com/metan-ucw/ltp/blob/master/testcases/kernel/syscalls/mbind/mbind02.c

Signature · 2 lines

Cyril Hrubis
[email protected]

Qian Cai

2019-03-18 16:08 UTC #1 lore raw

Quoted 16 lines · Cyril Hrubis

Hi!
I've started to write tests for mbind() and found out that mbind() does not work as described in manual page in a case that page has been faulted on different node that we are asking it to bind to. Looks like this is working fine on older kernels. On my testing machine with 3.0 mbind() fails correctly with EIO but succeeds unexpectedly on newer kernels such as 4.12.
What the test does is:
* mmap() private mapping
* fault it
* find out on which node it is faulted on
* mbind() it to a different node with MPOL_BIND and MPOL_MF_STRICT and Â expects to get EIO

Quoted 5 lines

The test code can be seen and compiled from:
https://github.com/metan-ucw/ltp/blob/master/testcases/kernel/syscalls/mbind/m
bind02.c

I am too lazy to checkout the repository and compile the whole thing just to be able to reproduce. If you can make it a standalone program without LTP markups, I'd be happy to take a look.

Cyril Hrubis

author

2019-03-19 12:59 UTC #2 lore raw

Hi!

Quoted 3 lines

I am too lazy to checkout the repository and compile the whole thing just to be able to reproduce. If you can make it a standalone program without LTP markups, I'd be happy to take a look.

Sigh, I've spend last few years so working on the testsuite so that you can compile and run a single test from the checked out repository, we even have howto for that in the README.md.

https://github.com/linux-test-project/ltp/#quick-guide-to-running-the-tests

It shuld be really easy, so can you please give it a try?

Signature · 2 lines

Cyril Hrubis
[email protected]

Yang Shi

2019-03-18 18:12 UTC #3 lore raw

In reply to Cyril Hrubis

Quoted 9 lines · Cyril Hrubis

Hi!
I've started to write tests for mbind() and found out that mbind() does not work as described in manual page in a case that page has been faulted on different node that we are asking it to bind to. Looks like this is working fine on older kernels. On my testing machine with 3.0 mbind() fails correctly with EIO but succeeds unexpectedly on newer kernels such as 4.12.

Quoted 7 lines

What the test does is:
* mmap() private mapping
* fault it
* find out on which node it is faulted on
* mbind() it to a different node with MPOL_BIND and MPOL_MF_STRICT and expects to get EIO

It looks the behavior was changed since v4.0 by the below commit:

6f4576e3687b mempolicy: apply page table walker on queue_pages_range()

The new queue_pages_to_pte_range() doesn't return -EIO anymore. Could you please try the below patch (5.1-rc1 based)?

mm/mempolicy.c +0 -0

index abe7a67..6ba45aa 100644
@@ -521,11 +521,14 @@ static int queue_pages_pte_range(pmd_t *pmd,

unsigned long addr,

                        continue;
                if (!queue_pages_required(page, qp))
                        continue;

- migrate_page_add(page, qp->pagelist, flags);
+ if (flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL))
+ migrate_page_add(page, qp->pagelist, flags);
+ else

+                       break;
        }
        pte_unmap_unlock(pte - 1, ptl);
        cond_resched();

- return 0;
+ return addr != end ? -EIO : 0;
}

static int queue_pages_hugetlb(pte_t *pte, unsigned long hmask,

Yang

Quoted 9 lines

The test code can be seen and compiled from:
https://github.com/metan-ucw/ltp/blob/master/testcases/kernel/syscalls/mbind/mbind02.c
--
Cyril Hrubis
[email protected]

Oscar Salvador

2019-03-19 13:27 UTC #4 lore raw

+CC Kirill

Quoted 21 lines · Yang Shi

mm/mempolicy.c +5 -2

index abe7a67..6ba45aa 100644
@@ -521,11 +521,14 @@ static int queue_pages_pte_range(pmd_t *pmd,
 unsigned long addr,
                        continue;
                if (!queue_pages_required(page, qp))
                        continue;
-               migrate_page_add(page, qp->pagelist, flags);
+               if (flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL))
+                       migrate_page_add(page, qp->pagelist, flags);
+               else
+                       break;
        }
        pte_unmap_unlock(pte - 1, ptl);
        cond_resched();
-       return 0;
+       return addr != end ? -EIO : 0;
 }

Quoted 1 line

static int queue_pages_hugetlb(pte_t *pte, unsigned long hmask,

This alone is not going to help.

The problem is that we do skip the vma early in queue_pages_test_walk() in case MPOL_MF_MOVE and MPOL_MF_MOVE_ALL are not set.

walk_page_range
walk_page_test
queue_pages_test_walk

	...
 	...
	/* queue pages from current vma */
	if (flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL))
		return 0;
	return 1;

So, we skip the vma and keep going.

Before ("77bf45e78050: mempolicy: do not try to queue pages from !vma_migratable()"), queue_pages_test_walk() would not have skipped the vma in case we had MPOL_MF_STRICT or MPOL_MF_MOVE | MPOL_MF_MOVE_ALL.

I did not give it a lot of thought, but it seems to me that we might need to reach queue_pages_to_pte_range() in order to see whether the page is in the required node or not by calling queue_pages_required(), and if it is not, check for MPOL_MF_MOVE | MPOL_MF_MOVE_ALL like the above patch does, so we would be able to return -EIO.
That would imply that we would need to re-add MPOL_MF_STRICT in queue_pages_test_walk().

Signature · 2 lines

Oscar Salvador
SUSE L3

Kirill A. Shutemov

2019-03-19 14:26 UTC #5 lore raw

Quoted 48 lines · Oscar Salvador

+CC Kirill
Quoted 21 lines · Yang Shi
mm/mempolicy.c +5 -2
index abe7a67..6ba45aa 100644
@@ -521,11 +521,14 @@ static int queue_pages_pte_range(pmd_t *pmd,
 unsigned long addr,
                        continue;
                if (!queue_pages_required(page, qp))
                        continue;
-               migrate_page_add(page, qp->pagelist, flags);
+               if (flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL))
+                       migrate_page_add(page, qp->pagelist, flags);
+               else
+                       break;
        }
        pte_unmap_unlock(pte - 1, ptl);
        cond_resched();
-       return 0;
+       return addr != end ? -EIO : 0;
 }
 
Quoted 1 line
static int queue_pages_hugetlb(pte_t *pte, unsigned long hmask,
This alone is not going to help.
The problem is that we do skip the vma early in queue_pages_test_walk() in case MPOL_MF_MOVE and MPOL_MF_MOVE_ALL are not set.
walk_page_range
walk_page_test
queue_pages_test_walk
	...
 	...
	/* queue pages from current vma */
	if (flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL))
		return 0;
	return 1;
So, we skip the vma and keep going.
Before ("77bf45e78050: mempolicy: do not try to queue pages from !vma_migratable()"), queue_pages_test_walk() would not have skipped the vma in case we had MPOL_MF_STRICT or MPOL_MF_MOVE | MPOL_MF_MOVE_ALL.

Quoted 6 lines

I did not give it a lot of thought, but it seems to me that we might need to reach queue_pages_to_pte_range() in order to see whether the page is in the required node or not by calling queue_pages_required(), and if it is not, check for MPOL_MF_MOVE | MPOL_MF_MOVE_ALL like the above patch does, so we would be able to return -EIO.
That would imply that we would need to re-add MPOL_MF_STRICT in queue_pages_test_walk().

That's all sounds reasonable.

We only need to make sure the bug fixed by 77bf45e78050 will not be re-introduced.

Signature · 1 line

 Kirill A. Shutemov

Cyril Hrubis

author

2019-03-19 14:30 UTC #6 lore raw

Hi!

Quoted 4 lines

That's all sounds reasonable.
We only need to make sure the bug fixed by 77bf45e78050 will not be re-introduced.

Well, we can turn the reproducer from that commit into an automated test, that's the only way to make sure bugs are not reintroduced anyways...

Signature · 2 lines

Cyril Hrubis
[email protected]

Oscar Salvador

2019-03-19 14:41 UTC #7 lore raw

In reply to Kirill A. Shutemov

Quoted 4 lines · Kirill A. Shutemov

That's all sounds reasonable.
We only need to make sure the bug fixed by 77bf45e78050 will not be re-introduced.

I gave it a spin with the below patch.
Your testcase works (so the bug is not re-introduced), and we get -EIO when running the ltp test [1].
So unless I am missing something, it should be enough.

mm/mempolicy.c +7 -3

index af171ccb56a2..b192b13460f0 100644
@@ -502,11 +508,16 @@ static int queue_pages_pte_range(pmd_t *pmd, unsigned long addr,
                        continue;
                if (!queue_pages_required(page, qp))
                        continue;
-               migrate_page_add(page, qp->pagelist, flags);
+               if (flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL))
+                       migrate_page_add(page, qp->pagelist, flags);
+               else
+                       break;
        }
        pte_unmap_unlock(pte - 1, ptl);
        cond_resched();
-       return 0;
+       return addr != end ? -EIO : 0;
 }
 
 static int queue_pages_hugetlb(pte_t *pte, unsigned long hmask,

@@ -603,7 +614,8 @@ static int queue_pages_test_walk(unsigned long start, unsigned long end,
        }
 
        /* queue pages from current vma */
-       if (flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL))
+       if ((flags & MPOL_MF_STRICT) ||
+               (flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL))
                return 0;

[1] https://github.com/metan-ucw/ltp/blob/master/testcases/kernel/syscalls/mbind/mbind02.c

Signature · 2 lines

Oscar Salvador
SUSE L3

Kirill A. Shutemov

2019-03-19 14:52 UTC #8 lore raw

Quoted 10 lines · Oscar Salvador

Quoted 4 lines · Kirill A. Shutemov
That's all sounds reasonable.
We only need to make sure the bug fixed by 77bf45e78050 will not be re-introduced.
I gave it a spin with the below patch.
Your testcase works (so the bug is not re-introduced), and we get -EIO when running the ltp test [1].
So unless I am missing something, it should be enough.

Don't we need to bypass !vma_migratable(vma) check in queue_pages_test_walk() for MPOL_MF_STRICT? I mean user still might want to check if all pages are on the right not even the vma is not migratable.

Signature · 1 line

 Kirill A. Shutemov

Oscar Salvador

2019-03-19 15:10 UTC #9 lore raw

Quoted 7 lines · Kirill A. Shutemov

Quoted 6 lines · Oscar Salvador
Quoted 4 lines · Kirill A. Shutemov
That's all sounds reasonable.
We only need to make sure the bug fixed by 77bf45e78050 will not be re-introduced.

Quoted 8 lines

Quoted 4 lines
I gave it a spin with the below patch.
Your testcase works (so the bug is not re-introduced), and we get -EIO when running the ltp test [1].
So unless I am missing something, it should be enough.
Don't we need to bypass !vma_migratable(vma) check in queue_pages_test_walk() for MPOL_MF_STRICT? I mean user still might want to check if all pages are on the right not even the vma is not migratable.

Yeah, I missed that.
Then, I guess that we have to put the check into queue_pages_pte_range as well, and place it right before migrate_page_add().
So, if it is not placed in the node and is not migreatable, we return -EIO.

Signature · 2 lines

Oscar Salvador
SUSE L3

Yang Shi

2019-03-19 16:29 UTC #10 lore raw

Quoted 14 lines · Oscar Salvador

Quoted 12 lines · Kirill A. Shutemov
Quoted 10 lines · Oscar Salvador
Quoted 4 lines · Kirill A. Shutemov
That's all sounds reasonable.
We only need to make sure the bug fixed by 77bf45e78050 will not be re-introduced.
I gave it a spin with the below patch.
Your testcase works (so the bug is not re-introduced), and we get -EIO when running the ltp test [1].
So unless I am missing something, it should be enough.

Quoted 8 lines

Quoted 3 lines
Don't we need to bypass !vma_migratable(vma) check in queue_pages_test_walk() for MPOL_MF_STRICT? I mean user still might want to check if all pages are on the right not even the vma is not migratable.
Yeah, I missed that.
Then, I guess that we have to put the check into queue_pages_pte_range as well, and place it right before migrate_page_add().
So, if it is not placed in the node and is not migreatable, we return -EIO.

Sorry, I didn't see this reply before I replied Kirill's email earlier. Yes, I agree, it should return -EIO too.

Thanks,
Yang

Quoted 4 lines

--
Oscar Salvador
SUSE L3

Yang Shi

2019-03-19 16:25 UTC #11 lore raw

In reply to Kirill A. Shutemov

Quoted 5 lines · Kirill A. Shutemov

Quoted 3 lines · Oscar Salvador
Quoted 2 lines · Kirill A. Shutemov
That's all sounds reasonable.

Quoted 7 lines

Quoted 7 lines
Quoted 2 lines
We only need to make sure the bug fixed by 77bf45e78050 will not be re-introduced.
I gave it a spin with the below patch.
Your testcase works (so the bug is not re-introduced), and we get -EIO when running the ltp test [1].
So unless I am missing something, it should be enough.

Thanks for adding the missing part.

Quoted 4 lines

Don't we need to bypass !vma_migratable(vma) check in queue_pages_test_walk() for MPOL_MF_STRICT? I mean user still might want to check if all pages are on the right not even the vma is not migratable.

I think we need. As long as there is "existing page was already on a node that does not follow the policy" with MPOL_MF_STRICT, it should return -EIO. So, even though the vma is not migratable it should check if the above condition is true or not.

I will wrap all the stuff into a formal patch.

Thanks,
Yang

Quoted 3 lines

--
Kirill A. Shutemov