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

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

[KOSAKI Motohiro]

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 <hugh.dickins@tiscali.co.uk>
Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
---
 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



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

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

[Hugh Dickins]

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 <hugh.dickins@tiscali.co.uk>
> Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>

Acked-by: Hugh Dickins <hugh.dickins@tiscali.co.uk>

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

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

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

[Andrew Morton]

On Tue, 17 Nov 2009 17:39:27 +0900 (JST)
KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> 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>

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

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

[KOSAKI Motohiro]

> On Tue, 17 Nov 2009 17:39:27 +0900 (JST)
> KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> 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



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

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

[Vincent Li]

Hi KOSAKI,

Thank you for the comment, I am still little confused with the last 
sentence.

On Thu, 19 Nov 2009, KOSAKI Motohiro wrote:

> 
> +
> +	/*
> +	 * 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.
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ I am having 
trouble to undestand it. Yeah, I should read more code, but the sentence 
itself make me confused :).

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

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

[KOSAKI Motohiro]

> 
> Hi KOSAKI,
> 
> Thank you for the comment, I am still little confused with the last 
> sentence.
> 
> On Thu, 19 Nov 2009, KOSAKI Motohiro wrote:
> 
> > 
> > +
> > +	/*
> > +	 * 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.
>            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ I am having 
> trouble to undestand it. Yeah, I should read more code, but the sentence 
> itself make me confused :).

Um, this is wrong.
Probably, It should be

	retry to move unevictable lru later.

Do you agree this?



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

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

[Vincent Li]

On Thu, 19 Nov 2009, KOSAKI Motohiro wrote:

> > 
> > Hi KOSAKI,
> > 
> > Thank you for the comment, I am still little confused with the last 
> > sentence.
> > 
> > On Thu, 19 Nov 2009, KOSAKI Motohiro wrote:
> > 
> > > 
> > > +
> > > +	/*
> > > +	 * 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.
> >            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ I am having 
> > trouble to undestand it. Yeah, I should read more code, but the sentence 
> > itself make me confused :).
> 
> Um, this is wrong.
> Probably, It should be
> 
> 	retry to move unevictable lru later.
> 
> Do you agree this?

Ah, let's see if I understand you correctly, if trylock failed, the page 
remain in evictable lru and later vmscan could retry to move the page to 
unevictable lru if the page is actually mlocked? 

Vincent

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

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

[KOSAKI Motohiro]

> 
> 
> On Thu, 19 Nov 2009, KOSAKI Motohiro wrote:
> 
> > > 
> > > Hi KOSAKI,
> > > 
> > > Thank you for the comment, I am still little confused with the last 
> > > sentence.
> > > 
> > > On Thu, 19 Nov 2009, KOSAKI Motohiro wrote:
> > > 
> > > > 
> > > > +
> > > > +	/*
> > > > +	 * 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.
> > >            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ I am having 
> > > trouble to undestand it. Yeah, I should read more code, but the sentence 
> > > itself make me confused :).
> > 
> > Um, this is wrong.
> > Probably, It should be
> > 
> > 	retry to move unevictable lru later.
> > 
> > Do you agree this?
> 
> Ah, let's see if I understand you correctly, if trylock failed, the page 
> remain in evictable lru and later vmscan could retry to move the page to 
> unevictable lru if the page is actually mlocked? 

Ah, your sentence is better. can you please change code itself?


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

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

[Vincent Li]

On Thu, 19 Nov 2009, KOSAKI Motohiro wrote:

> > 
> > 
> > On Thu, 19 Nov 2009, KOSAKI Motohiro wrote:
> > 
> > > > 
> > > > Hi KOSAKI,
> > > > 
> > > > Thank you for the comment, I am still little confused with the last 
> > > > sentence.
> > > > 
> > > > On Thu, 19 Nov 2009, KOSAKI Motohiro wrote:
> > > > 
> > > > > 
> > > > > +
> > > > > +	/*
> > > > > +	 * 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.
> > > >            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ I am having 
> > > > trouble to undestand it. Yeah, I should read more code, but the sentence 
> > > > itself make me confused :).
> > > 
> > > Um, this is wrong.
> > > Probably, It should be
> > > 
> > > 	retry to move unevictable lru later.
> > > 
> > > Do you agree this?
> > 
> > Ah, let's see if I understand you correctly, if trylock failed, the page 
> > remain in evictable lru and later vmscan could retry to move the page to 
> > unevictable lru if the page is actually mlocked? 
> 
> Ah, your sentence is better. can you please change code itself?

You mean I submit the change to last comment sentence? sure, no problem.

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

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

[KOSAKI Motohiro]

> > 
> > 
> > On Thu, 19 Nov 2009, KOSAKI Motohiro wrote:
> > 
> > > > 
> > > > Hi KOSAKI,
> > > > 
> > > > Thank you for the comment, I am still little confused with the last 
> > > > sentence.
> > > > 
> > > > On Thu, 19 Nov 2009, KOSAKI Motohiro wrote:
> > > > 
> > > > > 
> > > > > +
> > > > > +	/*
> > > > > +	 * 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.
> > > >            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ I am having 
> > > > trouble to undestand it. Yeah, I should read more code, but the sentence 
> > > > itself make me confused :).
> > > 
> > > Um, this is wrong.
> > > Probably, It should be
> > > 
> > > 	retry to move unevictable lru later.
> > > 
> > > Do you agree this?
> > 
> > Ah, let's see if I understand you correctly, if trylock failed, the page 
> > remain in evictable lru and later vmscan could retry to move the page to 
> > unevictable lru if the page is actually mlocked? 
> 
> Ah, your sentence is better. can you please change code itself?

Fix is here.

------------------------------------------
Subject: [PATCH] try_to_unmap_one() comment fix

Viencent Li pointed out current comment is wrong. This patch fixes it.

Reported-by: Vincent Li <macli@brc.ubc.ca>
Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
---
 mm/rmap.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/mm/rmap.c b/mm/rmap.c
index f1a9f7d..278cd27 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -875,8 +875,9 @@ out_mlock:
 	 * 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 trylock failed, the page remain in evictable lru and later
+	 * vmscan could retry to move the page to unevictable lru if the
+	 * page is actually mlocked.
 	 */
 	if (down_read_trylock(&vma->vm_mm->mmap_sem)) {
 		if (vma->vm_flags & VM_LOCKED) {
-- 
1.6.5.2



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