make try_to_unmap simple

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

Quoted 1 line

Cc Kirill A. Shutemov <[email protected]>

Signed-off-by Minchan Kim <[email protected]>

---
 mm/huge_memory.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

mm/huge_memory.c +0 -0

index 092cc5c..fe2ccd4 100644

Quoted 2 lines · Minchan Kim

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.

Quoted 19 lines

Quoted 2 lines

Cc Kirill A. Shutemov <[email protected]>

Signed-off-by Minchan Kim <[email protected]>

---
 mm/huge_memory.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

mm/huge_memory.c +1 -1

index 092cc5c..fe2ccd4 100644
@@ -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);
 }
 

mm/huge_memory.c static void freeze_page(struct page *page) · line 2120

Quoted 2 lines

mm/huge_memory.c +0 -0

 static void unfreeze_page(struct page *page)

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

Quoted 7 lines · Anshuman Khandual

Quoted 2 lines · Minchan Kim

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?

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

Quoted 2 lines · Minchan Kim

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.

Signature · 8 lines

-- 
 Kirill A. Shutemov

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

Anyone doesn't use ret variable. Remove it.

Signed-off-by Minchan Kim <[email protected]>

---
 mm/rmap.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

mm/rmap.c +0 -1

index bb45712..8076347 100644
@@ -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),

Quoted 2 lines · Minchan Kim

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.

Quoted 27 lines

Signed-off-by Minchan Kim <[email protected]>

---
 mm/rmap.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

mm/rmap.c +1 -2

index bb45712..8076347 100644
@@ -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;
 

mm/rmap.c int page_referenced(struct page *page, · line 844

Quoted 2 lines

mm/rmap.c +0 -0

 	if (we_locked)

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

Anyone doesn't use ret variable. Remove it.

Signed-off-by Minchan Kim <[email protected]>

mm/rmap.c static int try_to_unmap_one(struct page *page, struct vm_area_struct *vma, · line 1425

Quoted 3 lines · Minchan Kim

mm/rmap.c +0 -0

@@ -1424,7 +1424,8 @@ static int try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
 			} else if (!PageSwapBacked(page)) {
 				/* dirty MADV_FREE page */

Nit: enrich the comment please.

Quoted 5 lines

mm/rmap.c +2 -1

 				set_pte_at(mm, address, pvmw.pte, pteval);
-				ret = SWAP_DIRTY;
+				SetPageSwapBacked(page);
+				ret = SWAP_FAIL;
 				page_vma_mapped_walk_done(&pvmw);

Review static int try_to_unmap_one(struct page *page, struct vm_area_struct *vma, · line 1431

Quoted 2 lines

(quoted diff) +0 -0

 				break;
 			}

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

If we found lazyfree page is dirty, ttuo can just SetPageSwapBakced in there like PG_mlocked page and just return with SWAP_FAIL which is very natural because the page is not swappable right now so that vmscan can activate it. There is no point to introduce new return value SWAP_DIRTY in ttu at the moment.

If we found lazyfree page is dirty, ttuo can just SetPageSwapBakced in there like PG_mlocked page and just return with SWAP_FAIL which is very natural because the page is not swappable right now so that vmscan can activate it. There is no point to introduce new return value SWAP_DIRTY in ttu at the moment.

Cc Shaohua Li <[email protected]>

Signed-off-by Minchan Kim <[email protected]>

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 <[email protected]>

---
 mm/rmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

mm/rmap.c +0 -0

index 3a14013..0a48958 100644

Quoted 1 line · Minchan Kim

If the page is mapped and rescue in ttuo, page_mapcount(page) == 0 cannot

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

Quoted 3 lines

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.

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 <[email protected]>

---
 mm/rmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

mm/rmap.c +0 -0

index 3a14013..0a48958 100644

Quoted 17 lines · Minchan Kim

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 <[email protected]>

---
 mm/rmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

mm/rmap.c +0 -0

index 3a14013..0a48958 100644
@@ -1523,7 +1523,7 @@ int try_to_unmap(struct page *page, enum ttu_flags flags)
 	else
 		ret = rmap_walk(page, &rwc);
 

mm/rmap.c int try_to_unmap(struct page *page, enum ttu_flags flags) · line 1526

Quoted 2 lines

mm/rmap.c +1 -1

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

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 <[email protected]>

Cc Kirill A. Shutemov <[email protected]>

Signed-off-by Minchan Kim <[email protected]>

---
 include/linux/rmap.h |  2 +-
 mm/mlock.c           |  6 ++----
 mm/rmap.c            | 16 ++++------------
 3 files changed, 7 insertions(+), 17 deletions(-)

include/linux/rmap.h +1 -1

index b556eef..1b0cd4c 100644
@@ -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);
 

mm/mlock.c +0 -2

index cdbed8a..d34a540 100644
@@ -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;
-
 	/*

mm/mlock.c +0 -0

 		count_vm_event(UNEVICTABLE_PGMUNLOCKED);
 
 	putback_lru_page(page);

mm/rmap.c +2 -10

index 0a48958..61ae694 100644
@@ -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,

Quoted 4 lines · Minchan Kim

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.

Quoted 5 lines

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.

Quoted 34 lines

Cc Vlastimil Babka <[email protected]>

Cc Kirill A. Shutemov <[email protected]>

Signed-off-by Minchan Kim <[email protected]>

---
 include/linux/rmap.h |  2 +-
 mm/mlock.c           |  6 ++----
 mm/rmap.c            | 16 ++++------------
 3 files changed, 7 insertions(+), 17 deletions(-)

include/linux/rmap.h +1 -1

index b556eef..1b0cd4c 100644
@@ -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);
 

mm/mlock.c +0 -2

index cdbed8a..d34a540 100644
@@ -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;
-
 	/*

mm/mlock.c static bool __munlock_isolate_lru_page(struct page *page, bool getpage) · line 133

Quoted 10 lines

mm/mlock.c +2 -2

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

Quoted 31 lines

mm/mlock.c +0 -0

 		count_vm_event(UNEVICTABLE_PGMUNLOCKED);
 
 	putback_lru_page(page);

mm/rmap.c +2 -10

index 0a48958..61ae694 100644
@@ -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)
 	};
 

mm/rmap.c int try_to_munlock(struct page *page) · line 1556

Quoted 2 lines

mm/rmap.c +1 -0

 	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 ?

Quoted 94 lines · Anshuman Khandual

Quoted 4 lines · Minchan Kim

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.

Quoted 5 lines

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.

Quoted 34 lines

Cc Vlastimil Babka <[email protected]>

Cc Kirill A. Shutemov <[email protected]>

Signed-off-by Minchan Kim <[email protected]>

---
 include/linux/rmap.h |  2 +-
 mm/mlock.c           |  6 ++----
 mm/rmap.c            | 16 ++++------------
 3 files changed, 7 insertions(+), 17 deletions(-)

include/linux/rmap.h +1 -1

index b556eef..1b0cd4c 100644
@@ -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);
 

mm/mlock.c +0 -2

index cdbed8a..d34a540 100644
@@ -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;
-
 	/*

mm/mlock.c static bool __munlock_isolate_lru_page(struct page *page, bool getpage) · line 133

Quoted 10 lines

mm/mlock.c +2 -2

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

Quoted 31 lines

mm/mlock.c +0 -0

 		count_vm_event(UNEVICTABLE_PGMUNLOCKED);
 
 	putback_lru_page(page);

mm/rmap.c +2 -10

index 0a48958..61ae694 100644
@@ -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)
 	};
 

Quoted 8 lines

mm/rmap.c int try_to_munlock(struct page *page) · line 1556

Quoted 2 lines

mm/rmap.c +1 -0

 	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.

Quoted 104 lines · Minchan Kim

Quoted 94 lines · Anshuman Khandual

Quoted 4 lines · Minchan Kim

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.

Quoted 5 lines

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.

Quoted 34 lines

Cc Vlastimil Babka <[email protected]>

Cc Kirill A. Shutemov <[email protected]>

Signed-off-by Minchan Kim <[email protected]>

---
 include/linux/rmap.h |  2 +-
 mm/mlock.c           |  6 ++----
 mm/rmap.c            | 16 ++++------------
 3 files changed, 7 insertions(+), 17 deletions(-)

include/linux/rmap.h +1 -1

index b556eef..1b0cd4c 100644
@@ -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);
 

mm/mlock.c +0 -2

index cdbed8a..d34a540 100644
@@ -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;
-
 	/*

mm/mlock.c static bool __munlock_isolate_lru_page(struct page *page, bool getpage) · line 133

Quoted 10 lines

mm/mlock.c +2 -2

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

Quoted 31 lines

mm/mlock.c +0 -0

 		count_vm_event(UNEVICTABLE_PGMUNLOCKED);
 
 	putback_lru_page(page);

mm/rmap.c +2 -10

index 0a48958..61ae694 100644
@@ -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)
 	};
 

Quoted 8 lines

mm/rmap.c int try_to_munlock(struct page *page) · line 1556

Quoted 2 lines

mm/rmap.c +1 -0

 	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 ?

Quoted 2 lines

If I read code properly, __munlock_isolated_page calls try_to_munlock always pass the TestClearPageMlocked page to try_to_munlock.

Right.

Quoted 4 lines

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

Hi Anshuman,

Quoted 100 lines · Anshuman Khandual

Quoted 97 lines · Minchan Kim

Quoted 89 lines · Anshuman Khandual

Quoted 4 lines · Minchan Kim

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.

Quoted 4 lines

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.

Quoted 33 lines

Cc Vlastimil Babka <[email protected]>

Cc Kirill A. Shutemov <[email protected]>

Signed-off-by Minchan Kim <[email protected]>

---
 include/linux/rmap.h |  2 +-
 mm/mlock.c           |  6 ++----
 mm/rmap.c            | 16 ++++------------
 3 files changed, 7 insertions(+), 17 deletions(-)

include/linux/rmap.h +1 -1

index b556eef..1b0cd4c 100644
@@ -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);
 

mm/mlock.c +0 -2

index cdbed8a..d34a540 100644
@@ -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;
-
 	/*

mm/mlock.c static bool __munlock_isolate_lru_page(struct page *page, bool getpage) · line 133

Quoted 10 lines

mm/mlock.c +2 -2

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

Quoted 31 lines

mm/mlock.c +0 -0

 		count_vm_event(UNEVICTABLE_PGMUNLOCKED);
 
 	putback_lru_page(page);

mm/rmap.c +2 -10

index 0a48958..61ae694 100644
@@ -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)
 	};
 

Quoted 7 lines

mm/rmap.c int try_to_munlock(struct page *page) · line 1556

Quoted 2 lines

mm/rmap.c +1 -0

 	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 ?

Quoted 2 lines

If I read code properly, __munlock_isolated_page calls try_to_munlock always pass the TestClearPageMlocked page to try_to_munlock.

Quoted 8 lines

Right.

Quoted 4 lines

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

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.

Quoted 9 lines · Minchan Kim

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.

Signature · 8 lines

-- 
 Kirill A. Shutemov

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

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.

Quoted 4 lines · Minchan Kim

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.

Quoted 1 line

Quoted 2 lines

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 ?

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

Hi Anshuman,

Quoted 11 lines · Anshuman Khandual

Quoted 4 lines · Minchan Kim

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.

Quoted 1 line

Quoted 6 lines

Quoted 2 lines

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.

Quoted 21 lines · Minchan Kim

Hi Anshuman,

Quoted 11 lines · Anshuman Khandual

Quoted 4 lines · Minchan Kim

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.

Quoted 1 line

Quoted 6 lines

Quoted 2 lines

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 ?

Quoted 7 lines

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.

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 <[email protected]>

---
 mm/rmap.c   | 11 +++--------
 mm/vmscan.c |  2 --
 2 files changed, 3 insertions(+), 10 deletions(-)

mm/rmap.c +0 -3

index 47898a1..da18f21 100644
@@ -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)

Quoted 2 lines · Minchan Kim

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.

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

mm/memory-failure.c +1 -1

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

mm/vmscan.c +0 -1

index 170c61f..e4b74f1 100644
@@ -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();
 

Quoted 2 lines · Minchan Kim

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.

Quoted 2 lines

[...]

Quoted 10 lines

	if (PageSwapCache(p)) {

mm/memory-failure.c +1 -1

@@ -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));
 

mm/memory-failure.c static int hwpoison_user_mappings(struct page *p, unsigned long pfn, · line 989

Quoted 7 lines

mm/memory-failure.c +1 -2

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

Quoted 2 lines

[...]

Quoted 12 lines

mm/vmscan.c +0 -1

index 170c61f..e4b74f1 100644
@@ -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();
 

mm/vmscan.c static unsigned long shrink_page_list(struct list_head *page_list, · line 1144

Quoted 7 lines

mm/vmscan.c +0 -3

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

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

[...]

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.

Quoted 7 lines

[...]

mm/memory-failure.c static int hwpoison_user_mappings(struct page *p, unsigned long pfn, · line 989

Quoted 4 lines

	forcekill = PageDirty(hpage) || (flags & MF_MUST_KILL);

mm/memory-failure.c +1 -2

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

Quoted 4 lines

[...]

mm/vmscan.c static unsigned long shrink_page_list(struct list_head *page_list, · line 1144

Quoted 1 line

mm/vmscan.c +0 -1

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

There is no user for it. Remove it.

Quoted 1 line · Minchan Kim

There is no user for it. Remove it.

Last patches in the series prepared ground for this removal. The entire series looks pretty straight forward. As it does not change

any functionality, wondering what kind of tests this should go through to look for any potential problems.

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.

Quoted 5 lines · Minchan Kim

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 ?

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

Hi Anshuman,

Quoted 7 lines · Anshuman Khandual

Quoted 5 lines · Minchan Kim

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.

Quoted 2 lines

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.