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 1 of 2  [1] 2  Next page →

#1590882 — [RFC 00/11] make try_to_unmap simple

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.

This patchset is based on v4.10-mmots-2017-02-28-17-33.

Minchan Kim (11):
  mm: use SWAP_SUCCESS instead of 0
  mm: remove unncessary ret in page_referenced
  mm: remove SWAP_DIRTY in ttu
  mm: remove SWAP_MLOCK check for SWAP_SUCCESS in ttu
  mm: make the try_to_munlock void function
  mm: remove SWAP_MLOCK in ttu
  mm: remove SWAP_AGAIN in ttu
  mm: make ttu's return boolean
  mm: make rmap_walk void function
  mm: make rmap_one boolean function
  mm: remove SWAP_[SUCCESS|AGAIN|FAIL]

 include/linux/ksm.h  |  5 ++-
 include/linux/rmap.h | 21 ++++--------
 mm/huge_memory.c     |  4 +--
 mm/ksm.c             | 16 ++++-----
 mm/memory-failure.c  | 22 ++++++------
 mm/migrate.c         |  4 +--
 mm/mlock.c           |  6 ++--
 mm/page_idle.c       |  4 +--
 mm/rmap.c            | 97 ++++++++++++++++++++--------------------------------
 mm/vmscan.c          | 26 +++-----------
 10 files changed, 73 insertions(+), 132 deletions(-)

-- 
2.7.4

[toc] | [next] | [standalone]

#1590883 — [RFC 01/11] mm: use SWAP_SUCCESS instead of 0

SWAP_SUCCESS defined value 0 can be changed always so don't rely on
it. Instead, use explict macro.

Cc: Kirill A. Shutemov <kirill@shutemov.name>
Signed-off-by: Minchan Kim <minchan@kernel.org>
---
 mm/huge_memory.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 092cc5c..fe2ccd4 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -2114,7 +2114,7 @@ static void freeze_page(struct page *page)
 		ttu_flags |= TTU_MIGRATION;
 
 	ret = try_to_unmap(page, ttu_flags);
-	VM_BUG_ON_PAGE(ret, page);
+	VM_BUG_ON_PAGE(ret != SWAP_SUCCESS, page);
 }
 
 static void unfreeze_page(struct page *page)
-- 
2.7.4

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

#1591127 — Re: [RFC 01/11] mm: use SWAP_SUCCESS instead of 0

On 03/02/2017 12:09 PM, Minchan Kim wrote:
> SWAP_SUCCESS defined value 0 can be changed always so don't rely on
> it. Instead, use explict macro.

Right. But should not we move the changes to the callers last in the
patch series after doing the cleanup to the try_to_unmap() function
as intended first.

> > Cc: Kirill A. Shutemov <kirill@shutemov.name>
> Signed-off-by: Minchan Kim <minchan@kernel.org>
> ---
>  mm/huge_memory.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 092cc5c..fe2ccd4 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -2114,7 +2114,7 @@ static void freeze_page(struct page *page)
>  		ttu_flags |= TTU_MIGRATION;
>  
>  	ret = try_to_unmap(page, ttu_flags);
> -	VM_BUG_ON_PAGE(ret, page);
> +	VM_BUG_ON_PAGE(ret != SWAP_SUCCESS, page);
>  }
>  
>  static void unfreeze_page(struct page *page)
> 

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

#1591646 — Re: [RFC 01/11] mm: use SWAP_SUCCESS instead of 0

On Thu, Mar 02, 2017 at 07:57:10PM +0530, Anshuman Khandual wrote:
> On 03/02/2017 12:09 PM, Minchan Kim wrote:
> > SWAP_SUCCESS defined value 0 can be changed always so don't rely on
> > it. Instead, use explict macro.
> 
> Right. But should not we move the changes to the callers last in the
> patch series after doing the cleanup to the try_to_unmap() function
> as intended first.

I don't understand what you are pointing out. Could you elaborate it
a bit?

Thanks.

> 
> > > Cc: Kirill A. Shutemov <kirill@shutemov.name>
> > Signed-off-by: Minchan Kim <minchan@kernel.org>
> > ---
> >  mm/huge_memory.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> > index 092cc5c..fe2ccd4 100644
> > --- a/mm/huge_memory.c
> > +++ b/mm/huge_memory.c
> > @@ -2114,7 +2114,7 @@ static void freeze_page(struct page *page)
> >  		ttu_flags |= TTU_MIGRATION;
> >  
> >  	ret = try_to_unmap(page, ttu_flags);
> > -	VM_BUG_ON_PAGE(ret, page);
> > +	VM_BUG_ON_PAGE(ret != SWAP_SUCCESS, page);
> >  }
> >  
> >  static void unfreeze_page(struct page *page)
> > 
> 
> --
> 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]

#1593002 — Re: [RFC 01/11] mm: use SWAP_SUCCESS instead of 0

On 03/03/2017 08:31 AM, Minchan Kim wrote:
> On Thu, Mar 02, 2017 at 07:57:10PM +0530, Anshuman Khandual wrote:
>> On 03/02/2017 12:09 PM, Minchan Kim wrote:
>>> SWAP_SUCCESS defined value 0 can be changed always so don't rely on
>>> it. Instead, use explict macro.
>>
>> Right. But should not we move the changes to the callers last in the
>> patch series after doing the cleanup to the try_to_unmap() function
>> as intended first.
> 
> I don't understand what you are pointing out. Could you elaborate it
> a bit?

I was just referring to the order of this patch in the series and
thinking if it would have been better if this patch would be at a
later stage in the series. But I guess its okay as we are any way
dropping off SWAP_FAIL, SWAP_SUCCESS etc in the end.

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

#1594316 — Re: [RFC 01/11] mm: use SWAP_SUCCESS instead of 0

On Thu, Mar 02, 2017 at 03:39:15PM +0900, Minchan Kim wrote:
> SWAP_SUCCESS defined value 0 can be changed always so don't rely on
> it. Instead, use explict macro.

I'm okay with this as long as it's prepartion for something meaningful.
0 as success is widely used. I don't think replacing it's with macro here
has value on its own.

-- 
 Kirill A. Shutemov

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

#1594876 — Re: [RFC 01/11] mm: use SWAP_SUCCESS instead of 0

Hi Kirill,

On Tue, Mar 07, 2017 at 05:19:33PM +0300, Kirill A. Shutemov wrote:
> On Thu, Mar 02, 2017 at 03:39:15PM +0900, Minchan Kim wrote:
> > SWAP_SUCCESS defined value 0 can be changed always so don't rely on
> > it. Instead, use explict macro.
> 
> I'm okay with this as long as it's prepartion for something meaningful.
> 0 as success is widely used. I don't think replacing it's with macro here
> has value on its own.

It's the prepartion for making try_to_unmap return bool type but strictly
speaking, it's not necessary but I wanted to replace it with SWAP_SUCCESS
in this chance because it has several *defined* return type so it would
make it clear if we use one of those defiend type, IMO.
However, my thumb rule is to keep author/maintainer's credit for trivial
case and it seems you don't like so I will drop in next spin.

Thanks.


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

#1590885 — [RFC 09/11] mm: make rmap_walk void function

There is no user of return value from rmap_walk friend so this
patch makes them void function.

Signed-off-by: Minchan Kim <minchan@kernel.org>
---
 include/linux/ksm.h  |  5 ++---
 include/linux/rmap.h |  4 ++--
 mm/ksm.c             | 16 ++++++----------
 mm/rmap.c            | 32 +++++++++++++-------------------
 4 files changed, 23 insertions(+), 34 deletions(-)

diff --git a/include/linux/ksm.h b/include/linux/ksm.h
index 481c8c4..317ad0b 100644
--- a/include/linux/ksm.h
+++ b/include/linux/ksm.h
@@ -60,7 +60,7 @@ static inline void set_page_stable_node(struct page *page,
 struct page *ksm_might_need_to_copy(struct page *page,
 			struct vm_area_struct *vma, unsigned long address);
 
-int rmap_walk_ksm(struct page *page, struct rmap_walk_control *rwc);
+void rmap_walk_ksm(struct page *page, struct rmap_walk_control *rwc);
 void ksm_migrate_page(struct page *newpage, struct page *oldpage);
 
 #else  /* !CONFIG_KSM */
@@ -93,10 +93,9 @@ static inline int page_referenced_ksm(struct page *page,
 	return 0;
 }
 
-static inline int rmap_walk_ksm(struct page *page,
+static inline void rmap_walk_ksm(struct page *page,
 			struct rmap_walk_control *rwc)
 {
-	return 0;
 }
 
 static inline void ksm_migrate_page(struct page *newpage, struct page *oldpage)
diff --git a/include/linux/rmap.h b/include/linux/rmap.h
index 6028c38..1d7d457c 100644
--- a/include/linux/rmap.h
+++ b/include/linux/rmap.h
@@ -264,8 +264,8 @@ struct rmap_walk_control {
 	bool (*invalid_vma)(struct vm_area_struct *vma, void *arg);
 };
 
-int rmap_walk(struct page *page, struct rmap_walk_control *rwc);
-int rmap_walk_locked(struct page *page, struct rmap_walk_control *rwc);
+void rmap_walk(struct page *page, struct rmap_walk_control *rwc);
+void rmap_walk_locked(struct page *page, struct rmap_walk_control *rwc);
 
 #else	/* !CONFIG_MMU */
 
diff --git a/mm/ksm.c b/mm/ksm.c
index 520e4c3..68f8820 100644
--- a/mm/ksm.c
+++ b/mm/ksm.c
@@ -1931,11 +1931,10 @@ struct page *ksm_might_need_to_copy(struct page *page,
 	return new_page;
 }
 
-int rmap_walk_ksm(struct page *page, struct rmap_walk_control *rwc)
+void rmap_walk_ksm(struct page *page, struct rmap_walk_control *rwc)
 {
 	struct stable_node *stable_node;
 	struct rmap_item *rmap_item;
-	int ret = SWAP_AGAIN;
 	int search_new_forks = 0;
 
 	VM_BUG_ON_PAGE(!PageKsm(page), page);
@@ -1948,7 +1947,7 @@ int rmap_walk_ksm(struct page *page, struct rmap_walk_control *rwc)
 
 	stable_node = page_stable_node(page);
 	if (!stable_node)
-		return ret;
+		return;
 again:
 	hlist_for_each_entry(rmap_item, &stable_node->hlist, hlist) {
 		struct anon_vma *anon_vma = rmap_item->anon_vma;
@@ -1976,23 +1975,20 @@ int rmap_walk_ksm(struct page *page, struct rmap_walk_control *rwc)
 			if (rwc->invalid_vma && rwc->invalid_vma(vma, rwc->arg))
 				continue;
 
-			ret = rwc->rmap_one(page, vma,
-					rmap_item->address, rwc->arg);
-			if (ret != SWAP_AGAIN) {
+			if (SWAP_AGAIN != rwc->rmap_one(page, vma,
+					rmap_item->address, rwc->arg)) {
 				anon_vma_unlock_read(anon_vma);
-				goto out;
+				return;
 			}
 			if (rwc->done && rwc->done(page)) {
 				anon_vma_unlock_read(anon_vma);
-				goto out;
+				return;
 			}
 		}
 		anon_vma_unlock_read(anon_vma);
 	}
 	if (!search_new_forks++)
 		goto again;
-out:
-	return ret;
 }
 
 #ifdef CONFIG_MIGRATION
diff --git a/mm/rmap.c b/mm/rmap.c
index 01f7832..08e4f81 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -1595,13 +1595,12 @@ static struct anon_vma *rmap_walk_anon_lock(struct page *page,
  * vm_flags for that VMA.  That should be OK, because that vma shouldn't be
  * LOCKED.
  */
-static int rmap_walk_anon(struct page *page, struct rmap_walk_control *rwc,
+static void rmap_walk_anon(struct page *page, struct rmap_walk_control *rwc,
 		bool locked)
 {
 	struct anon_vma *anon_vma;
 	pgoff_t pgoff_start, pgoff_end;
 	struct anon_vma_chain *avc;
-	int ret = SWAP_AGAIN;
 
 	if (locked) {
 		anon_vma = page_anon_vma(page);
@@ -1611,7 +1610,7 @@ static int rmap_walk_anon(struct page *page, struct rmap_walk_control *rwc,
 		anon_vma = rmap_walk_anon_lock(page, rwc);
 	}
 	if (!anon_vma)
-		return ret;
+		return;
 
 	pgoff_start = page_to_pgoff(page);
 	pgoff_end = pgoff_start + hpage_nr_pages(page) - 1;
@@ -1625,8 +1624,7 @@ static int rmap_walk_anon(struct page *page, struct rmap_walk_control *rwc,
 		if (rwc->invalid_vma && rwc->invalid_vma(vma, rwc->arg))
 			continue;
 
-		ret = rwc->rmap_one(page, vma, address, rwc->arg);
-		if (ret != SWAP_AGAIN)
+		if (SWAP_AGAIN != rwc->rmap_one(page, vma, address, rwc->arg))
 			break;
 		if (rwc->done && rwc->done(page))
 			break;
@@ -1634,7 +1632,6 @@ static int rmap_walk_anon(struct page *page, struct rmap_walk_control *rwc,
 
 	if (!locked)
 		anon_vma_unlock_read(anon_vma);
-	return ret;
 }
 
 /*
@@ -1650,13 +1647,12 @@ static int rmap_walk_anon(struct page *page, struct rmap_walk_control *rwc,
  * vm_flags for that VMA.  That should be OK, because that vma shouldn't be
  * LOCKED.
  */
-static int rmap_walk_file(struct page *page, struct rmap_walk_control *rwc,
+static void rmap_walk_file(struct page *page, struct rmap_walk_control *rwc,
 		bool locked)
 {
 	struct address_space *mapping = page_mapping(page);
 	pgoff_t pgoff_start, pgoff_end;
 	struct vm_area_struct *vma;
-	int ret = SWAP_AGAIN;
 
 	/*
 	 * The page lock not only makes sure that page->mapping cannot
@@ -1667,7 +1663,7 @@ static int rmap_walk_file(struct page *page, struct rmap_walk_control *rwc,
 	VM_BUG_ON_PAGE(!PageLocked(page), page);
 
 	if (!mapping)
-		return ret;
+		return;
 
 	pgoff_start = page_to_pgoff(page);
 	pgoff_end = pgoff_start + hpage_nr_pages(page) - 1;
@@ -1682,8 +1678,7 @@ static int rmap_walk_file(struct page *page, struct rmap_walk_control *rwc,
 		if (rwc->invalid_vma && rwc->invalid_vma(vma, rwc->arg))
 			continue;
 
-		ret = rwc->rmap_one(page, vma, address, rwc->arg);
-		if (ret != SWAP_AGAIN)
+		if (SWAP_AGAIN != rwc->rmap_one(page, vma, address, rwc->arg))
 			goto done;
 		if (rwc->done && rwc->done(page))
 			goto done;
@@ -1692,28 +1687,27 @@ static int rmap_walk_file(struct page *page, struct rmap_walk_control *rwc,
 done:
 	if (!locked)
 		i_mmap_unlock_read(mapping);
-	return ret;
 }
 
-int rmap_walk(struct page *page, struct rmap_walk_control *rwc)
+void rmap_walk(struct page *page, struct rmap_walk_control *rwc)
 {
 	if (unlikely(PageKsm(page)))
-		return rmap_walk_ksm(page, rwc);
+		rmap_walk_ksm(page, rwc);
 	else if (PageAnon(page))
-		return rmap_walk_anon(page, rwc, false);
+		rmap_walk_anon(page, rwc, false);
 	else
-		return rmap_walk_file(page, rwc, false);
+		rmap_walk_file(page, rwc, false);
 }
 
 /* Like rmap_walk, but caller holds relevant rmap lock */
-int rmap_walk_locked(struct page *page, struct rmap_walk_control *rwc)
+void rmap_walk_locked(struct page *page, struct rmap_walk_control *rwc)
 {
 	/* no ksm support for now */
 	VM_BUG_ON_PAGE(PageKsm(page), page);
 	if (PageAnon(page))
-		return rmap_walk_anon(page, rwc, true);
+		rmap_walk_anon(page, rwc, true);
 	else
-		return rmap_walk_file(page, rwc, true);
+		rmap_walk_file(page, rwc, true);
 }
 
 #ifdef CONFIG_HUGETLB_PAGE
-- 
2.7.4

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

#1590886 — [RFC 08/11] mm: make ttu's return boolean

try_to_unmap returns SWAP_SUCCESS or SWAP_FAIL so it's suitable for
boolean return. This patch changes it.

Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
Signed-off-by: Minchan Kim <minchan@kernel.org>
---
 include/linux/rmap.h |  4 ++--
 mm/huge_memory.c     |  4 ++--
 mm/memory-failure.c  | 22 ++++++++++------------
 mm/rmap.c            |  8 +++-----
 mm/vmscan.c          |  7 +------
 5 files changed, 18 insertions(+), 27 deletions(-)

diff --git a/include/linux/rmap.h b/include/linux/rmap.h
index 3630d4d..6028c38 100644
--- a/include/linux/rmap.h
+++ b/include/linux/rmap.h
@@ -191,7 +191,7 @@ static inline void page_dup_rmap(struct page *page, bool compound)
 int page_referenced(struct page *, int is_locked,
 			struct mem_cgroup *memcg, unsigned long *vm_flags);
 
-int try_to_unmap(struct page *, enum ttu_flags flags);
+bool try_to_unmap(struct page *, enum ttu_flags flags);
 
 /* Avoid racy checks */
 #define PVMW_SYNC		(1 << 0)
@@ -281,7 +281,7 @@ static inline int page_referenced(struct page *page, int is_locked,
 	return 0;
 }
 
-#define try_to_unmap(page, refs) SWAP_FAIL
+#define try_to_unmap(page, refs) false
 
 static inline int page_mkclean(struct page *page)
 {
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index fe2ccd4..79ea769 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -2106,7 +2106,7 @@ static void freeze_page(struct page *page)
 {
 	enum ttu_flags ttu_flags = TTU_IGNORE_MLOCK | TTU_IGNORE_ACCESS |
 		TTU_RMAP_LOCKED | TTU_SPLIT_HUGE_PMD;
-	int ret;
+	bool ret;
 
 	VM_BUG_ON_PAGE(!PageHead(page), page);
 
@@ -2114,7 +2114,7 @@ static void freeze_page(struct page *page)
 		ttu_flags |= TTU_MIGRATION;
 
 	ret = try_to_unmap(page, ttu_flags);
-	VM_BUG_ON_PAGE(ret != SWAP_SUCCESS, page);
+	VM_BUG_ON_PAGE(!ret, page);
 }
 
 static void unfreeze_page(struct page *page)
diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index b78d080..75fcbd8 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -321,7 +321,7 @@ static void add_to_kill(struct task_struct *tsk, struct page *p,
  * wrong earlier.
  */
 static void kill_procs(struct list_head *to_kill, int forcekill, int trapno,
-			  int fail, struct page *page, unsigned long pfn,
+			  bool fail, struct page *page, unsigned long pfn,
 			  int flags)
 {
 	struct to_kill *tk, *next;
@@ -903,13 +903,13 @@ EXPORT_SYMBOL_GPL(get_hwpoison_page);
  * Do all that is necessary to remove user space mappings. Unmap
  * the pages and send SIGBUS to the processes if the data was dirty.
  */
-static int hwpoison_user_mappings(struct page *p, unsigned long pfn,
+static bool hwpoison_user_mappings(struct page *p, unsigned long pfn,
 				  int trapno, int flags, struct page **hpagep)
 {
 	enum ttu_flags ttu = TTU_IGNORE_MLOCK | TTU_IGNORE_ACCESS;
 	struct address_space *mapping;
 	LIST_HEAD(tokill);
-	int ret;
+	bool ret;
 	int kill = 1, forcekill;
 	struct page *hpage = *hpagep;
 
@@ -918,20 +918,20 @@ static int hwpoison_user_mappings(struct page *p, unsigned long pfn,
 	 * other types of pages.
 	 */
 	if (PageReserved(p) || PageSlab(p))
-		return SWAP_SUCCESS;
+		return true;
 	if (!(PageLRU(hpage) || PageHuge(p)))
-		return SWAP_SUCCESS;
+		return true;
 
 	/*
 	 * This check implies we don't kill processes if their pages
 	 * are in the swap cache early. Those are always late kills.
 	 */
 	if (!page_mapped(hpage))
-		return SWAP_SUCCESS;
+		return true;
 
 	if (PageKsm(p)) {
 		pr_err("Memory failure: %#lx: can't handle KSM pages.\n", pfn);
-		return SWAP_FAIL;
+		return false;
 	}
 
 	if (PageSwapCache(p)) {
@@ -971,7 +971,7 @@ static int hwpoison_user_mappings(struct page *p, unsigned long pfn,
 		collect_procs(hpage, &tokill, flags & MF_ACTION_REQUIRED);
 
 	ret = try_to_unmap(hpage, ttu);
-	if (ret != SWAP_SUCCESS)
+	if (!ret)
 		pr_err("Memory failure: %#lx: failed to unmap page (mapcount=%d)\n",
 		       pfn, page_mapcount(hpage));
 
@@ -986,8 +986,7 @@ static int hwpoison_user_mappings(struct page *p, unsigned long pfn,
 	 * any accesses to the poisoned memory.
 	 */
 	forcekill = PageDirty(hpage) || (flags & MF_MUST_KILL);
-	kill_procs(&tokill, forcekill, trapno,
-		      ret != SWAP_SUCCESS, p, pfn, flags);
+	kill_procs(&tokill, forcekill, trapno, !ret , p, pfn, flags);
 
 	return ret;
 }
@@ -1229,8 +1228,7 @@ int memory_failure(unsigned long pfn, int trapno, int flags)
 	 * When the raw error page is thp tail page, hpage points to the raw
 	 * page after thp split.
 	 */
-	if (hwpoison_user_mappings(p, pfn, trapno, flags, &hpage)
-	    != SWAP_SUCCESS) {
+	if (!hwpoison_user_mappings(p, pfn, trapno, flags, &hpage)) {
 		action_result(pfn, MF_MSG_UNMAP_FAILED, MF_IGNORED);
 		res = -EBUSY;
 		goto out;
diff --git a/mm/rmap.c b/mm/rmap.c
index da18f21..01f7832 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -1489,12 +1489,10 @@ static int page_mapcount_is_zero(struct page *page)
  *
  * Tries to remove all the page table entries which are mapping this
  * page, used in the pageout path.  Caller must hold the page lock.
- * Return values are:
  *
- * SWAP_SUCCESS	- we succeeded in removing all mappings
- * SWAP_FAIL	- the page is unswappable
+ * If unmap is successful, return true. Otherwise, false.
  */
-int try_to_unmap(struct page *page, enum ttu_flags flags)
+bool try_to_unmap(struct page *page, enum ttu_flags flags)
 {
 	struct rmap_walk_control rwc = {
 		.rmap_one = try_to_unmap_one,
@@ -1519,7 +1517,7 @@ int try_to_unmap(struct page *page, enum ttu_flags flags)
 	else
 		rmap_walk(page, &rwc);
 
-	return !page_mapcount(page) ? SWAP_SUCCESS: SWAP_FAIL;
+	return !page_mapcount(page) ? true: false;
 }
 
 static int page_not_mapped(struct page *page)
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 170c61f..e4b74f1 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -966,7 +966,6 @@ static unsigned long shrink_page_list(struct list_head *page_list,
 		int may_enter_fs;
 		enum page_references references = PAGEREF_RECLAIM_CLEAN;
 		bool dirty, writeback;
-		int ret = SWAP_SUCCESS;
 
 		cond_resched();
 
@@ -1139,13 +1138,9 @@ static unsigned long shrink_page_list(struct list_head *page_list,
 		 * processes. Try to unmap it here.
 		 */
 		if (page_mapped(page)) {
-			switch (ret = try_to_unmap(page,
-				ttu_flags | TTU_BATCH_FLUSH)) {
-			case SWAP_FAIL:
+			if (!try_to_unmap(page, ttu_flags | TTU_BATCH_FLUSH)) {
 				nr_unmap_fail++;
 				goto activate_locked;
-			case SWAP_SUCCESS:
-				; /* try to free the page below */
 			}
 		}
 
-- 
2.7.4

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

#1594948 — Re: [RFC 08/11] mm: make ttu's return boolean

On 03/01/2017 10:39 PM, Minchan Kim wrote:
> try_to_unmap returns SWAP_SUCCESS or SWAP_FAIL so it's suitable for
> boolean return. This patch changes it.

Hi Minchan,

So, up until this patch, I definitely like the cleanup, because as you observed, the 
return values didn't need so many different values. However, at this point, I think 
you should stop, and keep the SWAP_SUCCESS and SWAP_FAIL (or maybe even rename them 
to UNMAP_* or TTU_RESULT_*, to match their functions' names better), because 
removing them makes the code considerably less readable.

And since this is billed as a cleanup, we care here, even though this is a minor 
point. :)

Bool return values are sometimes perfect, such as when asking a question:

    bool mode_changed = needs_modeset(crtc_state);

The above is very nice. However, for returning success or failure, bools are not as 
nice, because *usually* success == true, except when you use the errno-based system, 
in which success == 0 (which would translate to false, if you mistakenly treated it 
as a bool). That leads to the reader having to remember which system is in use, 
usually with no visual cues to help.

>
[...]
>  	if (PageSwapCache(p)) {
> @@ -971,7 +971,7 @@ static int hwpoison_user_mappings(struct page *p, unsigned long pfn,
>  		collect_procs(hpage, &tokill, flags & MF_ACTION_REQUIRED);
>
>  	ret = try_to_unmap(hpage, ttu);
> -	if (ret != SWAP_SUCCESS)
> +	if (!ret)
>  		pr_err("Memory failure: %#lx: failed to unmap page (mapcount=%d)\n",
>  		       pfn, page_mapcount(hpage));
>
> @@ -986,8 +986,7 @@ static int hwpoison_user_mappings(struct page *p, unsigned long pfn,
>  	 * any accesses to the poisoned memory.
>  	 */
>  	forcekill = PageDirty(hpage) || (flags & MF_MUST_KILL);
> -	kill_procs(&tokill, forcekill, trapno,
> -		      ret != SWAP_SUCCESS, p, pfn, flags);
> +	kill_procs(&tokill, forcekill, trapno, !ret , p, pfn, flags);

The kill_procs() invocation was a little more readable before.

>
[...]
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 170c61f..e4b74f1 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -966,7 +966,6 @@ static unsigned long shrink_page_list(struct list_head *page_list,
>  		int may_enter_fs;
>  		enum page_references references = PAGEREF_RECLAIM_CLEAN;
>  		bool dirty, writeback;
> -		int ret = SWAP_SUCCESS;
>
>  		cond_resched();
>
> @@ -1139,13 +1138,9 @@ static unsigned long shrink_page_list(struct list_head *page_list,
>  		 * processes. Try to unmap it here.
>  		 */
>  		if (page_mapped(page)) {
> -			switch (ret = try_to_unmap(page,
> -				ttu_flags | TTU_BATCH_FLUSH)) {
> -			case SWAP_FAIL:

Again: the SWAP_FAIL makes it crystal clear which case we're in.

I also wonder if UNMAP_FAIL or TTU_RESULT_FAIL is a better name?

thanks,
John Hubbard
NVIDIA

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

#1595704 — Re: [RFC 08/11] mm: make ttu's return boolean

Hi John,

On Tue, Mar 07, 2017 at 11:13:26PM -0800, John Hubbard wrote:
> On 03/01/2017 10:39 PM, Minchan Kim wrote:
> >try_to_unmap returns SWAP_SUCCESS or SWAP_FAIL so it's suitable for
> >boolean return. This patch changes it.
> 
> Hi Minchan,
> 
> So, up until this patch, I definitely like the cleanup, because as you
> observed, the return values didn't need so many different values. However,
> at this point, I think you should stop, and keep the SWAP_SUCCESS and
> SWAP_FAIL (or maybe even rename them to UNMAP_* or TTU_RESULT_*, to match
> their functions' names better), because removing them makes the code
> considerably less readable.
> 
> And since this is billed as a cleanup, we care here, even though this is a
> minor point. :)
> 
> Bool return values are sometimes perfect, such as when asking a question:
> 
>    bool mode_changed = needs_modeset(crtc_state);
> 
> The above is very nice. However, for returning success or failure, bools are
> not as nice, because *usually* success == true, except when you use the
> errno-based system, in which success == 0 (which would translate to false,
> if you mistakenly treated it as a bool). That leads to the reader having to
> remember which system is in use, usually with no visual cues to help.

I think it's the matter of taste.

        if (try_to_unmap(xxx))
                something
        else
                something

It's perfectly understandable to me. IOW, if try_to_unmap returns true,
it means it did unmap successfully. Otherwise, failed.

IMHO, SWAP_SUCCESS or TTU_RESULT_* seems to be an over-engineering.
If the user want it, user can do it by introducing right variable name
in his context. See below.

> 
> >
> [...]
> > 	if (PageSwapCache(p)) {
> >@@ -971,7 +971,7 @@ static int hwpoison_user_mappings(struct page *p, unsigned long pfn,
> > 		collect_procs(hpage, &tokill, flags & MF_ACTION_REQUIRED);
> >
> > 	ret = try_to_unmap(hpage, ttu);
> >-	if (ret != SWAP_SUCCESS)
> >+	if (!ret)
> > 		pr_err("Memory failure: %#lx: failed to unmap page (mapcount=%d)\n",
> > 		       pfn, page_mapcount(hpage));
> >
> >@@ -986,8 +986,7 @@ static int hwpoison_user_mappings(struct page *p, unsigned long pfn,
> > 	 * any accesses to the poisoned memory.
> > 	 */
> > 	forcekill = PageDirty(hpage) || (flags & MF_MUST_KILL);
> >-	kill_procs(&tokill, forcekill, trapno,
> >-		      ret != SWAP_SUCCESS, p, pfn, flags);
> >+	kill_procs(&tokill, forcekill, trapno, !ret , p, pfn, flags);
> 
> The kill_procs() invocation was a little more readable before.

Indeed but I think it's not a problem of try_to_unmap but ret variable name
isn't good any more. How about this?

        bool unmap_success;

        unmap_success = try_to_unmap(hpage, ttu);

        ..

        kill_procs(&tokill, forcekill, trapno, !unmap_success , p, pfn, flags);

        ..

        return unmap_success;

My point is user can introduce whatever variable name depends on his
context. No need to make return variable complicated, IMHO.

> 
> >
> [...]
> >diff --git a/mm/vmscan.c b/mm/vmscan.c
> >index 170c61f..e4b74f1 100644
> >--- a/mm/vmscan.c
> >+++ b/mm/vmscan.c
> >@@ -966,7 +966,6 @@ static unsigned long shrink_page_list(struct list_head *page_list,
> > 		int may_enter_fs;
> > 		enum page_references references = PAGEREF_RECLAIM_CLEAN;
> > 		bool dirty, writeback;
> >-		int ret = SWAP_SUCCESS;
> >
> > 		cond_resched();
> >
> >@@ -1139,13 +1138,9 @@ static unsigned long shrink_page_list(struct list_head *page_list,
> > 		 * processes. Try to unmap it here.
> > 		 */
> > 		if (page_mapped(page)) {
> >-			switch (ret = try_to_unmap(page,
> >-				ttu_flags | TTU_BATCH_FLUSH)) {
> >-			case SWAP_FAIL:
> 
> Again: the SWAP_FAIL makes it crystal clear which case we're in.

To me, I don't feel it.
To me, below is perfectly understandable.

        if (try_to_unmap())
                do something
 
That's why I think it's matter of taste. Okay, I admit I might be
biased, too so I will consider what you suggested if others votes
it.

Thanks.

> 
> I also wonder if UNMAP_FAIL or TTU_RESULT_FAIL is a better name?
> 
> thanks,
> John Hubbard
> NVIDIA
> 
> --
> 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]

#1595711 — Re: [RFC 08/11] mm: make ttu's return boolean

On 03/08/2017 10:37 PM, Minchan Kim wrote:
 >[...]
>
> I think it's the matter of taste.
>
>         if (try_to_unmap(xxx))
>                 something
>         else
>                 something
>
> It's perfectly understandable to me. IOW, if try_to_unmap returns true,
> it means it did unmap successfully. Otherwise, failed.
>
> IMHO, SWAP_SUCCESS or TTU_RESULT_* seems to be an over-engineering.
> If the user want it, user can do it by introducing right variable name
> in his context. See below.

I'm OK with that approach. Just something to avoid the "what does !ret mean in this 
function call" is what I was looking for...


>> [...]
>>> 	forcekill = PageDirty(hpage) || (flags & MF_MUST_KILL);
>>> -	kill_procs(&tokill, forcekill, trapno,
>>> -		      ret != SWAP_SUCCESS, p, pfn, flags);
>>> +	kill_procs(&tokill, forcekill, trapno, !ret , p, pfn, flags);
>>
>> The kill_procs() invocation was a little more readable before.
>
> Indeed but I think it's not a problem of try_to_unmap but ret variable name
> isn't good any more. How about this?
>
>         bool unmap_success;
>
>         unmap_success = try_to_unmap(hpage, ttu);
>
>         ..
>
>         kill_procs(&tokill, forcekill, trapno, !unmap_success , p, pfn, flags);
>
>         ..
>
>         return unmap_success;
>
> My point is user can introduce whatever variable name depends on his
> context. No need to make return variable complicated, IMHO.

Yes, the local variable basically achieves what I was hoping for, so sure, works for 
me.

>> [...]
>>> -			case SWAP_FAIL:
>>
>> Again: the SWAP_FAIL makes it crystal clear which case we're in.
>
> To me, I don't feel it.
> To me, below is perfectly understandable.
>
>         if (try_to_unmap())
>                 do something
>
> That's why I think it's matter of taste. Okay, I admit I might be
> biased, too so I will consider what you suggested if others votes
> it.

Yes, if it's really just a matter of taste, then not worth debating. Your change 
above is fine I think.

thanks
john h

>
> Thanks.
>

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

#1590888 — [RFC 06/11] mm: remove SWAP_MLOCK in ttu

ttu don't need to return SWAP_MLOCK. Instead, just return SWAP_FAIL
because it means the page is not-swappable so it should move to
another LRU list(active or unevictable). putback friends will
move it to right list depending on the page's LRU flag.

A side effect is shrink_page_list accounts unevictable list movement
by PGACTIVATE but I don't think it corrupts something severe.

Signed-off-by: Minchan Kim <minchan@kernel.org>
---
 include/linux/rmap.h |  1 -
 mm/rmap.c            |  3 +--
 mm/vmscan.c          | 14 +++-----------
 3 files changed, 4 insertions(+), 14 deletions(-)

diff --git a/include/linux/rmap.h b/include/linux/rmap.h
index 1b0cd4c..3630d4d 100644
--- a/include/linux/rmap.h
+++ b/include/linux/rmap.h
@@ -297,6 +297,5 @@ static inline int page_mkclean(struct page *page)
 #define SWAP_SUCCESS	0
 #define SWAP_AGAIN	1
 #define SWAP_FAIL	2
-#define SWAP_MLOCK	3
 
 #endif	/* _LINUX_RMAP_H */
diff --git a/mm/rmap.c b/mm/rmap.c
index 61ae694..47898a1 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -1328,7 +1328,7 @@ static int try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
 					 */
 					mlock_vma_page(page);
 				}
-				ret = SWAP_MLOCK;
+				ret = SWAP_FAIL;
 				page_vma_mapped_walk_done(&pvmw);
 				break;
 			}
@@ -1494,7 +1494,6 @@ static int page_mapcount_is_zero(struct page *page)
  * SWAP_SUCCESS	- we succeeded in removing all mappings
  * SWAP_AGAIN	- we missed a mapping, try again later
  * SWAP_FAIL	- the page is unswappable
- * SWAP_MLOCK	- page is mlocked.
  */
 int try_to_unmap(struct page *page, enum ttu_flags flags)
 {
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 91cef05..3cdd270b 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -981,7 +981,7 @@ static unsigned long shrink_page_list(struct list_head *page_list,
 		sc->nr_scanned++;
 
 		if (unlikely(!page_evictable(page)))
-			goto cull_mlocked;
+			goto activate_locked;
 
 		if (!sc->may_unmap && page_mapped(page))
 			goto keep_locked;
@@ -1146,8 +1146,6 @@ static unsigned long shrink_page_list(struct list_head *page_list,
 				goto activate_locked;
 			case SWAP_AGAIN:
 				goto keep_locked;
-			case SWAP_MLOCK:
-				goto cull_mlocked;
 			case SWAP_SUCCESS:
 				; /* try to free the page below */
 			}
@@ -1289,16 +1287,10 @@ static unsigned long shrink_page_list(struct list_head *page_list,
 		list_add(&page->lru, &free_pages);
 		continue;
 
-cull_mlocked:
-		if (PageSwapCache(page))
-			try_to_free_swap(page);
-		unlock_page(page);
-		list_add(&page->lru, &ret_pages);
-		continue;
-
 activate_locked:
 		/* Not a candidate for swapping, so reclaim swap space. */
-		if (PageSwapCache(page) && mem_cgroup_swap_full(page))
+		if (PageSwapCache(page) && (mem_cgroup_swap_full(page) ||
+						PageMlocked(page)))
 			try_to_free_swap(page);
 		VM_BUG_ON_PAGE(PageActive(page), page);
 		SetPageActive(page);
-- 
2.7.4

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

#1591975 — Re: [RFC 06/11] mm: remove SWAP_MLOCK in ttu

On 03/02/2017 12:09 PM, Minchan Kim wrote:
> ttu don't need to return SWAP_MLOCK. Instead, just return SWAP_FAIL
> because it means the page is not-swappable so it should move to
> another LRU list(active or unevictable). putback friends will
> move it to right list depending on the page's LRU flag.

Right, if it cannot be swapped out there is not much difference with
SWAP_FAIL once we change the callers who expected to see a SWAP_MLOCK
return instead.

> 
> A side effect is shrink_page_list accounts unevictable list movement
> by PGACTIVATE but I don't think it corrupts something severe.

Not sure I got that, could you please elaborate on this. We will still
activate the page and put it in an appropriate LRU list if it is marked
mlocked ?

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

#1592889 — Re: [RFC 06/11] mm: remove SWAP_MLOCK in ttu

Hi Anshuman,

On Fri, Mar 03, 2017 at 06:06:38PM +0530, Anshuman Khandual wrote:
> On 03/02/2017 12:09 PM, Minchan Kim wrote:
> > ttu don't need to return SWAP_MLOCK. Instead, just return SWAP_FAIL
> > because it means the page is not-swappable so it should move to
> > another LRU list(active or unevictable). putback friends will
> > move it to right list depending on the page's LRU flag.
> 
> Right, if it cannot be swapped out there is not much difference with
> SWAP_FAIL once we change the callers who expected to see a SWAP_MLOCK
> return instead.
> 
> > 
> > A side effect is shrink_page_list accounts unevictable list movement
> > by PGACTIVATE but I don't think it corrupts something severe.
> 
> Not sure I got that, could you please elaborate on this. We will still
> activate the page and put it in an appropriate LRU list if it is marked
> mlocked ?

Right. putback_iactive_pages/putback_lru_page has a logic to filter
out unevictable pages and move them to unevictable LRU list so it
doesn't break LRU change behavior but the concern is until now,
we have accounted PGACTIVATE for only evictable LRU list page but
by this change, it accounts it to unevictable LRU list as well.
However, although I don't think it's big problem in real practice,
we can fix it simply with checking PG_mlocked if someone reports.

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]

#1594501 — Re: [RFC 06/11] mm: remove SWAP_MLOCK in ttu

On Mon, Mar 06, 2017 at 11:15:08AM +0900, Minchan Kim wrote:
> Hi Anshuman,
> 
> On Fri, Mar 03, 2017 at 06:06:38PM +0530, Anshuman Khandual wrote:
> > On 03/02/2017 12:09 PM, Minchan Kim wrote:
> > > ttu don't need to return SWAP_MLOCK. Instead, just return SWAP_FAIL
> > > because it means the page is not-swappable so it should move to
> > > another LRU list(active or unevictable). putback friends will
> > > move it to right list depending on the page's LRU flag.
> > 
> > Right, if it cannot be swapped out there is not much difference with
> > SWAP_FAIL once we change the callers who expected to see a SWAP_MLOCK
> > return instead.
> > 
> > > 
> > > A side effect is shrink_page_list accounts unevictable list movement
> > > by PGACTIVATE but I don't think it corrupts something severe.
> > 
> > Not sure I got that, could you please elaborate on this. We will still
> > activate the page and put it in an appropriate LRU list if it is marked
> > mlocked ?
> 
> Right. putback_iactive_pages/putback_lru_page has a logic to filter
> out unevictable pages and move them to unevictable LRU list so it
> doesn't break LRU change behavior but the concern is until now,
> we have accounted PGACTIVATE for only evictable LRU list page but
> by this change, it accounts it to unevictable LRU list as well.
> However, although I don't think it's big problem in real practice,
> we can fix it simply with checking PG_mlocked if someone reports.

I think it's better to do this pro-actively. Let's hide both pgactivate++
and SetPageActive() under "if (!PageMlocked())".
SetPageActive() is not free.

-- 
 Kirill A. Shutemov

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

#1594880 — Re: [RFC 06/11] mm: remove SWAP_MLOCK in ttu

On Tue, Mar 07, 2017 at 06:24:37PM +0300, Kirill A. Shutemov wrote:
> On Mon, Mar 06, 2017 at 11:15:08AM +0900, Minchan Kim wrote:
> > Hi Anshuman,
> > 
> > On Fri, Mar 03, 2017 at 06:06:38PM +0530, Anshuman Khandual wrote:
> > > On 03/02/2017 12:09 PM, Minchan Kim wrote:
> > > > ttu don't need to return SWAP_MLOCK. Instead, just return SWAP_FAIL
> > > > because it means the page is not-swappable so it should move to
> > > > another LRU list(active or unevictable). putback friends will
> > > > move it to right list depending on the page's LRU flag.
> > > 
> > > Right, if it cannot be swapped out there is not much difference with
> > > SWAP_FAIL once we change the callers who expected to see a SWAP_MLOCK
> > > return instead.
> > > 
> > > > 
> > > > A side effect is shrink_page_list accounts unevictable list movement
> > > > by PGACTIVATE but I don't think it corrupts something severe.
> > > 
> > > Not sure I got that, could you please elaborate on this. We will still
> > > activate the page and put it in an appropriate LRU list if it is marked
> > > mlocked ?
> > 
> > Right. putback_iactive_pages/putback_lru_page has a logic to filter
> > out unevictable pages and move them to unevictable LRU list so it
> > doesn't break LRU change behavior but the concern is until now,
> > we have accounted PGACTIVATE for only evictable LRU list page but
> > by this change, it accounts it to unevictable LRU list as well.
> > However, although I don't think it's big problem in real practice,
> > we can fix it simply with checking PG_mlocked if someone reports.
> 
> I think it's better to do this pro-actively. Let's hide both pgactivate++
> and SetPageActive() under "if (!PageMlocked())".
> SetPageActive() is not free.

I will consider it in next spin.

Thanks!

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

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

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))
 		ret = SWAP_SUCCESS;
 	return ret;
 }
-- 
2.7.4

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

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

On 03/02/2017 12:09 PM, Minchan Kim wrote:
> If the page is mapped and rescue in ttuo, page_mapcount(page) == 0 cannot

Nit: "ttuo" is very cryptic. Please expand it.

> be true so page_mapcount check in ttu is enough to return SWAP_SUCCESS.
> IOW, SWAP_MLOCK check is redundant so remove it.

Right, page_mapcount(page) should be enough to tell whether swapping
out happened successfully or the page is still mapped in some page
table.

> 
> 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))
>  		ret = SWAP_SUCCESS;
>  	return ret;
>  }
> 

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

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

On Thu, Mar 02, 2017 at 08:21:46PM +0530, Anshuman Khandual wrote:
> On 03/02/2017 12:09 PM, Minchan Kim wrote:
> > If the page is mapped and rescue in ttuo, page_mapcount(page) == 0 cannot
> 
> Nit: "ttuo" is very cryptic. Please expand it.

No problem.

> 
> > be true so page_mapcount check in ttu is enough to return SWAP_SUCCESS.
> > IOW, SWAP_MLOCK check is redundant so remove it.
> 
> Right, page_mapcount(page) should be enough to tell whether swapping
> out happened successfully or the page is still mapped in some page
> table.
> 

Thanks for the review, Anshuman!

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

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web