Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1590882 > unrolled thread

[RFC 00/11] make try_to_unmap simple

Back to article view | Back to linux.kernel

Contents

  [RFC 00/11] make try_to_unmap simple Minchan Kim <minchan@kernel.org> - 2017-03-02 08:20 +0100
    [RFC 01/11] mm: use SWAP_SUCCESS instead of 0 Minchan Kim <minchan@kernel.org> - 2017-03-02 08:20 +0100
      Re: [RFC 01/11] mm: use SWAP_SUCCESS instead of 0 Anshuman Khandual <khandual@linux.vnet.ibm.com> - 2017-03-02 15:30 +0100
        Re: [RFC 01/11] mm: use SWAP_SUCCESS instead of 0 Minchan Kim <minchan@kernel.org> - 2017-03-03 04:20 +0100
          Re: [RFC 01/11] mm: use SWAP_SUCCESS instead of 0 Anshuman Khandual <khandual@linux.vnet.ibm.com> - 2017-03-06 10:10 +0100
      Re: [RFC 01/11] mm: use SWAP_SUCCESS instead of 0 "Kirill A. Shutemov" <kirill@shutemov.name> - 2017-03-07 16:00 +0100
        Re: [RFC 01/11] mm: use SWAP_SUCCESS instead of 0 Minchan Kim <minchan@kernel.org> - 2017-03-08 07:50 +0100
    [RFC 09/11] mm: make rmap_walk void function Minchan Kim <minchan@kernel.org> - 2017-03-02 08:20 +0100
    [RFC 08/11] mm: make ttu's return boolean Minchan Kim <minchan@kernel.org> - 2017-03-02 08:20 +0100
      Re: [RFC 08/11] mm: make ttu's return boolean John Hubbard <jhubbard@nvidia.com> - 2017-03-08 09:50 +0100
        Re: [RFC 08/11] mm: make ttu's return boolean Minchan Kim <minchan@kernel.org> - 2017-03-09 07:40 +0100
          Re: [RFC 08/11] mm: make ttu's return boolean John Hubbard <jhubbard@nvidia.com> - 2017-03-09 07:50 +0100
    [RFC 06/11] mm: remove SWAP_MLOCK in ttu Minchan Kim <minchan@kernel.org> - 2017-03-02 08:20 +0100
      Re: [RFC 06/11] mm: remove SWAP_MLOCK in ttu Anshuman Khandual <khandual@linux.vnet.ibm.com> - 2017-03-03 15:20 +0100
        Re: [RFC 06/11] mm: remove SWAP_MLOCK in ttu Minchan Kim <minchan@kernel.org> - 2017-03-06 03:20 +0100
          Re: [RFC 06/11] mm: remove SWAP_MLOCK in ttu "Kirill A. Shutemov" <kirill@shutemov.name> - 2017-03-07 19:30 +0100
            Re: [RFC 06/11] mm: remove SWAP_MLOCK in ttu Minchan Kim <minchan@kernel.org> - 2017-03-08 08:00 +0100
    [RFC 04/11] mm: remove SWAP_MLOCK check for SWAP_SUCCESS in ttu Minchan Kim <minchan@kernel.org> - 2017-03-02 08:20 +0100
      Re: [RFC 04/11] mm: remove SWAP_MLOCK check for SWAP_SUCCESS in ttu Anshuman Khandual <khandual@linux.vnet.ibm.com> - 2017-03-02 18:40 +0100
        Re: [RFC 04/11] mm: remove SWAP_MLOCK check for SWAP_SUCCESS in ttu Minchan Kim <minchan@kernel.org> - 2017-03-03 05:30 +0100
      Re: [RFC 04/11] mm: remove SWAP_MLOCK check for SWAP_SUCCESS in ttu "Kirill A. Shutemov" <kirill@shutemov.name> - 2017-03-07 15:30 +0100
        Re: [RFC 04/11] mm: remove SWAP_MLOCK check for SWAP_SUCCESS in ttu Minchan Kim <minchan@kernel.org> - 2017-03-08 07:50 +0100
    [RFC 07/11] mm: remove SWAP_AGAIN in ttu Minchan Kim <minchan@kernel.org> - 2017-03-02 08:20 +0100
      Re: [RFC 07/11] mm: remove SWAP_AGAIN in ttu Anshuman Khandual <khandual@linux.vnet.ibm.com> - 2017-03-03 17:20 +0100
        Re: [RFC 07/11] mm: remove SWAP_AGAIN in ttu Minchan Kim <minchan@kernel.org> - 2017-03-06 03:40 +0100
    [RFC 02/11] mm: remove unncessary ret in page_referenced Minchan Kim <minchan@kernel.org> - 2017-03-02 08:20 +0100
      Re: [RFC 02/11] mm: remove unncessary ret in page_referenced Anshuman Khandual <khandual@linux.vnet.ibm.com> - 2017-03-02 19:20 +0100
        Re: [RFC 02/11] mm: remove unncessary ret in page_referenced Minchan Kim <minchan@kernel.org> - 2017-03-03 04:50 +0100
      Re: [RFC 02/11] mm: remove unncessary ret in page_referenced "Kirill A. Shutemov" <kirill@shutemov.name> - 2017-03-07 15:40 +0100
    [RFC 05/11] mm: make the try_to_munlock void function Minchan Kim <minchan@kernel.org> - 2017-03-02 08:20 +0100
      Re: [RFC 05/11] mm: make the try_to_munlock void function Anshuman Khandual <khandual@linux.vnet.ibm.com> - 2017-03-03 12:50 +0100
        Re: [RFC 05/11] mm: make the try_to_munlock void function Minchan Kim <minchan@kernel.org> - 2017-03-06 03:10 +0100
          Re: [RFC 05/11] mm: make the try_to_munlock void function Anshuman Khandual <khandual@linux.vnet.ibm.com> - 2017-03-06 13:20 +0100
            Re: [RFC 05/11] mm: make the try_to_munlock void function Minchan Kim <minchan@kernel.org> - 2017-03-07 09:30 +0100
              Re: [RFC 05/11] mm: make the try_to_munlock void function Anshuman Khandual <khandual@linux.vnet.ibm.com> - 2017-03-07 15:50 +0100
      Re: [RFC 05/11] mm: make the try_to_munlock void function "Kirill A. Shutemov" <kirill@shutemov.name> - 2017-03-07 16:30 +0100
        Re: [RFC 05/11] mm: make the try_to_munlock void function Minchan Kim <minchan@kernel.org> - 2017-03-08 07:50 +0100
    Re: [RFC 00/11] make try_to_unmap simple Anshuman Khandual <khandual@linux.vnet.ibm.com> - 2017-03-02 15:30 +0100
      Re: [RFC 00/11] make try_to_unmap simple Minchan Kim <minchan@kernel.org> - 2017-03-03 03:50 +0100
        Re: [RFC 00/11] make try_to_unmap simple Anshuman Khandual <khandual@linux.vnet.ibm.com> - 2017-03-03 10:10 +0100

Page 2 of 2 — ← Prev page 1 [2]

#1594279 — Re: [RFC 04/11] mm: remove SWAP_MLOCK check for SWAP_SUCCESS in ttu

On Thu, Mar 02, 2017 at 03:39:18PM +0900, Minchan Kim wrote:
> If the page is mapped and rescue in ttuo, page_mapcount(page) == 0 cannot
> be true so page_mapcount check in ttu is enough to return SWAP_SUCCESS.
> IOW, SWAP_MLOCK check is redundant so remove it.
> 
> Signed-off-by: Minchan Kim <minchan@kernel.org>
> ---
>  mm/rmap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/rmap.c b/mm/rmap.c
> index 3a14013..0a48958 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -1523,7 +1523,7 @@ int try_to_unmap(struct page *page, enum ttu_flags flags)
>  	else
>  		ret = rmap_walk(page, &rwc);
>  
> -	if (ret != SWAP_MLOCK && !page_mapcount(page))
> +	if (!page_mapcount(page))

Hm. I think there's bug in current code.
It should be !total_mapcount(page) otherwise it can be false-positive if
there's THP mapped with PTEs.

And in this case ret != SWAP_MLOCK is helpful to cut down some cost.
Althouth it should be fine to remove it, I guess.

-- 
 Kirill A. Shutemov

[toc] | [prev] | [next] | [standalone]

#1594878 — Re: [RFC 04/11] mm: remove SWAP_MLOCK check for SWAP_SUCCESS in ttu

On Tue, Mar 07, 2017 at 05:26:43PM +0300, Kirill A. Shutemov wrote:
> On Thu, Mar 02, 2017 at 03:39:18PM +0900, Minchan Kim wrote:
> > If the page is mapped and rescue in ttuo, page_mapcount(page) == 0 cannot
> > be true so page_mapcount check in ttu is enough to return SWAP_SUCCESS.
> > IOW, SWAP_MLOCK check is redundant so remove it.
> > 
> > Signed-off-by: Minchan Kim <minchan@kernel.org>
> > ---
> >  mm/rmap.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/mm/rmap.c b/mm/rmap.c
> > index 3a14013..0a48958 100644
> > --- a/mm/rmap.c
> > +++ b/mm/rmap.c
> > @@ -1523,7 +1523,7 @@ int try_to_unmap(struct page *page, enum ttu_flags flags)
> >  	else
> >  		ret = rmap_walk(page, &rwc);
> >  
> > -	if (ret != SWAP_MLOCK && !page_mapcount(page))
> > +	if (!page_mapcount(page))
> 
> Hm. I think there's bug in current code.
> It should be !total_mapcount(page) otherwise it can be false-positive if
> there's THP mapped with PTEs.

Hmm, I lost THP thesedays totally so I can miss something easily.
When I look at that, it seems every pages passed try_to_unmap is already
splited by split split_huge_page_to_list which calls freeze_page which
split pmd. So I guess it's no problem. Right?

Anyway, it's out of scope in this patch so if it's really problem,
I'd like to handle it separately.

One asking:

When we should use total_mapcount instead of page_mapcount?
If total_mapcount has some lengthy description, it would be very helpful
for one who not is faimilar with that.

> 
> And in this case ret != SWAP_MLOCK is helpful to cut down some cost.
> Althouth it should be fine to remove it, I guess.

Sure but be hard to measure it, I think. As well, later patch removes
SWAP_MLOCK.

[toc] | [prev] | [next] | [standalone]

#1590890 — [RFC 07/11] mm: remove SWAP_AGAIN in ttu

In 2002, [1] introduced SWAP_AGAIN.
At that time, ttuo used spin_trylock(&mm->page_table_lock) so it's
really easy to contend and fail to hold a lock so SWAP_AGAIN to keep
LRU status makes sense.

However, now we changed it to mutex-based lock and be able to block
without skip pte so there is a few of small window to return
SWAP_AGAIN so remove SWAP_AGAIN and just return SWAP_FAIL.

[1] c48c43e, minimal rmap
Signed-off-by: Minchan Kim <minchan@kernel.org>
---
 mm/rmap.c   | 11 +++--------
 mm/vmscan.c |  2 --
 2 files changed, 3 insertions(+), 10 deletions(-)

diff --git a/mm/rmap.c b/mm/rmap.c
index 47898a1..da18f21 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -1492,13 +1492,10 @@ static int page_mapcount_is_zero(struct page *page)
  * Return values are:
  *
  * SWAP_SUCCESS	- we succeeded in removing all mappings
- * SWAP_AGAIN	- we missed a mapping, try again later
  * SWAP_FAIL	- the page is unswappable
  */
 int try_to_unmap(struct page *page, enum ttu_flags flags)
 {
-	int ret;
-
 	struct rmap_walk_control rwc = {
 		.rmap_one = try_to_unmap_one,
 		.arg = (void *)flags,
@@ -1518,13 +1515,11 @@ int try_to_unmap(struct page *page, enum ttu_flags flags)
 		rwc.invalid_vma = invalid_migration_vma;
 
 	if (flags & TTU_RMAP_LOCKED)
-		ret = rmap_walk_locked(page, &rwc);
+		rmap_walk_locked(page, &rwc);
 	else
-		ret = rmap_walk(page, &rwc);
+		rmap_walk(page, &rwc);
 
-	if (!page_mapcount(page))
-		ret = SWAP_SUCCESS;
-	return ret;
+	return !page_mapcount(page) ? SWAP_SUCCESS: SWAP_FAIL;
 }
 
 static int page_not_mapped(struct page *page)
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 3cdd270b..170c61f 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -1144,8 +1144,6 @@ static unsigned long shrink_page_list(struct list_head *page_list,
 			case SWAP_FAIL:
 				nr_unmap_fail++;
 				goto activate_locked;
-			case SWAP_AGAIN:
-				goto keep_locked;
 			case SWAP_SUCCESS:
 				; /* try to free the page below */
 			}
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1592102 — Re: [RFC 07/11] mm: remove SWAP_AGAIN in ttu

On 03/02/2017 12:09 PM, Minchan Kim wrote:
> In 2002, [1] introduced SWAP_AGAIN.
> At that time, ttuo used spin_trylock(&mm->page_table_lock) so it's

Small nit: Please expand "ttuo" here. TTU in the first place is also
not very clear but we have that in many places.

> really easy to contend and fail to hold a lock so SWAP_AGAIN to keep
> LRU status makes sense.

Okay.

> 
> However, now we changed it to mutex-based lock and be able to block
> without skip pte so there is a few of small window to return
> SWAP_AGAIN so remove SWAP_AGAIN and just return SWAP_FAIL.

Makes sense.

> 
> [1] c48c43e, minimal rmap
> Signed-off-by: Minchan Kim <minchan@kernel.org>
> ---
>  mm/rmap.c   | 11 +++--------
>  mm/vmscan.c |  2 --
>  2 files changed, 3 insertions(+), 10 deletions(-)
> 
> diff --git a/mm/rmap.c b/mm/rmap.c
> index 47898a1..da18f21 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -1492,13 +1492,10 @@ static int page_mapcount_is_zero(struct page *page)
>   * Return values are:
>   *
>   * SWAP_SUCCESS	- we succeeded in removing all mappings
> - * SWAP_AGAIN	- we missed a mapping, try again later
>   * SWAP_FAIL	- the page is unswappable
>   */
>  int try_to_unmap(struct page *page, enum ttu_flags flags)
>  {
> -	int ret;
> -
>  	struct rmap_walk_control rwc = {
>  		.rmap_one = try_to_unmap_one,
>  		.arg = (void *)flags,
> @@ -1518,13 +1515,11 @@ int try_to_unmap(struct page *page, enum ttu_flags flags)
>  		rwc.invalid_vma = invalid_migration_vma;
>  
>  	if (flags & TTU_RMAP_LOCKED)
> -		ret = rmap_walk_locked(page, &rwc);
> +		rmap_walk_locked(page, &rwc);
>  	else
> -		ret = rmap_walk(page, &rwc);
> +		rmap_walk(page, &rwc);
>  
> -	if (!page_mapcount(page))
> -		ret = SWAP_SUCCESS;
> -	return ret;
> +	return !page_mapcount(page) ? SWAP_SUCCESS: SWAP_FAIL;

Its very simple now. So after the rmap_walk() if page is not mapped any
more return SWAP_SUCCESS otherwise SWAP_FAIL.

[toc] | [prev] | [next] | [standalone]

#1592898 — Re: [RFC 07/11] mm: remove SWAP_AGAIN in ttu

On Fri, Mar 03, 2017 at 06:24:06PM +0530, Anshuman Khandual wrote:
> On 03/02/2017 12:09 PM, Minchan Kim wrote:
> > In 2002, [1] introduced SWAP_AGAIN.
> > At that time, ttuo used spin_trylock(&mm->page_table_lock) so it's
> 
> Small nit: Please expand "ttuo" here. TTU in the first place is also
> not very clear but we have that in many places.

No problem.

[toc] | [prev] | [next] | [standalone]

#1590891 — [RFC 02/11] mm: remove unncessary ret in page_referenced

Anyone doesn't use ret variable. Remove it.

Signed-off-by: Minchan Kim <minchan@kernel.org>
---
 mm/rmap.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/mm/rmap.c b/mm/rmap.c
index bb45712..8076347 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -805,7 +805,6 @@ int page_referenced(struct page *page,
 		    struct mem_cgroup *memcg,
 		    unsigned long *vm_flags)
 {
-	int ret;
 	int we_locked = 0;
 	struct page_referenced_arg pra = {
 		.mapcount = total_mapcount(page),
@@ -839,7 +838,7 @@ int page_referenced(struct page *page,
 		rwc.invalid_vma = invalid_page_referenced_vma;
 	}
 
-	ret = rmap_walk(page, &rwc);
+	rmap_walk(page, &rwc);
 	*vm_flags = pra.vm_flags;
 
 	if (we_locked)
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1591384 — Re: [RFC 02/11] mm: remove unncessary ret in page_referenced

On 03/02/2017 12:09 PM, Minchan Kim wrote:
> Anyone doesn't use ret variable. Remove it.
> 

This change is correct. But not sure how this is related to
try_to_unmap() clean up though.


> Signed-off-by: Minchan Kim <minchan@kernel.org>
> ---
>  mm/rmap.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/mm/rmap.c b/mm/rmap.c
> index bb45712..8076347 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -805,7 +805,6 @@ int page_referenced(struct page *page,
>  		    struct mem_cgroup *memcg,
>  		    unsigned long *vm_flags)
>  {
> -	int ret;
>  	int we_locked = 0;
>  	struct page_referenced_arg pra = {
>  		.mapcount = total_mapcount(page),
> @@ -839,7 +838,7 @@ int page_referenced(struct page *page,
>  		rwc.invalid_vma = invalid_page_referenced_vma;
>  	}
>  
> -	ret = rmap_walk(page, &rwc);
> +	rmap_walk(page, &rwc);
>  	*vm_flags = pra.vm_flags;
>  
>  	if (we_locked)
> 

[toc] | [prev] | [next] | [standalone]

#1591659 — Re: [RFC 02/11] mm: remove unncessary ret in page_referenced

On Thu, Mar 02, 2017 at 08:03:16PM +0530, Anshuman Khandual wrote:
> On 03/02/2017 12:09 PM, Minchan Kim wrote:
> > Anyone doesn't use ret variable. Remove it.
> > 
> 
> This change is correct. But not sure how this is related to
> try_to_unmap() clean up though.

In this patchset, I made rmap_walk void function with upcoming
patch so it's a preparation step for it.

> 
> 
> > Signed-off-by: Minchan Kim <minchan@kernel.org>
> > ---
> >  mm/rmap.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/mm/rmap.c b/mm/rmap.c
> > index bb45712..8076347 100644
> > --- a/mm/rmap.c
> > +++ b/mm/rmap.c
> > @@ -805,7 +805,6 @@ int page_referenced(struct page *page,
> >  		    struct mem_cgroup *memcg,
> >  		    unsigned long *vm_flags)
> >  {
> > -	int ret;
> >  	int we_locked = 0;
> >  	struct page_referenced_arg pra = {
> >  		.mapcount = total_mapcount(page),
> > @@ -839,7 +838,7 @@ int page_referenced(struct page *page,
> >  		rwc.invalid_vma = invalid_page_referenced_vma;
> >  	}
> >  
> > -	ret = rmap_walk(page, &rwc);
> > +	rmap_walk(page, &rwc);
> >  	*vm_flags = pra.vm_flags;
> >  
> >  	if (we_locked)
> > 
> 
> --
> 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>

[toc] | [prev] | [next] | [standalone]

#1594293 — Re: [RFC 02/11] mm: remove unncessary ret in page_referenced

On Thu, Mar 02, 2017 at 03:39:16PM +0900, Minchan Kim wrote:
> Anyone doesn't use ret variable. Remove it.
> 
> Signed-off-by: Minchan Kim <minchan@kernel.org>

Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>

-- 
 Kirill A. Shutemov

[toc] | [prev] | [next] | [standalone]

#1590892 — [RFC 05/11] mm: make the try_to_munlock void function

try_to_munlock returns SWAP_MLOCK if the one of VMAs mapped
the page has VM_LOCKED flag. In that time, VM set PG_mlocked to
the page if the page is not pte-mapped THP which cannot be
mlocked, either.

With that, __munlock_isolated_page can use PageMlocked to check
whether try_to_munlock is successful or not without relying on
try_to_munlock's retval. It helps to make ttu/ttuo simple with
upcoming patches.

Cc: Vlastimil Babka <vbabka@suse.cz>
Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Signed-off-by: Minchan Kim <minchan@kernel.org>
---
 include/linux/rmap.h |  2 +-
 mm/mlock.c           |  6 ++----
 mm/rmap.c            | 16 ++++------------
 3 files changed, 7 insertions(+), 17 deletions(-)

diff --git a/include/linux/rmap.h b/include/linux/rmap.h
index b556eef..1b0cd4c 100644
--- a/include/linux/rmap.h
+++ b/include/linux/rmap.h
@@ -235,7 +235,7 @@ int page_mkclean(struct page *);
  * called in munlock()/munmap() path to check for other vmas holding
  * the page mlocked.
  */
-int try_to_munlock(struct page *);
+void try_to_munlock(struct page *);
 
 void remove_migration_ptes(struct page *old, struct page *new, bool locked);
 
diff --git a/mm/mlock.c b/mm/mlock.c
index cdbed8a..d34a540 100644
--- a/mm/mlock.c
+++ b/mm/mlock.c
@@ -122,17 +122,15 @@ static bool __munlock_isolate_lru_page(struct page *page, bool getpage)
  */
 static void __munlock_isolated_page(struct page *page)
 {
-	int ret = SWAP_AGAIN;
-
 	/*
 	 * Optimization: if the page was mapped just once, that's our mapping
 	 * and we don't need to check all the other vmas.
 	 */
 	if (page_mapcount(page) > 1)
-		ret = try_to_munlock(page);
+		try_to_munlock(page);
 
 	/* Did try_to_unlock() succeed or punt? */
-	if (ret != SWAP_MLOCK)
+	if (!PageMlocked(page))
 		count_vm_event(UNEVICTABLE_PGMUNLOCKED);
 
 	putback_lru_page(page);
diff --git a/mm/rmap.c b/mm/rmap.c
index 0a48958..61ae694 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -1540,18 +1540,10 @@ static int page_not_mapped(struct page *page)
  * Called from munlock code.  Checks all of the VMAs mapping the page
  * to make sure nobody else has this page mlocked. The page will be
  * returned with PG_mlocked cleared if no other vmas have it mlocked.
- *
- * Return values are:
- *
- * SWAP_AGAIN	- no vma is holding page mlocked, or,
- * SWAP_AGAIN	- page mapped in mlocked vma -- couldn't acquire mmap sem
- * SWAP_FAIL	- page cannot be located at present
- * SWAP_MLOCK	- page is now mlocked.
  */
-int try_to_munlock(struct page *page)
-{
-	int ret;
 
+void try_to_munlock(struct page *page)
+{
 	struct rmap_walk_control rwc = {
 		.rmap_one = try_to_unmap_one,
 		.arg = (void *)TTU_MUNLOCK,
@@ -1561,9 +1553,9 @@ int try_to_munlock(struct page *page)
 	};
 
 	VM_BUG_ON_PAGE(!PageLocked(page) || PageLRU(page), page);
+	VM_BUG_ON_PAGE(PageMlocked(page), page);
 
-	ret = rmap_walk(page, &rwc);
-	return ret;
+	rmap_walk(page, &rwc);
 }
 
 void __put_anon_vma(struct anon_vma *anon_vma)
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1591886 — Re: [RFC 05/11] mm: make the try_to_munlock void function

On 03/02/2017 12:09 PM, Minchan Kim wrote:
> try_to_munlock returns SWAP_MLOCK if the one of VMAs mapped
> the page has VM_LOCKED flag. In that time, VM set PG_mlocked to
> the page if the page is not pte-mapped THP which cannot be
> mlocked, either.

Right.

> 
> With that, __munlock_isolated_page can use PageMlocked to check
> whether try_to_munlock is successful or not without relying on
> try_to_munlock's retval. It helps to make ttu/ttuo simple with
> upcoming patches.

Right.

> 
> Cc: Vlastimil Babka <vbabka@suse.cz>
> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Signed-off-by: Minchan Kim <minchan@kernel.org>
> ---
>  include/linux/rmap.h |  2 +-
>  mm/mlock.c           |  6 ++----
>  mm/rmap.c            | 16 ++++------------
>  3 files changed, 7 insertions(+), 17 deletions(-)
> 
> diff --git a/include/linux/rmap.h b/include/linux/rmap.h
> index b556eef..1b0cd4c 100644
> --- a/include/linux/rmap.h
> +++ b/include/linux/rmap.h
> @@ -235,7 +235,7 @@ int page_mkclean(struct page *);
>   * called in munlock()/munmap() path to check for other vmas holding
>   * the page mlocked.
>   */
> -int try_to_munlock(struct page *);
> +void try_to_munlock(struct page *);
>  
>  void remove_migration_ptes(struct page *old, struct page *new, bool locked);
>  
> diff --git a/mm/mlock.c b/mm/mlock.c
> index cdbed8a..d34a540 100644
> --- a/mm/mlock.c
> +++ b/mm/mlock.c
> @@ -122,17 +122,15 @@ static bool __munlock_isolate_lru_page(struct page *page, bool getpage)
>   */
>  static void __munlock_isolated_page(struct page *page)
>  {
> -	int ret = SWAP_AGAIN;
> -
>  	/*
>  	 * Optimization: if the page was mapped just once, that's our mapping
>  	 * and we don't need to check all the other vmas.
>  	 */
>  	if (page_mapcount(page) > 1)
> -		ret = try_to_munlock(page);
> +		try_to_munlock(page);
>  
>  	/* Did try_to_unlock() succeed or punt? */
> -	if (ret != SWAP_MLOCK)
> +	if (!PageMlocked(page))

Checks if the page is still mlocked or not.

>  		count_vm_event(UNEVICTABLE_PGMUNLOCKED);
>  
>  	putback_lru_page(page);
> diff --git a/mm/rmap.c b/mm/rmap.c
> index 0a48958..61ae694 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -1540,18 +1540,10 @@ static int page_not_mapped(struct page *page)
>   * Called from munlock code.  Checks all of the VMAs mapping the page
>   * to make sure nobody else has this page mlocked. The page will be
>   * returned with PG_mlocked cleared if no other vmas have it mlocked.
> - *
> - * Return values are:
> - *
> - * SWAP_AGAIN	- no vma is holding page mlocked, or,
> - * SWAP_AGAIN	- page mapped in mlocked vma -- couldn't acquire mmap sem
> - * SWAP_FAIL	- page cannot be located at present
> - * SWAP_MLOCK	- page is now mlocked.
>   */
> -int try_to_munlock(struct page *page)
> -{
> -	int ret;
>  
> +void try_to_munlock(struct page *page)
> +{
>  	struct rmap_walk_control rwc = {
>  		.rmap_one = try_to_unmap_one,
>  		.arg = (void *)TTU_MUNLOCK,
> @@ -1561,9 +1553,9 @@ int try_to_munlock(struct page *page)
>  	};
>  
>  	VM_BUG_ON_PAGE(!PageLocked(page) || PageLRU(page), page);
> +	VM_BUG_ON_PAGE(PageMlocked(page), page);

We are calling on the page to see if its mlocked from any of it's
mapping VMAs. Then it is a possibility that the page is mlocked
and the above condition is true and we print VM BUG report there.
The point is if its a valid possibility why we have added the
above check ?

[toc] | [prev] | [next] | [standalone]

#1592887 — Re: [RFC 05/11] mm: make the try_to_munlock void function

On Fri, Mar 03, 2017 at 05:13:54PM +0530, Anshuman Khandual wrote:
> On 03/02/2017 12:09 PM, Minchan Kim wrote:
> > try_to_munlock returns SWAP_MLOCK if the one of VMAs mapped
> > the page has VM_LOCKED flag. In that time, VM set PG_mlocked to
> > the page if the page is not pte-mapped THP which cannot be
> > mlocked, either.
> 
> Right.
> 
> > 
> > With that, __munlock_isolated_page can use PageMlocked to check
> > whether try_to_munlock is successful or not without relying on
> > try_to_munlock's retval. It helps to make ttu/ttuo simple with
> > upcoming patches.
> 
> Right.
> 
> > 
> > Cc: Vlastimil Babka <vbabka@suse.cz>
> > Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > Signed-off-by: Minchan Kim <minchan@kernel.org>
> > ---
> >  include/linux/rmap.h |  2 +-
> >  mm/mlock.c           |  6 ++----
> >  mm/rmap.c            | 16 ++++------------
> >  3 files changed, 7 insertions(+), 17 deletions(-)
> > 
> > diff --git a/include/linux/rmap.h b/include/linux/rmap.h
> > index b556eef..1b0cd4c 100644
> > --- a/include/linux/rmap.h
> > +++ b/include/linux/rmap.h
> > @@ -235,7 +235,7 @@ int page_mkclean(struct page *);
> >   * called in munlock()/munmap() path to check for other vmas holding
> >   * the page mlocked.
> >   */
> > -int try_to_munlock(struct page *);
> > +void try_to_munlock(struct page *);
> >  
> >  void remove_migration_ptes(struct page *old, struct page *new, bool locked);
> >  
> > diff --git a/mm/mlock.c b/mm/mlock.c
> > index cdbed8a..d34a540 100644
> > --- a/mm/mlock.c
> > +++ b/mm/mlock.c
> > @@ -122,17 +122,15 @@ static bool __munlock_isolate_lru_page(struct page *page, bool getpage)
> >   */
> >  static void __munlock_isolated_page(struct page *page)
> >  {
> > -	int ret = SWAP_AGAIN;
> > -
> >  	/*
> >  	 * Optimization: if the page was mapped just once, that's our mapping
> >  	 * and we don't need to check all the other vmas.
> >  	 */
> >  	if (page_mapcount(page) > 1)
> > -		ret = try_to_munlock(page);
> > +		try_to_munlock(page);
> >  
> >  	/* Did try_to_unlock() succeed or punt? */
> > -	if (ret != SWAP_MLOCK)
> > +	if (!PageMlocked(page))
> 
> Checks if the page is still mlocked or not.
> 
> >  		count_vm_event(UNEVICTABLE_PGMUNLOCKED);
> >  
> >  	putback_lru_page(page);
> > diff --git a/mm/rmap.c b/mm/rmap.c
> > index 0a48958..61ae694 100644
> > --- a/mm/rmap.c
> > +++ b/mm/rmap.c
> > @@ -1540,18 +1540,10 @@ static int page_not_mapped(struct page *page)
> >   * Called from munlock code.  Checks all of the VMAs mapping the page
> >   * to make sure nobody else has this page mlocked. The page will be
> >   * returned with PG_mlocked cleared if no other vmas have it mlocked.
> > - *
> > - * Return values are:
> > - *
> > - * SWAP_AGAIN	- no vma is holding page mlocked, or,
> > - * SWAP_AGAIN	- page mapped in mlocked vma -- couldn't acquire mmap sem
> > - * SWAP_FAIL	- page cannot be located at present
> > - * SWAP_MLOCK	- page is now mlocked.
> >   */
> > -int try_to_munlock(struct page *page)
> > -{
> > -	int ret;
> >  
> > +void try_to_munlock(struct page *page)
> > +{
> >  	struct rmap_walk_control rwc = {
> >  		.rmap_one = try_to_unmap_one,
> >  		.arg = (void *)TTU_MUNLOCK,
> > @@ -1561,9 +1553,9 @@ int try_to_munlock(struct page *page)
> >  	};
> >  
> >  	VM_BUG_ON_PAGE(!PageLocked(page) || PageLRU(page), page);
> > +	VM_BUG_ON_PAGE(PageMlocked(page), page);
> 
> We are calling on the page to see if its mlocked from any of it's
> mapping VMAs. Then it is a possibility that the page is mlocked
> and the above condition is true and we print VM BUG report there.
> The point is if its a valid possibility why we have added the
> above check ?

If I read code properly,  __munlock_isolated_page calls try_to_munlock
always pass the TestClearPageMlocked page to try_to_munlock.
(e.g., munlock_vma_page and __munlock_pagevec) so I thought
try_to_munlock should be called non-PG_mlocked page and try_to_unmap_one
returns PG_mlocked page once it found a VM_LOCKED VMA for a page.
IOW, non-PG_mlocked page is precondition for try_to_munlock.

[toc] | [prev] | [next] | [standalone]

#1593262 — Re: [RFC 05/11] mm: make the try_to_munlock void function

On 03/06/2017 07:39 AM, Minchan Kim wrote:
> On Fri, Mar 03, 2017 at 05:13:54PM +0530, Anshuman Khandual wrote:
>> On 03/02/2017 12:09 PM, Minchan Kim wrote:
>>> try_to_munlock returns SWAP_MLOCK if the one of VMAs mapped
>>> the page has VM_LOCKED flag. In that time, VM set PG_mlocked to
>>> the page if the page is not pte-mapped THP which cannot be
>>> mlocked, either.
>>
>> Right.
>>
>>>
>>> With that, __munlock_isolated_page can use PageMlocked to check
>>> whether try_to_munlock is successful or not without relying on
>>> try_to_munlock's retval. It helps to make ttu/ttuo simple with
>>> upcoming patches.
>>
>> Right.
>>
>>>
>>> Cc: Vlastimil Babka <vbabka@suse.cz>
>>> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>>> Signed-off-by: Minchan Kim <minchan@kernel.org>
>>> ---
>>>  include/linux/rmap.h |  2 +-
>>>  mm/mlock.c           |  6 ++----
>>>  mm/rmap.c            | 16 ++++------------
>>>  3 files changed, 7 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/include/linux/rmap.h b/include/linux/rmap.h
>>> index b556eef..1b0cd4c 100644
>>> --- a/include/linux/rmap.h
>>> +++ b/include/linux/rmap.h
>>> @@ -235,7 +235,7 @@ int page_mkclean(struct page *);
>>>   * called in munlock()/munmap() path to check for other vmas holding
>>>   * the page mlocked.
>>>   */
>>> -int try_to_munlock(struct page *);
>>> +void try_to_munlock(struct page *);
>>>  
>>>  void remove_migration_ptes(struct page *old, struct page *new, bool locked);
>>>  
>>> diff --git a/mm/mlock.c b/mm/mlock.c
>>> index cdbed8a..d34a540 100644
>>> --- a/mm/mlock.c
>>> +++ b/mm/mlock.c
>>> @@ -122,17 +122,15 @@ static bool __munlock_isolate_lru_page(struct page *page, bool getpage)
>>>   */
>>>  static void __munlock_isolated_page(struct page *page)
>>>  {
>>> -	int ret = SWAP_AGAIN;
>>> -
>>>  	/*
>>>  	 * Optimization: if the page was mapped just once, that's our mapping
>>>  	 * and we don't need to check all the other vmas.
>>>  	 */
>>>  	if (page_mapcount(page) > 1)
>>> -		ret = try_to_munlock(page);
>>> +		try_to_munlock(page);
>>>  
>>>  	/* Did try_to_unlock() succeed or punt? */
>>> -	if (ret != SWAP_MLOCK)
>>> +	if (!PageMlocked(page))
>>
>> Checks if the page is still mlocked or not.
>>
>>>  		count_vm_event(UNEVICTABLE_PGMUNLOCKED);
>>>  
>>>  	putback_lru_page(page);
>>> diff --git a/mm/rmap.c b/mm/rmap.c
>>> index 0a48958..61ae694 100644
>>> --- a/mm/rmap.c
>>> +++ b/mm/rmap.c
>>> @@ -1540,18 +1540,10 @@ static int page_not_mapped(struct page *page)
>>>   * Called from munlock code.  Checks all of the VMAs mapping the page
>>>   * to make sure nobody else has this page mlocked. The page will be
>>>   * returned with PG_mlocked cleared if no other vmas have it mlocked.
>>> - *
>>> - * Return values are:
>>> - *
>>> - * SWAP_AGAIN	- no vma is holding page mlocked, or,
>>> - * SWAP_AGAIN	- page mapped in mlocked vma -- couldn't acquire mmap sem
>>> - * SWAP_FAIL	- page cannot be located at present
>>> - * SWAP_MLOCK	- page is now mlocked.
>>>   */
>>> -int try_to_munlock(struct page *page)
>>> -{
>>> -	int ret;
>>>  
>>> +void try_to_munlock(struct page *page)
>>> +{
>>>  	struct rmap_walk_control rwc = {
>>>  		.rmap_one = try_to_unmap_one,
>>>  		.arg = (void *)TTU_MUNLOCK,
>>> @@ -1561,9 +1553,9 @@ int try_to_munlock(struct page *page)
>>>  	};
>>>  
>>>  	VM_BUG_ON_PAGE(!PageLocked(page) || PageLRU(page), page);
>>> +	VM_BUG_ON_PAGE(PageMlocked(page), page);
>>
>> We are calling on the page to see if its mlocked from any of it's
>> mapping VMAs. Then it is a possibility that the page is mlocked
>> and the above condition is true and we print VM BUG report there.
>> The point is if its a valid possibility why we have added the
>> above check ?
> 
> If I read code properly,  __munlock_isolated_page calls try_to_munlock
> always pass the TestClearPageMlocked page to try_to_munlock.

Right.

> (e.g., munlock_vma_page and __munlock_pagevec) so I thought
> try_to_munlock should be called non-PG_mlocked page and try_to_unmap_one
> returns PG_mlocked page once it found a VM_LOCKED VMA for a page.
> IOW, non-PG_mlocked page is precondition for try_to_munlock.

Okay, I have missed that part. Nonetheless this is a separate issue,
should be part of a different patch ? Not inside these cleanups.

[toc] | [prev] | [next] | [standalone]

#1593980 — Re: [RFC 05/11] mm: make the try_to_munlock void function

Hi Anshuman,

On Mon, Mar 06, 2017 at 03:10:17PM +0530, Anshuman Khandual wrote:
> On 03/06/2017 07:39 AM, Minchan Kim wrote:
> > On Fri, Mar 03, 2017 at 05:13:54PM +0530, Anshuman Khandual wrote:
> >> On 03/02/2017 12:09 PM, Minchan Kim wrote:
> >>> try_to_munlock returns SWAP_MLOCK if the one of VMAs mapped
> >>> the page has VM_LOCKED flag. In that time, VM set PG_mlocked to
> >>> the page if the page is not pte-mapped THP which cannot be
> >>> mlocked, either.
> >>
> >> Right.
> >>
> >>>
> >>> With that, __munlock_isolated_page can use PageMlocked to check
> >>> whether try_to_munlock is successful or not without relying on
> >>> try_to_munlock's retval. It helps to make ttu/ttuo simple with
> >>> upcoming patches.
> >>
> >> Right.
> >>
> >>>
> >>> Cc: Vlastimil Babka <vbabka@suse.cz>
> >>> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> >>> Signed-off-by: Minchan Kim <minchan@kernel.org>
> >>> ---
> >>>  include/linux/rmap.h |  2 +-
> >>>  mm/mlock.c           |  6 ++----
> >>>  mm/rmap.c            | 16 ++++------------
> >>>  3 files changed, 7 insertions(+), 17 deletions(-)
> >>>
> >>> diff --git a/include/linux/rmap.h b/include/linux/rmap.h
> >>> index b556eef..1b0cd4c 100644
> >>> --- a/include/linux/rmap.h
> >>> +++ b/include/linux/rmap.h
> >>> @@ -235,7 +235,7 @@ int page_mkclean(struct page *);
> >>>   * called in munlock()/munmap() path to check for other vmas holding
> >>>   * the page mlocked.
> >>>   */
> >>> -int try_to_munlock(struct page *);
> >>> +void try_to_munlock(struct page *);
> >>>  
> >>>  void remove_migration_ptes(struct page *old, struct page *new, bool locked);
> >>>  
> >>> diff --git a/mm/mlock.c b/mm/mlock.c
> >>> index cdbed8a..d34a540 100644
> >>> --- a/mm/mlock.c
> >>> +++ b/mm/mlock.c
> >>> @@ -122,17 +122,15 @@ static bool __munlock_isolate_lru_page(struct page *page, bool getpage)
> >>>   */
> >>>  static void __munlock_isolated_page(struct page *page)
> >>>  {
> >>> -	int ret = SWAP_AGAIN;
> >>> -
> >>>  	/*
> >>>  	 * Optimization: if the page was mapped just once, that's our mapping
> >>>  	 * and we don't need to check all the other vmas.
> >>>  	 */
> >>>  	if (page_mapcount(page) > 1)
> >>> -		ret = try_to_munlock(page);
> >>> +		try_to_munlock(page);
> >>>  
> >>>  	/* Did try_to_unlock() succeed or punt? */
> >>> -	if (ret != SWAP_MLOCK)
> >>> +	if (!PageMlocked(page))
> >>
> >> Checks if the page is still mlocked or not.
> >>
> >>>  		count_vm_event(UNEVICTABLE_PGMUNLOCKED);
> >>>  
> >>>  	putback_lru_page(page);
> >>> diff --git a/mm/rmap.c b/mm/rmap.c
> >>> index 0a48958..61ae694 100644
> >>> --- a/mm/rmap.c
> >>> +++ b/mm/rmap.c
> >>> @@ -1540,18 +1540,10 @@ static int page_not_mapped(struct page *page)
> >>>   * Called from munlock code.  Checks all of the VMAs mapping the page
> >>>   * to make sure nobody else has this page mlocked. The page will be
> >>>   * returned with PG_mlocked cleared if no other vmas have it mlocked.
> >>> - *
> >>> - * Return values are:
> >>> - *
> >>> - * SWAP_AGAIN	- no vma is holding page mlocked, or,
> >>> - * SWAP_AGAIN	- page mapped in mlocked vma -- couldn't acquire mmap sem
> >>> - * SWAP_FAIL	- page cannot be located at present
> >>> - * SWAP_MLOCK	- page is now mlocked.
> >>>   */
> >>> -int try_to_munlock(struct page *page)
> >>> -{
> >>> -	int ret;
> >>>  
> >>> +void try_to_munlock(struct page *page)
> >>> +{
> >>>  	struct rmap_walk_control rwc = {
> >>>  		.rmap_one = try_to_unmap_one,
> >>>  		.arg = (void *)TTU_MUNLOCK,
> >>> @@ -1561,9 +1553,9 @@ int try_to_munlock(struct page *page)
> >>>  	};
> >>>  
> >>>  	VM_BUG_ON_PAGE(!PageLocked(page) || PageLRU(page), page);
> >>> +	VM_BUG_ON_PAGE(PageMlocked(page), page);
> >>
> >> We are calling on the page to see if its mlocked from any of it's
> >> mapping VMAs. Then it is a possibility that the page is mlocked
> >> and the above condition is true and we print VM BUG report there.
> >> The point is if its a valid possibility why we have added the
> >> above check ?
> > 
> > If I read code properly,  __munlock_isolated_page calls try_to_munlock
> > always pass the TestClearPageMlocked page to try_to_munlock.
> 
> Right.
> 
> > (e.g., munlock_vma_page and __munlock_pagevec) so I thought
> > try_to_munlock should be called non-PG_mlocked page and try_to_unmap_one
> > returns PG_mlocked page once it found a VM_LOCKED VMA for a page.
> > IOW, non-PG_mlocked page is precondition for try_to_munlock.
> 
> Okay, I have missed that part. Nonetheless this is a separate issue,
> should be part of a different patch ? Not inside these cleanups.

If that precondition is not true, this patch changes the behavior
slightly.

        UNEVICTABLE_PGMUNLOCKED count mistmatch compared to old.

I wanted to catch it up. If you still think it's separate issue,
I will do. Please tell me. However, I still think it's no problem
to merge it in this clean up patch.

Thanks.

[toc] | [prev] | [next] | [standalone]

#1594307 — Re: [RFC 05/11] mm: make the try_to_munlock void function

On 03/07/2017 12:20 PM, Minchan Kim wrote:
> Hi Anshuman,
> 
> On Mon, Mar 06, 2017 at 03:10:17PM +0530, Anshuman Khandual wrote:
>> On 03/06/2017 07:39 AM, Minchan Kim wrote:
>>> On Fri, Mar 03, 2017 at 05:13:54PM +0530, Anshuman Khandual wrote:
>>>> On 03/02/2017 12:09 PM, Minchan Kim wrote:
>>>>> try_to_munlock returns SWAP_MLOCK if the one of VMAs mapped
>>>>> the page has VM_LOCKED flag. In that time, VM set PG_mlocked to
>>>>> the page if the page is not pte-mapped THP which cannot be
>>>>> mlocked, either.
>>>> Right.
>>>>
>>>>> With that, __munlock_isolated_page can use PageMlocked to check
>>>>> whether try_to_munlock is successful or not without relying on
>>>>> try_to_munlock's retval. It helps to make ttu/ttuo simple with
>>>>> upcoming patches.
>>>> Right.
>>>>
>>>>> Cc: Vlastimil Babka <vbabka@suse.cz>
>>>>> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>>>>> Signed-off-by: Minchan Kim <minchan@kernel.org>
>>>>> ---
>>>>>  include/linux/rmap.h |  2 +-
>>>>>  mm/mlock.c           |  6 ++----
>>>>>  mm/rmap.c            | 16 ++++------------
>>>>>  3 files changed, 7 insertions(+), 17 deletions(-)
>>>>>
>>>>> diff --git a/include/linux/rmap.h b/include/linux/rmap.h
>>>>> index b556eef..1b0cd4c 100644
>>>>> --- a/include/linux/rmap.h
>>>>> +++ b/include/linux/rmap.h
>>>>> @@ -235,7 +235,7 @@ int page_mkclean(struct page *);
>>>>>   * called in munlock()/munmap() path to check for other vmas holding
>>>>>   * the page mlocked.
>>>>>   */
>>>>> -int try_to_munlock(struct page *);
>>>>> +void try_to_munlock(struct page *);
>>>>>  
>>>>>  void remove_migration_ptes(struct page *old, struct page *new, bool locked);
>>>>>  
>>>>> diff --git a/mm/mlock.c b/mm/mlock.c
>>>>> index cdbed8a..d34a540 100644
>>>>> --- a/mm/mlock.c
>>>>> +++ b/mm/mlock.c
>>>>> @@ -122,17 +122,15 @@ static bool __munlock_isolate_lru_page(struct page *page, bool getpage)
>>>>>   */
>>>>>  static void __munlock_isolated_page(struct page *page)
>>>>>  {
>>>>> -	int ret = SWAP_AGAIN;
>>>>> -
>>>>>  	/*
>>>>>  	 * Optimization: if the page was mapped just once, that's our mapping
>>>>>  	 * and we don't need to check all the other vmas.
>>>>>  	 */
>>>>>  	if (page_mapcount(page) > 1)
>>>>> -		ret = try_to_munlock(page);
>>>>> +		try_to_munlock(page);
>>>>>  
>>>>>  	/* Did try_to_unlock() succeed or punt? */
>>>>> -	if (ret != SWAP_MLOCK)
>>>>> +	if (!PageMlocked(page))
>>>> Checks if the page is still mlocked or not.
>>>>
>>>>>  		count_vm_event(UNEVICTABLE_PGMUNLOCKED);
>>>>>  
>>>>>  	putback_lru_page(page);
>>>>> diff --git a/mm/rmap.c b/mm/rmap.c
>>>>> index 0a48958..61ae694 100644
>>>>> --- a/mm/rmap.c
>>>>> +++ b/mm/rmap.c
>>>>> @@ -1540,18 +1540,10 @@ static int page_not_mapped(struct page *page)
>>>>>   * Called from munlock code.  Checks all of the VMAs mapping the page
>>>>>   * to make sure nobody else has this page mlocked. The page will be
>>>>>   * returned with PG_mlocked cleared if no other vmas have it mlocked.
>>>>> - *
>>>>> - * Return values are:
>>>>> - *
>>>>> - * SWAP_AGAIN	- no vma is holding page mlocked, or,
>>>>> - * SWAP_AGAIN	- page mapped in mlocked vma -- couldn't acquire mmap sem
>>>>> - * SWAP_FAIL	- page cannot be located at present
>>>>> - * SWAP_MLOCK	- page is now mlocked.
>>>>>   */
>>>>> -int try_to_munlock(struct page *page)
>>>>> -{
>>>>> -	int ret;
>>>>>  
>>>>> +void try_to_munlock(struct page *page)
>>>>> +{
>>>>>  	struct rmap_walk_control rwc = {
>>>>>  		.rmap_one = try_to_unmap_one,
>>>>>  		.arg = (void *)TTU_MUNLOCK,
>>>>> @@ -1561,9 +1553,9 @@ int try_to_munlock(struct page *page)
>>>>>  	};
>>>>>  
>>>>>  	VM_BUG_ON_PAGE(!PageLocked(page) || PageLRU(page), page);
>>>>> +	VM_BUG_ON_PAGE(PageMlocked(page), page);
>>>> We are calling on the page to see if its mlocked from any of it's
>>>> mapping VMAs. Then it is a possibility that the page is mlocked
>>>> and the above condition is true and we print VM BUG report there.
>>>> The point is if its a valid possibility why we have added the
>>>> above check ?
>>> If I read code properly,  __munlock_isolated_page calls try_to_munlock
>>> always pass the TestClearPageMlocked page to try_to_munlock.
>> Right.
>>
>>> (e.g., munlock_vma_page and __munlock_pagevec) so I thought
>>> try_to_munlock should be called non-PG_mlocked page and try_to_unmap_one
>>> returns PG_mlocked page once it found a VM_LOCKED VMA for a page.
>>> IOW, non-PG_mlocked page is precondition for try_to_munlock.
>> Okay, I have missed that part. Nonetheless this is a separate issue,
>> should be part of a different patch ? Not inside these cleanups.
> If that precondition is not true, this patch changes the behavior
> slightly.
> 
>         UNEVICTABLE_PGMUNLOCKED count mistmatch compared to old.
> 
> I wanted to catch it up. If you still think it's separate issue,
> I will do. Please tell me. However, I still think it's no problem
> to merge it in this clean up patch.

Got it, its okay. Let this change be part of this patch itself.

[toc] | [prev] | [next] | [standalone]

#1594343 — Re: [RFC 05/11] mm: make the try_to_munlock void function

On Thu, Mar 02, 2017 at 03:39:19PM +0900, Minchan Kim wrote:
> try_to_munlock returns SWAP_MLOCK if the one of VMAs mapped
> the page has VM_LOCKED flag. In that time, VM set PG_mlocked to
> the page if the page is not pte-mapped THP which cannot be
> mlocked, either.
> 
> With that, __munlock_isolated_page can use PageMlocked to check
> whether try_to_munlock is successful or not without relying on
> try_to_munlock's retval. It helps to make ttu/ttuo simple with
> upcoming patches.

I *think* you're correct, but it took time to wrap my head around.
We basically rely on try_to_munlock() never caller for PTE-mapped THP.
And we don't at the moment.

It worth adding something like

	VM_BUG_ON_PAGE(PageCompound(page) && PageDoubleMap(page), page);

into try_to_munlock().

Otherwise looks good to me.

Will free adding my Acked-by once this nit is addressed.

-- 
 Kirill A. Shutemov

[toc] | [prev] | [next] | [standalone]

#1594877 — Re: [RFC 05/11] mm: make the try_to_munlock void function

On Tue, Mar 07, 2017 at 06:17:47PM +0300, Kirill A. Shutemov wrote:
> On Thu, Mar 02, 2017 at 03:39:19PM +0900, Minchan Kim wrote:
> > try_to_munlock returns SWAP_MLOCK if the one of VMAs mapped
> > the page has VM_LOCKED flag. In that time, VM set PG_mlocked to
> > the page if the page is not pte-mapped THP which cannot be
> > mlocked, either.
> > 
> > With that, __munlock_isolated_page can use PageMlocked to check
> > whether try_to_munlock is successful or not without relying on
> > try_to_munlock's retval. It helps to make ttu/ttuo simple with
> > upcoming patches.
> 
> I *think* you're correct, but it took time to wrap my head around.
> We basically rely on try_to_munlock() never caller for PTE-mapped THP.
> And we don't at the moment.
> 
> It worth adding something like
> 
> 	VM_BUG_ON_PAGE(PageCompound(page) && PageDoubleMap(page), page);
> 
> into try_to_munlock().

Agree.

> 
> Otherwise looks good to me.
> 
> Will free adding my Acked-by once this nit is addressed.

Thanks for the review this part, Kirill!

> 
> -- 
>  Kirill A. Shutemov
> 
> --
> 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>

[toc] | [prev] | [next] | [standalone]

#1591132

On 03/02/2017 12:09 PM, Minchan Kim wrote:
> Currently, try_to_unmap returns various return value(SWAP_SUCCESS,
> SWAP_FAIL, SWAP_AGAIN, SWAP_DIRTY and SWAP_MLOCK). When I look into
> that, it's unncessary complicated so this patch aims for cleaning
> it up. Change ttu to boolean function so we can remove SWAP_AGAIN,
> SWAP_DIRTY, SWAP_MLOCK.

It may be a trivial question but apart from being a cleanup does it
help in improving it's callers some way ? Any other benefits ?

[toc] | [prev] | [next] | [standalone]

#1591639

Hi Anshuman,

On Thu, Mar 02, 2017 at 07:52:27PM +0530, Anshuman Khandual wrote:
> On 03/02/2017 12:09 PM, Minchan Kim wrote:
> > Currently, try_to_unmap returns various return value(SWAP_SUCCESS,
> > SWAP_FAIL, SWAP_AGAIN, SWAP_DIRTY and SWAP_MLOCK). When I look into
> > that, it's unncessary complicated so this patch aims for cleaning
> > it up. Change ttu to boolean function so we can remove SWAP_AGAIN,
> > SWAP_DIRTY, SWAP_MLOCK.
> 
> It may be a trivial question but apart from being a cleanup does it
> help in improving it's callers some way ? Any other benefits ?

If you mean some performace, I don't think so. It just aims for cleanup
so caller don't need to think much about return value of try_to_unmap.
What he should consider is just "success/fail". Others will be done in
isolate/putback friends which makes API simple/easy to use.

Thanks.

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

[toc] | [prev] | [next] | [standalone]

#1591762

On 03/03/2017 07:41 AM, Minchan Kim wrote:
> Hi Anshuman,
> 
> On Thu, Mar 02, 2017 at 07:52:27PM +0530, Anshuman Khandual wrote:
>> On 03/02/2017 12:09 PM, Minchan Kim wrote:
>>> Currently, try_to_unmap returns various return value(SWAP_SUCCESS,
>>> SWAP_FAIL, SWAP_AGAIN, SWAP_DIRTY and SWAP_MLOCK). When I look into
>>> that, it's unncessary complicated so this patch aims for cleaning
>>> it up. Change ttu to boolean function so we can remove SWAP_AGAIN,
>>> SWAP_DIRTY, SWAP_MLOCK.
>>
>> It may be a trivial question but apart from being a cleanup does it
>> help in improving it's callers some way ? Any other benefits ?
> 
> If you mean some performace, I don't think so. It just aims for cleanup
> so caller don't need to think much about return value of try_to_unmap.
> What he should consider is just "success/fail". Others will be done in
> isolate/putback friends which makes API simple/easy to use.

Right, got it. Thanks !

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web