2009-11-17 08:39:29

by KOSAKI Motohiro

Subject: [PATCH] [for mmotm-1113] mm: Simplify try_to_unmap_one()

SWAP_MLOCK mean "We marked the page as PG_MLOCK, please move it to
unevictable-lru". So, following code is easy confusable.

if (vma->vm_flags & VM_LOCKED) {
ret = SWAP_MLOCK;
goto out_unmap;
}

Plus, if the VMA doesn't have VM_LOCKED, We don't need to check
the needed of calling mlock_vma_page().

Cc: Hugh Dickins <[email protected]>
Signed-off-by: KOSAKI Motohiro <[email protected]>
---
mm/rmap.c | 26 +++++++++++++-------------
1 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/mm/rmap.c b/mm/rmap.c
index 82e31fb..70dec01 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -779,10 +779,9 @@ static int try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
* skipped over this mm) then we should reactivate it.
*/
if (!(flags & TTU_IGNORE_MLOCK)) {
- if (vma->vm_flags & VM_LOCKED) {
- ret = SWAP_MLOCK;
- goto out_unmap;
- }
+ if (vma->vm_flags & VM_LOCKED)
+ goto out_mlock;
+
if (TTU_ACTION(flags) == TTU_MUNLOCK)
goto out_unmap;
}
@@ -855,18 +854,19 @@ static int try_to_unmap_one(struct page *page, struct vm_area_struct *vma,

out_unmap:
pte_unmap_unlock(pte, ptl);
+out:
+ return ret;

- if (ret == SWAP_MLOCK) {
- ret = SWAP_AGAIN;
- if (down_read_trylock(&vma->vm_mm->mmap_sem)) {
- if (vma->vm_flags & VM_LOCKED) {
- mlock_vma_page(page);
- ret = SWAP_MLOCK;
- }
- up_read(&vma->vm_mm->mmap_sem);
+out_mlock:
+ pte_unmap_unlock(pte, ptl);
+
+ if (down_read_trylock(&vma->vm_mm->mmap_sem)) {
+ if (vma->vm_flags & VM_LOCKED) {
+ mlock_vma_page(page);
+ ret = SWAP_MLOCK;
}
+ up_read(&vma->vm_mm->mmap_sem);
}
-out:
return ret;
}

--
1.6.2.5

2009-11-18 16:35:00

by Hugh Dickins

Subject: Re: [PATCH] [for mmotm-1113] mm: Simplify try_to_unmap_one()

On Tue, 17 Nov 2009, KOSAKI Motohiro wrote:

> SWAP_MLOCK mean "We marked the page as PG_MLOCK, please move it to
> unevictable-lru". So, following code is easy confusable.
>
> if (vma->vm_flags & VM_LOCKED) {
> ret = SWAP_MLOCK;
> goto out_unmap;
> }
>
> Plus, if the VMA doesn't have VM_LOCKED, We don't need to check
> the needed of calling mlock_vma_page().
>
> Cc: Hugh Dickins <[email protected]>
> Signed-off-by: KOSAKI Motohiro <[email protected]>

Acked-by: Hugh Dickins <[email protected]>

> ---
> mm/rmap.c | 26 +++++++++++++-------------
> 1 files changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/mm/rmap.c b/mm/rmap.c
> index 82e31fb..70dec01 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -779,10 +779,9 @@ static int try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
> * skipped over this mm) then we should reactivate it.
> */
> if (!(flags & TTU_IGNORE_MLOCK)) {
> - if (vma->vm_flags & VM_LOCKED) {
> - ret = SWAP_MLOCK;
> - goto out_unmap;
> - }
> + if (vma->vm_flags & VM_LOCKED)
> + goto out_mlock;
> +
> if (TTU_ACTION(flags) == TTU_MUNLOCK)
> goto out_unmap;
> }
> @@ -855,18 +854,19 @@ static int try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
>
> out_unmap:
> pte_unmap_unlock(pte, ptl);
> +out:
> + return ret;
>
> - if (ret == SWAP_MLOCK) {
> - ret = SWAP_AGAIN;
> - if (down_read_trylock(&vma->vm_mm->mmap_sem)) {
> - if (vma->vm_flags & VM_LOCKED) {
> - mlock_vma_page(page);
> - ret = SWAP_MLOCK;
> - }
> - up_read(&vma->vm_mm->mmap_sem);
> +out_mlock:
> + pte_unmap_unlock(pte, ptl);
> +
> + if (down_read_trylock(&vma->vm_mm->mmap_sem)) {
> + if (vma->vm_flags & VM_LOCKED) {
> + mlock_vma_page(page);
> + ret = SWAP_MLOCK;
> }
> + up_read(&vma->vm_mm->mmap_sem);
> }
> -out:
> return ret;
> }
>
> --
> 1.6.2.5

2009-11-18 23:18:09

by Andrew Morton

Subject: Re: [PATCH] [for mmotm-1113] mm: Simplify try_to_unmap_one()

On Tue, 17 Nov 2009 17:39:27 +0900 (JST)
KOSAKI Motohiro <[email protected]> wrote:

> +out_mlock:
> + pte_unmap_unlock(pte, ptl);
> +
> + if (down_read_trylock(&vma->vm_mm->mmap_sem)) {
> + if (vma->vm_flags & VM_LOCKED) {
> + mlock_vma_page(page);
> + ret = SWAP_MLOCK;
> }
> + up_read(&vma->vm_mm->mmap_sem);

It's somewhat unobvious why we're using a trylock here. Ranking versus
lock_page(), perhaps?

In general I think a trylock should have an associated comment which explains

a) why it is being used at this site and

b) what happens when the trylock fails - why this isn't a
bug, how the kernel recovers from the inconsistency, what its
overall effect is, etc.

<wonders why we need to take mmap_sem here at all>

2009-11-19 01:23:16

by KOSAKI Motohiro

Subject: Re: [PATCH] [for mmotm-1113] mm: Simplify try_to_unmap_one()

> On Tue, 17 Nov 2009 17:39:27 +0900 (JST)
> KOSAKI Motohiro <[email protected]> wrote:
>
> > +out_mlock:
> > + pte_unmap_unlock(pte, ptl);
> > +
> > + if (down_read_trylock(&vma->vm_mm->mmap_sem)) {
> > + if (vma->vm_flags & VM_LOCKED) {
> > + mlock_vma_page(page);
> > + ret = SWAP_MLOCK;
> > }
> > + up_read(&vma->vm_mm->mmap_sem);
>
> It's somewhat unobvious why we're using a trylock here. Ranking versus
> lock_page(), perhaps?
>
> In general I think a trylock should have an associated comment which explains
>
> a) why it is being used at this site and
>
> b) what happens when the trylock fails - why this isn't a
> bug, how the kernel recovers from the inconsistency, what its
> overall effect is, etc.
>
> <wonders why we need to take mmap_sem here at all>

This mmap_sem is needed certainenaly. Following comment is sufficient?


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

diff --git a/mm/rmap.c b/mm/rmap.c
index 70dec01..b1c9342 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -860,6 +860,14 @@ out:
out_mlock:
pte_unmap_unlock(pte, ptl);

+
+ /*
+ * We need mmap_sem locking, Otherwise VM_LOCKED check makes
+ * unstable result and race. Plus, We can't wait here because
+ * we now hold anon_vma->lock or mapping->i_mmap_lock.
+ * If trylock failed, The page remain evictable lru and
+ * retry to more unevictable lru by later vmscan.
+ */
if (down_read_trylock(&vma->vm_mm->mmap_sem)) {
if (vma->vm_flags & VM_LOCKED) {
mlock_vma_page(page);
--
1.6.2.5