mm: fix some MADV_FREE issues

Johannes pointed out TTU_LZFREE is unnecessary. It's true because we always have the flag set if we want to do an unmap. For cases we don't do an unmap, the TTU_LZFREE part of code should never run.

Also the TTU_UNMAP is unnecessary. If no other flags set (for example, TTU_MIGRATION), an unmap is implied.

Cc Michal Hocko <[email protected]>

Cc Minchan Kim <[email protected]>

Cc Hugh Dickins <[email protected]>

Cc Rik van Riel <[email protected]>

Cc Mel Gorman <[email protected]>

Cc Andrew Morton <[email protected]>

Suggested-by Johannes Weiner <[email protected]>

Signed-off-by Shaohua Li <[email protected]>

Johannes pointed out TTU_LZFREE is unnecessary. It's true because we always have the flag set if we want to do an unmap. For cases we don't do an unmap, the TTU_LZFREE part of code should never run.

Also the TTU_UNMAP is unnecessary. If no other flags set (for example, TTU_MIGRATION), an unmap is implied.

Cc Michal Hocko <[email protected]>

Cc Minchan Kim <[email protected]>

Cc Hugh Dickins <[email protected]>

Cc Rik van Riel <[email protected]>

Cc Mel Gorman <[email protected]>

Cc Andrew Morton <[email protected]>

Suggested-by Johannes Weiner <[email protected]>

Signed-off-by Shaohua Li <[email protected]>

Johannes pointed out TTU_LZFREE is unnecessary. It's true because we always have the flag set if we want to do an unmap. For cases we don't do an unmap, the TTU_LZFREE part of code should never run.

Also the TTU_UNMAP is unnecessary. If no other flags set (for example, TTU_MIGRATION), an unmap is implied.

Cc Michal Hocko <[email protected]>

Cc Minchan Kim <[email protected]>

Cc Hugh Dickins <[email protected]>

Cc Rik van Riel <[email protected]>

Cc Mel Gorman <[email protected]>

Cc Andrew Morton <[email protected]>

Suggested-by Johannes Weiner <[email protected]>

Signed-off-by Shaohua Li <[email protected]>

---

mm/swap.c +4 -4

@@ -561,20 +567,26 @@ static void lru_deactivate_file_fn(struct page *page, struct lruvec *lruvec,
 }
 
 
-static void lru_deactivate_fn(struct page *page, struct lruvec *lruvec,
+static void lru_lazyfree_fn(struct page *page, struct lruvec *lruvec,
 			    void *arg)
 {
-	if (PageLRU(page) && PageActive(page) && !PageUnevictable(page)) {
-		int file = page_is_file_cache(page);
-		int lru = page_lru_base_type(page);
+	if (PageLRU(page) && PageAnon(page) && PageSwapBacked(page) &&
+	    !PageUnevictable(page)) {
+		bool active = PageActive(page);

Hi Shaohua,

Quoted 3 lines · Shaohua Li

mm/swap.c +0 -0

@@ -268,6 +268,12 @@ static void __activate_page(struct page *page, struct lruvec *lruvec,
 		int lru = page_lru_base_type(page);
 

mm/swap.c static void __activate_page(struct page *page, struct lruvec *lruvec, · line 276

Quoted 7 lines

mm/swap.c +6 -0

 		del_page_from_lru_list(page, lruvec, lru);
+		if (PageAnon(page) && !PageSwapBacked(page)) {
+			SetPageSwapBacked(page);
+			/* charge to anon scanned/rotated reclaim_stat */
+			file = 0;
+			lru = LRU_INACTIVE_ANON;
+		}

As per my previous feedback, please remove this. Write-after-free will be caught and handled in the reclaimer, read-after-free is a bug that really doesn't require optimizing page aging for. And we definitely shouldn't declare invalid data suddenly valid because it's being read.

Quoted 6 lines · Johannes Weiner

Hi Shaohua,

Quoted 3 lines · Shaohua Li

mm/swap.c +0 -0

@@ -268,6 +268,12 @@ static void __activate_page(struct page *page, struct lruvec *lruvec,
 		int lru = page_lru_base_type(page);
 

Quoted 12 lines

mm/swap.c static void __activate_page(struct page *page, struct lruvec *lruvec, · line 276

Quoted 7 lines

mm/swap.c +6 -0

 		del_page_from_lru_list(page, lruvec, lru);
+		if (PageAnon(page) && !PageSwapBacked(page)) {
+			SetPageSwapBacked(page);
+			/* charge to anon scanned/rotated reclaim_stat */
+			file = 0;
+			lru = LRU_INACTIVE_ANON;
+		}

As per my previous feedback, please remove this. Write-after-free will be caught and handled in the reclaimer, read-after-free is a bug that really doesn't require optimizing page aging for. And we definitely shouldn't declare invalid data suddenly valid because it's being read.

GUP could run into this. Don't we move the page because it's hot? I think it's not just about page aging. If we leave the page there, page reclaim will just waste time to reclaim the pages which should't be reclaimed.

Quoted 20 lines · Shaohua Li

Quoted 18 lines · Johannes Weiner

Hi Shaohua,

mm/swap.c static void __activate_page(struct page *page, struct lruvec *lruvec, · line 276

Quoted 10 lines · Shaohua Li

mm/swap.c +6 -0

@@ -268,6 +268,12 @@ static void __activate_page(struct page *page, struct lruvec *lruvec,
 		int lru = page_lru_base_type(page);
 
 		del_page_from_lru_list(page, lruvec, lru);
+		if (PageAnon(page) && !PageSwapBacked(page)) {
+			SetPageSwapBacked(page);
+			/* charge to anon scanned/rotated reclaim_stat */
+			file = 0;
+			lru = LRU_INACTIVE_ANON;
+		}

As per my previous feedback, please remove this. Write-after-free will be caught and handled in the reclaimer, read-after-free is a bug that really doesn't require optimizing page aging for. And we definitely shouldn't declare invalid data suddenly valid because it's being read.

Quoted 3 lines

GUP could run into this. Don't we move the page because it's hot? I think it's not just about page aging. If we leave the page there, page reclaim will just waste time to reclaim the pages which should't be reclaimed.

There is just no convincing justification to add this code, because it optimizes something that doesn't have a real world application. If we just delete this branch, for all intents and purposes the outcome will be perfectly acceptable.

madv MADV_FREE indicate pages are 'lazyfree'. They are still anonymous pages, but they can be freed without pageout. To destinguish them against normal anonymous pages, we clear their SwapBacked flag.

MADV_FREE pages could be freed without pageout, so they pretty much like used once file pages. For such pages, we'd like to reclaim them once there is memory pressure. Also it might be unfair reclaiming MADV_FREE pages always before used once file pages and we definitively want to reclaim the pages before other anonymous and file pages.

To speed up MADV_FREE pages reclaim, we put the pages into LRU_INACTIVE_FILE list. The rationale is LRU_INACTIVE_FILE list is tiny nowadays and should be full of used once file pages. Reclaiming MADV_FREE pages will not have much interfere of anonymous and active file pages. And the inactive file pages and MADV_FREE pages will be reclaimed according to their age, so we don't reclaim too many MADV_FREE pages too. Putting the MADV_FREE pages into LRU_INACTIVE_FILE_LIST also means we can reclaim the pages without swap support. This idea is suggested by Johannes.

This patch doesn't move MADV_FREE pages to LRU_INACTIVE_FILE list yet to avoid bisect failure, next patch will do it.

The patch is based on Minchan's original patch.

Cc Michal Hocko <[email protected]>

Cc Minchan Kim <[email protected]>

Cc Hugh Dickins <[email protected]>

Cc Rik van Riel <[email protected]>

Cc Mel Gorman <[email protected]>

Cc Andrew Morton <[email protected]>

Suggested-by Johannes Weiner <[email protected]>

Signed-off-by Shaohua Li <[email protected]>

Quoted 25 lines · Shaohua Li

madv MADV_FREE indicate pages are 'lazyfree'. They are still anonymous pages, but they can be freed without pageout. To destinguish them against normal anonymous pages, we clear their SwapBacked flag.

MADV_FREE pages could be freed without pageout, so they pretty much like used once file pages. For such pages, we'd like to reclaim them once there is memory pressure. Also it might be unfair reclaiming MADV_FREE pages always before used once file pages and we definitively want to reclaim the pages before other anonymous and file pages.

To speed up MADV_FREE pages reclaim, we put the pages into LRU_INACTIVE_FILE list. The rationale is LRU_INACTIVE_FILE list is tiny nowadays and should be full of used once file pages. Reclaiming MADV_FREE pages will not have much interfere of anonymous and active file pages. And the inactive file pages and MADV_FREE pages will be reclaimed according to their age, so we don't reclaim too many MADV_FREE pages too. Putting the MADV_FREE pages into LRU_INACTIVE_FILE_LIST also means we can reclaim the pages without swap support. This idea is suggested by Johannes.

This patch doesn't move MADV_FREE pages to LRU_INACTIVE_FILE list yet to avoid bisect failure, next patch will do it.

The patch is based on Minchan's original patch.

Quoted 8 lines

Cc Michal Hocko <[email protected]>

Cc Minchan Kim <[email protected]>

Cc Hugh Dickins <[email protected]>

Cc Rik van Riel <[email protected]>

Cc Mel Gorman <[email protected]>

Cc Andrew Morton <[email protected]>

Suggested-by Johannes Weiner <[email protected]>

Signed-off-by Shaohua Li <[email protected]>

Other than that Johannes pointed out, code itself looks good to me. However, I hope to merge this patch with next one.
It's enough simple to merge, change behavior(about deactivation), mark_page_lazyfree is introduced but there is no callsite to use it in this patch.

I don't think it's worth to separate.

Quoted 41 lines · Minchan Kim

Quoted 25 lines · Shaohua Li

madv MADV_FREE indicate pages are 'lazyfree'. They are still anonymous pages, but they can be freed without pageout. To destinguish them against normal anonymous pages, we clear their SwapBacked flag.

MADV_FREE pages could be freed without pageout, so they pretty much like used once file pages. For such pages, we'd like to reclaim them once there is memory pressure. Also it might be unfair reclaiming MADV_FREE pages always before used once file pages and we definitively want to reclaim the pages before other anonymous and file pages.

To speed up MADV_FREE pages reclaim, we put the pages into LRU_INACTIVE_FILE list. The rationale is LRU_INACTIVE_FILE list is tiny nowadays and should be full of used once file pages. Reclaiming MADV_FREE pages will not have much interfere of anonymous and active file pages. And the inactive file pages and MADV_FREE pages will be reclaimed according to their age, so we don't reclaim too many MADV_FREE pages too. Putting the MADV_FREE pages into LRU_INACTIVE_FILE_LIST also means we can reclaim the pages without swap support. This idea is suggested by Johannes.

This patch doesn't move MADV_FREE pages to LRU_INACTIVE_FILE list yet to avoid bisect failure, next patch will do it.

The patch is based on Minchan's original patch.

Quoted 8 lines

Cc Michal Hocko <[email protected]>

Cc Minchan Kim <[email protected]>

Cc Hugh Dickins <[email protected]>

Cc Rik van Riel <[email protected]>

Cc Mel Gorman <[email protected]>

Cc Andrew Morton <[email protected]>

Suggested-by Johannes Weiner <[email protected]>

Signed-off-by Shaohua Li <[email protected]>

Other than that Johannes pointed out, code itself looks good to me. However, I hope to merge this patch with next one.
It's enough simple to merge, change behavior(about deactivation), mark_page_lazyfree is introduced but there is no callsite to use it in this patch.

Quoted 1 line

I don't think it's worth to separate.

I think it's more clear in this way, doing one thing in one patch.

mm/rmap.c +6 -0

@@ -1424,6 +1424,12 @@ static int try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
 				dec_mm_counter(mm, MM_ANONPAGES);
 				rp->lazyfreed++;
 				goto discard;
+			} else if (!PageSwapBacked(page)) {
+				/* dirty MADV_FREE page */
+				set_pte_at(mm, address, pvmw.pte, pteval);
+				ret = SWAP_DIRTY;
+				page_vma_mapped_walk_done(&pvmw);
+				break;
 			}
 
 			if (swap_duplicate(entry) < 0) {

Quoted 14 lines · Shaohua Li

mm/rmap.c +6 -0

@@ -1424,6 +1424,12 @@ static int try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
 				dec_mm_counter(mm, MM_ANONPAGES);
 				rp->lazyfreed++;
 				goto discard;
+			} else if (!PageSwapBacked(page)) {
+				/* dirty MADV_FREE page */
+				set_pte_at(mm, address, pvmw.pte, pteval);
+				ret = SWAP_DIRTY;
+				page_vma_mapped_walk_done(&pvmw);
+				break;
 			}
 
 			if (swap_duplicate(entry) < 0) {

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

Quoted 7 lines

mm/rmap.c +2 -2

@@ -1525,8 +1531,8 @@ int try_to_unmap(struct page *page, enum ttu_flags flags)
 
 	if (ret != SWAP_MLOCK && !page_mapcount(page)) {
 		ret = SWAP_SUCCESS;
-		if (rp.lazyfreed && !PageDirty(page))
-			ret = SWAP_LZFREE;
+		if (rp.lazyfreed && PageDirty(page))
+			ret = SWAP_DIRTY;

Can this actually happen? If the page is dirty, ret should already be SWAP_DIRTY, right? How would a dirty page get fully unmapped?

It seems to me rp.lazyfreed can be removed entirely now that we don't have to identify the lazyfree case anymore. The failure case is much easier to identify - all it takes is a single pte to be dirty.

When memory pressure is high, we free MADV_FREE pages. If the pages are not dirty in pte, the pages could be freed immediately. Otherwise we can't reclaim them. We put the pages back to anonumous LRU list (by setting SwapBacked flag) and the pages will be reclaimed in normal swapout way.

We use normal page reclaim policy. Since MADV_FREE pages are put into inactive file list, such pages and inactive file pages are reclaimed according to their age. This is expected, because we don't want to reclaim too many MADV_FREE pages before used once pages.

Based on Minchan's original patch

Cc Michal Hocko <[email protected]>

Cc Minchan Kim <[email protected]>

Cc Hugh Dickins <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc Rik van Riel <[email protected]>

Cc Mel Gorman <[email protected]>

Cc Andrew Morton <[email protected]>

Signed-off-by Shaohua Li <[email protected]>

---
 include/linux/rmap.h |  2 +-
 mm/huge_memory.c     |  2 ++
 mm/madvise.c         |  1 +
 mm/rmap.c            | 10 ++++++++--
 mm/vmscan.c          | 34 ++++++++++++++++++++++------------
 5 files changed, 34 insertions(+), 15 deletions(-)

include/linux/rmap.h +0 -0

index e2cd8f9..2bfd8c6 100644

mm/rmap.c +6 -0

index c621088..083f32e 100644
@@ -1424,6 +1424,12 @@ static int try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
 				dec_mm_counter(mm, MM_ANONPAGES);
 				rp->lazyfreed++;
 				goto discard;
+			} else if (!PageSwapBacked(page)) {
+				/* dirty MADV_FREE page */
+				set_pte_at(mm, address, pvmw.pte, pteval);
+				ret = SWAP_DIRTY;
+				page_vma_mapped_walk_done(&pvmw);
+				break;
 			}
 
 			if (swap_duplicate(entry) < 0) {

mm/rmap.c +0 -0

 	}
 	return ret;
 }

mm/vmscan.c +0 -0

index 68ea50d..830981a 100644

mm/vmscan.c +5 -2

 			goto keep_locked;
 
 		/* Double the slab pressure for mapped and swapcache pages */
-		if (page_mapped(page) || PageSwapCache(page))
+		if ((page_mapped(page) || PageSwapCache(page)) &&
+		    !(PageAnon(page) && !PageSwapBacked(page)))
 			sc->nr_scanned++;
 
 		may_enter_fs = (sc->gfp_mask & __GFP_FS) ||

@@ -1118,8 +1120,10 @@ static unsigned long shrink_page_list(struct list_head *page_list,
 		/*
 		 * Anonymous process memory has backing store?
 		 * Try to allocate it some swap space here.
+		 * Lazyfree page could be freed directly
 		 */
-		if (PageAnon(page) && !PageSwapCache(page)) {
+		if (PageAnon(page) && !PageSwapCache(page) &&
+		    PageSwapBacked(page)) {
 			if (!(sc->gfp_mask & __GFP_IO))
 				goto keep_locked;
 			if (!add_to_swap(page, page_list))

@@ -1140,9 +1144,12 @@ static unsigned long shrink_page_list(struct list_head *page_list,
 		 * The page is mapped into the page tables of one or more
 		 * processes. Try to unmap it here.
 		 */

Quoted 34 lines · Shaohua Li

When memory pressure is high, we free MADV_FREE pages. If the pages are not dirty in pte, the pages could be freed immediately. Otherwise we can't reclaim them. We put the pages back to anonumous LRU list (by setting SwapBacked flag) and the pages will be reclaimed in normal swapout way.

We use normal page reclaim policy. Since MADV_FREE pages are put into inactive file list, such pages and inactive file pages are reclaimed according to their age. This is expected, because we don't want to reclaim too many MADV_FREE pages before used once pages.

Based on Minchan's original patch

Cc Michal Hocko <[email protected]>

Cc Minchan Kim <[email protected]>

Cc Hugh Dickins <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc Rik van Riel <[email protected]>

Cc Mel Gorman <[email protected]>

Cc Andrew Morton <[email protected]>

Signed-off-by Shaohua Li <[email protected]>

---
 include/linux/rmap.h |  2 +-
 mm/huge_memory.c     |  2 ++
 mm/madvise.c         |  1 +
 mm/rmap.c            | 10 ++++++++--
 mm/vmscan.c          | 34 ++++++++++++++++++++++------------
 5 files changed, 34 insertions(+), 15 deletions(-)

include/linux/rmap.h +0 -0

index e2cd8f9..2bfd8c6 100644

include/linux/rmap.h static inline int page_mkclean(struct page *page) · line 303

Quoted 5 lines

include/linux/rmap.h +1 -1

@@ -300,6 +300,6 @@ static inline int page_mkclean(struct page *page)
 #define SWAP_AGAIN	1
 #define SWAP_FAIL	2
 #define SWAP_MLOCK	3
-#define SWAP_LZFREE	4
+#define SWAP_DIRTY	4

Could you write down about SWAP_DIRTY in try_to_unmap's description?

< snip >

Quoted 19 lines

mm/rmap.c +6 -0

index c621088..083f32e 100644
@@ -1424,6 +1424,12 @@ static int try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
 				dec_mm_counter(mm, MM_ANONPAGES);
 				rp->lazyfreed++;
 				goto discard;
+			} else if (!PageSwapBacked(page)) {
+				/* dirty MADV_FREE page */
+				set_pte_at(mm, address, pvmw.pte, pteval);
+				ret = SWAP_DIRTY;
+				page_vma_mapped_walk_done(&pvmw);
+				break;
 			}
 
 			if (swap_duplicate(entry) < 0) {

@@ -1525,8 +1531,8 @@ int try_to_unmap(struct page *page, enum ttu_flags flags)
 

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

Quoted 6 lines

mm/rmap.c +2 -2

 	if (ret != SWAP_MLOCK && !page_mapcount(page)) {
 		ret = SWAP_SUCCESS;
-		if (rp.lazyfreed && !PageDirty(page))
-			ret = SWAP_LZFREE;
+		if (rp.lazyfreed && PageDirty(page))
+			ret = SWAP_DIRTY;

Hmm, I don't understand why we need to introduce new return value. Can't we set SetPageSwapBacked and return SWAP_FAIL in try_to_unmap_one?

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

Quoted 8 lines

mm/rmap.c +2 -2

@@ -1525,8 +1531,8 @@ int try_to_unmap(struct page *page, enum ttu_flags flags)
 
 	if (ret != SWAP_MLOCK && !page_mapcount(page)) {
 		ret = SWAP_SUCCESS;
-		if (rp.lazyfreed && !PageDirty(page))
-			ret = SWAP_LZFREE;
+		if (rp.lazyfreed && PageDirty(page))
+			ret = SWAP_DIRTY;

Hmm, I don't understand why we need to introduce new return value. Can't we set SetPageSwapBacked and return SWAP_FAIL in try_to_unmap_one?

Quoted 11 lines · Minchan Kim

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

Quoted 8 lines

mm/rmap.c +2 -2

@@ -1525,8 +1531,8 @@ int try_to_unmap(struct page *page, enum ttu_flags flags)
 
 	if (ret != SWAP_MLOCK && !page_mapcount(page)) {
 		ret = SWAP_SUCCESS;
-		if (rp.lazyfreed && !PageDirty(page))
-			ret = SWAP_LZFREE;
+		if (rp.lazyfreed && PageDirty(page))
+			ret = SWAP_DIRTY;

Hmm, I don't understand why we need to introduce new return value. Can't we set SetPageSwapBacked and return SWAP_FAIL in try_to_unmap_one?

I think that's a bad idea. A function called "try_to_unmap" shouldn't have as a side effect that it changes the page's LRU type in an error case. Let try_to_unmap be about unmapping the page. If it fails, make it report why and let the caller deal with the fallout. Any LRU fixups are much better placed in vmscan.c.

show MADV_FREE pages info of each vma in smaps. The interface is for diganose or monitoring purpose, userspace could use it to understand what happens in the application. Since userspace could dirty MADV_FREE pages without notice from kernel, this interface is the only place we can get accurate accounting info about MADV_FREE pages.

Cc Michal Hocko <[email protected]>

Cc Minchan Kim <[email protected]>

Cc Hugh Dickins <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc Rik van Riel <[email protected]>

Cc Mel Gorman <[email protected]>

Cc Andrew Morton <[email protected]>

Signed-off-by Shaohua Li <[email protected]>

show MADV_FREE pages info of each vma in smaps. The interface is for diganose or monitoring purpose, userspace could use it to understand what happens in the application. Since userspace could dirty MADV_FREE pages without notice from kernel, this interface is the only place we can get accurate accounting info about MADV_FREE pages.

Cc Michal Hocko <[email protected]>

Cc Minchan Kim <[email protected]>

Cc Hugh Dickins <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc Rik van Riel <[email protected]>

Cc Mel Gorman <[email protected]>

Cc Andrew Morton <[email protected]>

Signed-off-by Shaohua Li <[email protected]>

fs/proc/task_mmu.c static int show_smap(struct seq_file *m, void *v, int is_pid) · line 780

Quoted 8 lines · Shaohua Li

fs/proc/task_mmu.c +1 -0

@@ -770,6 +774,7 @@ static int show_smap(struct seq_file *m, void *v, int is_pid)
 		   "Private_Dirty:  %8lu kB\n"
 		   "Referenced:     %8lu kB\n"
 		   "Anonymous:      %8lu kB\n"
+		   "LazyFree:       %8lu kB\n"
 		   "AnonHugePages:  %8lu kB\n"
 		   "ShmemPmdMapped: %8lu kB\n"
 		   "Shared_Hugetlb: %8lu kB\n"

I've been as guily of this in the past as anyone, but are we just going to keep adding fields to smaps forever? For the vast, vast, majority of folks, this will simply waste the 21 bytes * nr_vmas that it costs us to
print "LazyFree: 0 kB\n" over and over.

Should we maybe start a habit of not printing an entry when it's "0 kB"?