mm: free swp_entry in madvise_free

-----Original Message-----
From: Minchan Kim [mailto:[email protected]]
Sent: Wednesday, March 11, 2015 9:21 AM
To: Andrew Morton
Cc: [email protected]; [email protected]; Michal Hocko; Johannes Weiner; Mel Gorman; Rik van Riel; Shaohua Li; Wang, Yalin; Minchan Kim
Subject: [PATCH 3/4] mm: move lazy free pages to inactive list

MADV_FREE is hint that it's okay to discard pages if there is memory pressure and we uses reclaimers(ie, kswapd and direct reclaim) to free them so there is no worth to remain them in active anonymous LRU so this patch moves them to inactive LRU list's head.

This means that MADV_FREE-ed pages which were living on the inactive list are reclaimed first because they are more likely to be cold rather than recently active pages.

A arguable issue for the approach would be whether we should put it to head or tail in inactive list. I selected *head* because kernel cannot make sure it's really cold or warm for every MADV_FREE usecase but at least we know it's not *hot* so landing of inactive head would be comprimise for various usecases.

This is fixing a suboptimal behavior of MADV_FREE when pages living on the active list will sit there for a long time even under memory pressure while the inactive list is reclaimed heavily. This basically breaks the whole purpose of using MADV_FREE to help the system to free memory which is might not be used.

Acked-by Michal Hocko <[email protected]>

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

---
 include/linux/swap.h |  1 +
 mm/madvise.c         |  2 ++
 mm/swap.c            | 35 +++++++++++++++++++++++++++++++++++
 3 files changed, 38 insertions(+)

include/linux/swap.h +1 -0

index cee108c..0428e4c 100644
@@ -308,6 +308,7 @@ extern void lru_add_drain_cpu(int cpu);
 extern void lru_add_drain_all(void);
 extern void rotate_reclaimable_page(struct page *page);
 extern void deactivate_file_page(struct page *page);
+extern void deactivate_page(struct page *page);
 extern void swap_setup(void);
 
 extern void add_page_to_unevictable_list(struct page *page);

mm/madvise.c +0 -0

index ebe692e..22e8f0c 100644

Quoted 61 lines

-----Original Message-----
From: Minchan Kim [mailto:[email protected]]
Sent: Wednesday, March 11, 2015 9:21 AM
To: Andrew Morton
Cc: [email protected]; [email protected]; Michal Hocko; Johannes Weiner; Mel Gorman; Rik van Riel; Shaohua Li; Wang, Yalin; Minchan Kim
Subject: [PATCH 3/4] mm: move lazy free pages to inactive list

MADV_FREE is hint that it's okay to discard pages if there is memory pressure and we uses reclaimers(ie, kswapd and direct reclaim) to free them so there is no worth to remain them in active anonymous LRU so this patch moves them to inactive LRU list's head.

This means that MADV_FREE-ed pages which were living on the inactive list are reclaimed first because they are more likely to be cold rather than recently active pages.

A arguable issue for the approach would be whether we should put it to head or tail in inactive list. I selected *head* because kernel cannot make sure it's really cold or warm for every MADV_FREE usecase but at least we know it's not *hot* so landing of inactive head would be comprimise for various usecases.

This is fixing a suboptimal behavior of MADV_FREE when pages living on the active list will sit there for a long time even under memory pressure while the inactive list is reclaimed heavily. This basically breaks the whole purpose of using MADV_FREE to help the system to free memory which is might not be used.

Acked-by Michal Hocko <[email protected]>

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

---
 include/linux/swap.h |  1 +
 mm/madvise.c         |  2 ++
 mm/swap.c            | 35 +++++++++++++++++++++++++++++++++++
 3 files changed, 38 insertions(+)

include/linux/swap.h +1 -0

index cee108c..0428e4c 100644
@@ -308,6 +308,7 @@ extern void lru_add_drain_cpu(int cpu);
 extern void lru_add_drain_all(void);
 extern void rotate_reclaimable_page(struct page *page);
 extern void deactivate_file_page(struct page *page);
+extern void deactivate_page(struct page *page);
 extern void swap_setup(void);
 
 extern void add_page_to_unevictable_list(struct page *page);

mm/madvise.c +2 -0

index ebe692e..22e8f0c 100644
@@ -340,6 +340,8 @@ static int madvise_free_pte_range(pmd_t *pmd, unsigned
 long addr,
 		ptent = pte_mkold(ptent);
 		ptent = pte_mkclean(ptent);
 		set_pte_at(mm, addr, pte, ptent);
+		if (PageActive(page))
+			deactivate_page(page);

mm/madvise.c static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr, · line 346

Quoted 2 lines

mm/madvise.c +0 -0

 		tlb_remove_tlb_entry(tlb, pte, addr);
 	}

I think this place should be changed like this:
+ if (!page_referenced(page, false, NULL, NULL, NULL) && PageActive(page))
+ deactivate_page(page);
Because we don't know if other processes are reference this page, If it is true, don't need deactivate this page.

Quoted 61 lines

-----Original Message-----
From: Minchan Kim [mailto:[email protected]]
Sent: Wednesday, March 11, 2015 9:21 AM
To: Andrew Morton
Cc: [email protected]; [email protected]; Michal Hocko; Johannes Weiner; Mel Gorman; Rik van Riel; Shaohua Li; Wang, Yalin; Minchan Kim
Subject: [PATCH 3/4] mm: move lazy free pages to inactive list

MADV_FREE is hint that it's okay to discard pages if there is memory pressure and we uses reclaimers(ie, kswapd and direct reclaim) to free them so there is no worth to remain them in active anonymous LRU so this patch moves them to inactive LRU list's head.

This means that MADV_FREE-ed pages which were living on the inactive list are reclaimed first because they are more likely to be cold rather than recently active pages.

A arguable issue for the approach would be whether we should put it to head or tail in inactive list. I selected *head* because kernel cannot make sure it's really cold or warm for every MADV_FREE usecase but at least we know it's not *hot* so landing of inactive head would be comprimise for various usecases.

This is fixing a suboptimal behavior of MADV_FREE when pages living on the active list will sit there for a long time even under memory pressure while the inactive list is reclaimed heavily. This basically breaks the whole purpose of using MADV_FREE to help the system to free memory which is might not be used.

Acked-by Michal Hocko <[email protected]>

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

---
 include/linux/swap.h |  1 +
 mm/madvise.c         |  2 ++
 mm/swap.c            | 35 +++++++++++++++++++++++++++++++++++
 3 files changed, 38 insertions(+)

include/linux/swap.h +1 -0

index cee108c..0428e4c 100644
@@ -308,6 +308,7 @@ extern void lru_add_drain_cpu(int cpu);
 extern void lru_add_drain_all(void);
 extern void rotate_reclaimable_page(struct page *page);
 extern void deactivate_file_page(struct page *page);
+extern void deactivate_page(struct page *page);
 extern void swap_setup(void);
 
 extern void add_page_to_unevictable_list(struct page *page);

mm/madvise.c +2 -0

index ebe692e..22e8f0c 100644
@@ -340,6 +340,8 @@ static int madvise_free_pte_range(pmd_t *pmd, unsigned
 long addr,
 		ptent = pte_mkold(ptent);
 		ptent = pte_mkclean(ptent);
 		set_pte_at(mm, addr, pte, ptent);
+		if (PageActive(page))
+			deactivate_page(page);

mm/madvise.c static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr, · line 346

Quoted 2 lines

mm/madvise.c +0 -0

 		tlb_remove_tlb_entry(tlb, pte, addr);
 	}

I think this place should be changed like this:
+ if (!page_referenced(page, false, NULL, NULL, NULL) && PageActive(page))
+ deactivate_page(page);
Because we don't know if other processes are reference this page, If it is true, don't need deactivate this page.

MADV_FREE is hint that it's okay to discard pages if there is memory pressure and we uses reclaimers(ie, kswapd and direct reclaim) to free them so there is no worth to remain them in active anonymous LRU so this patch moves them to inactive LRU list's head.

This means that MADV_FREE-ed pages which were living on the inactive list are reclaimed first because they are more likely to be cold rather than recently active pages.

A arguable issue for the approach would be whether we should put it to head or tail in inactive list. I selected *head* because kernel cannot make sure it's really cold or warm for every MADV_FREE usecase but at least we know it's not *hot* so landing of inactive head would be comprimise for various usecases.

This is fixing a suboptimal behavior of MADV_FREE when pages living on the active list will sit there for a long time even under memory pressure while the inactive list is reclaimed heavily. This basically breaks the whole purpose of using MADV_FREE to help the system to free memory which is might not be used.

mm/swap.c +3 -0

@@ -789,6 +790,23 @@ static void lru_deactivate_file_fn(struct page *page, struct lruvec *lruvec,
 	update_page_reclaim_stat(lruvec, file, 0);
 }
 
+
+static void lru_deactivate_fn(struct page *page, struct lruvec *lruvec,
+			    void *arg)
 
...

Quoted 35 lines · On Wed, 11 Mar 2015 10:20:37 +0900 Minchan Kim <[email protected]> wrote:

MADV_FREE is hint that it's okay to discard pages if there is memory pressure and we uses reclaimers(ie, kswapd and direct reclaim) to free them so there is no worth to remain them in active anonymous LRU so this patch moves them to inactive LRU list's head.

This means that MADV_FREE-ed pages which were living on the inactive list are reclaimed first because they are more likely to be cold rather than recently active pages.

A arguable issue for the approach would be whether we should put it to head or tail in inactive list. I selected *head* because kernel cannot make sure it's really cold or warm for every MADV_FREE usecase but at least we know it's not *hot* so landing of inactive head would be comprimise for various usecases.

This is fixing a suboptimal behavior of MADV_FREE when pages living on the active list will sit there for a long time even under memory pressure while the inactive list is reclaimed heavily. This basically breaks the whole purpose of using MADV_FREE to help the system to free memory which is might not be used.

(quoted diff) +3 -0

@@ -789,6 +790,23 @@ static void lru_deactivate_file_fn(struct page *page, struct lruvec *lruvec,
 	update_page_reclaim_stat(lruvec, file, 0);
 }
 
+
+static void lru_deactivate_fn(struct page *page, struct lruvec *lruvec,
+			    void *arg)
 
...

@@ -844,6 +866,18 @@ void deactivate_file_page(struct page *page)
 	}
 }
 

Review void deactivate_file_page(struct page *page) · line 870

Quoted 2 lines

(quoted diff) +2 -0

+void deactivate_page(struct page *page)
+{

lru_deactivate_file_fn() and deactivate_file_page() are carefully documented and lru_deactivate_fn() and deactivate_page() should be as well. In fact it becomes more important now that we have two similar-looking things.

mm/swap.c +0 -0

@@ -866,6 +866,13 @@ void deactivate_file_page(struct page *page)
 	}
 }
 

Quoted 6 lines · On Mon, 30 Mar 2015 14:35:02 +0900 Minchan Kim <[email protected]> wrote:

mm/swap.c +0 -0

@@ -866,6 +866,13 @@ void deactivate_file_page(struct page *page)
 	}
 }
 

mm/swap.c void deactivate_file_page(struct page *page) · line 878

Quoted 10 lines

mm/swap.c +7 -0

+/**
+ * deactivate_page - deactivate a page
+ * @page: page to deactivate
+ *
+ * This function moves @page to inactive list if @page was on active list and
+ * was not unevictable page to accelerate to reclaim @page.
+ */
 void deactivate_page(struct page *page)
 {
 	if (PageLRU(page) && PageActive(page) && !PageUnevictable(page)) {

Thanks.

deactivate_page() doesn't look at or alter PageReferenced(). Should it?

Quoted 2 lines

deactivate_page() doesn't look at or alter PageReferenced(). Should it?

Absolutely true. Thanks.
Here it goes.

Quoted 1 line

From 2b2c92eb73a1cceac615b9abd4c0f5f0c3395ff5 Mon Sep 17 00:00:00 2001

From: Minchan Kim <[email protected]>
Date: Tue, 31 Mar 2015 13:38:46 +0900
Subject: [PATCH] mm: lru_deactivate_fn should clear PG_referenced

deactivate_page aims for accelerate for reclaiming through moving pages from active list to inactive list so we should clear PG_referenced for the goal.

...

Quoted 26 lines · On Tue, 31 Mar 2015 13:45:25 +0900 Minchan Kim <[email protected]> wrote:

Quoted 2 lines

deactivate_page() doesn't look at or alter PageReferenced(). Should it?

Absolutely true. Thanks.
Here it goes.

Quoted 1 line

From 2b2c92eb73a1cceac615b9abd4c0f5f0c3395ff5 Mon Sep 17 00:00:00 2001

From: Minchan Kim <[email protected]>
Date: Tue, 31 Mar 2015 13:38:46 +0900
Subject: [PATCH] mm: lru_deactivate_fn should clear PG_referenced

deactivate_page aims for accelerate for reclaiming through moving pages from active list to inactive list so we should clear PG_referenced for the goal.

...

mm/swap.c +1 -0

@@ -800,6 +800,7 @@ static void lru_deactivate_fn(struct page *page, struct lruvec *lruvec,
 
 		del_page_from_lru_list(page, lruvec, lru + LRU_ACTIVE);
 		ClearPageActive(page);
+		ClearPageReferenced(page);
 		add_page_to_lru_list(page, lruvec, lru);
 

mm/swap.c static void lru_deactivate_fn(struct page *page, struct lruvec *lruvec, · line 806

Quoted 1 line

mm/swap.c +0 -0

 		__count_vm_event(PGDEACTIVATE);

What if we have

	PageLRU(page) && !PageActive(page) && PageReferenced(page)

if we really want to "accelerate the reclaim of @page" then we should clear PG_referenced there too.

(And what about page_referenced(page) :))

2nd description trial.

Quoted 1 line · Minchan Kim

2nd description trial.

...
Hi Minchan, could you please point for which repo this patch, linux-next?

Hi Cyrill,

Quoted 2 lines · Cyrill Gorcunov

Quoted 1 line · Minchan Kim

2nd description trial.

Quoted 3 lines

...
Hi Minchan, could you please point for which repo this patch, linux-next?

It was based on v4.0-rc5-mmotm-2015-03-24-17-02. As well, I confirmed it was applied on local-next-20150327.

Thanks.

Quoted 9 lines · Minchan Kim

Hi Cyrill,

Quoted 5 lines · Cyrill Gorcunov

Quoted 1 line · Minchan Kim

2nd description trial.

...
Hi Minchan, could you please point for which repo this patch, linux-next?

Quoted 4 lines

It was based on v4.0-rc5-mmotm-2015-03-24-17-02. As well, I confirmed it was applied on local-next-20150327.

Thanks.

Hi Minchan! I managed to fetch mmotm and the change looks reasonable to me. Still better to wait for review from Mel or Hugh, maybe I miss something obvious.

Bascially, MADV_FREE relys on the pte dirty to decide whether it allows VM to discard the page. However, if there is swap-in, pte pointed out the page has no pte_dirty. So, MADV_FREE checks PageDirty and PageSwapCache for those pages to not discard it because swapped-in page could live on swap cache or PageDirty when it is removed from swapcache.

The problem in here is that anonymous pages can have PageDirty if it is removed from swapcache so that VM cannot parse those pages as freeable even if we did madvise_free. Look at below example.

ptr = malloc();
memset(ptr);
..
heavy memory pressure -> swap-out all of pages
..
out of memory pressure so there are lots of free pages
..
var = *ptr; -> swap-in page/remove the page from swapcache. so pte_clean
but SetPageDirty

madvise_free(ptr);
..
..
heavy memory pressure -> VM cannot discard the page by PageDirty.

PageDirty for anonymous page aims for avoiding duplicating swapping out. In other words, if a page have swapped-in but live swapcache(ie, !PageDirty), we could save swapout if the page is selected as victim by VM in future because swap device have kept previous swapped-out contents of the page.

So, rather than relying on the PG_dirty for working madvise_free, pte_dirty is more straightforward. Inherently, swapped-out page was pte_dirty so this patch restores the dirtiness when swap-in fault happens so madvise_free doesn't rely on the PageDirty any more.

Cc Hugh Dickins <[email protected]>

Cc Cyrill Gorcunov <[email protected]>

Cc Pavel Emelyanov <[email protected]>

Reported-by Yalin Wang <[email protected]>

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

---
 mm/madvise.c | 1 -
 mm/memory.c  | 9 +++++++--
 mm/rmap.c    | 2 +-
 mm/vmscan.c  | 3 +--
 4 files changed, 9 insertions(+), 6 deletions(-)

mm/madvise.c +0 -1

index 22e8f0c..a045798 100644
@@ -325,7 +325,6 @@ static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
 				continue;
 			}
 
-			ClearPageDirty(page);
 			unlock_page(page);
 		}
 

mm/memory.c +7 -2

index 0f96a4a..40428a5 100644
@@ -2521,9 +2521,14 @@ static int do_swap_page(struct mm_struct *mm, struct vm_area_struct *vma,
 
 	inc_mm_counter_fast(mm, MM_ANONPAGES);
 	dec_mm_counter_fast(mm, MM_SWAPENTS);
-	pte = mk_pte(page, vma->vm_page_prot);
+
+	/*
+	 * Every page swapped-out was pte_dirty so we make pte dirty again.
+	 * MADV_FREE relies on it.
+	 */
+	pte = pte_mkdirty(mk_pte(page, vma->vm_page_prot));
 	if ((flags & FAULT_FLAG_WRITE) && reuse_swap_page(page)) {
-		pte = maybe_mkwrite(pte_mkdirty(pte), vma);
+		pte = maybe_mkwrite(pte, vma);
 		flags &= ~FAULT_FLAG_WRITE;
 		ret |= VM_FAULT_WRITE;
 		exclusive = 1;

mm/rmap.c +1 -1

index 47b3ba8..34c1d66 100644
@@ -1268,7 +1268,7 @@ static int try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
 
 		if (flags & TTU_FREE) {
 			VM_BUG_ON_PAGE(PageSwapCache(page), page);
-			if (!dirty && !PageDirty(page)) {
+			if (!dirty) {
 				/* It's a freeable page by MADV_FREE */
 				dec_mm_counter(mm, MM_ANONPAGES);
 				goto discard;

mm/vmscan.c +1 -2

index 260c413..3357ffa 100644
@@ -805,8 +805,7 @@ static enum page_references page_check_references(struct page *page,
 		return PAGEREF_KEEP;
 	}
 
-	if (PageAnon(page) && !pte_dirty && !PageSwapCache(page) &&
-			!PageDirty(page))
+	if (PageAnon(page) && !pte_dirty && !PageSwapCache(page))
 		*freeable = true;
 
 	/* Reclaim if clean, defer dirty pages to writeback */

Signature · 1 line

1.9.3

Bump.

Quoted 1 line · On Thu, 9 Apr 2015 08:50:25 +0900 Minchan Kim <[email protected]> wrote:

Bump.

I'm getting the feeling that MADV_FREE is out of control.

Below is the overall rollup of

mm-support-madvisemadv_free.patch mm-support-madvisemadv_free-fix.patch mm-support-madvisemadv_free-fix-2.patch mm-dont-split-thp-page-when-syscall-is-called.patch mm-dont-split-thp-page-when-syscall-is-called-fix.patch mm-dont-split-thp-page-when-syscall-is-called-fix-2.patch mm-free-swp_entry-in-madvise_free.patch mm-move-lazy-free-pages-to-inactive-list.patch mm-move-lazy-free-pages-to-inactive-list-fix.patch mm-move-lazy-free-pages-to-inactive-list-fix-fix.patch mm-move-lazy-free-pages-to-inactive-list-fix-fix-fix.patch mm-make-every-pte-dirty-on-do_swap_page.patch

It's pretty large and has its sticky little paws in all sorts of places.

The feature would need to be pretty darn useful to justify a mainline merge. Has any such usefulness been demonstrated?

Quoted 1 line · On Thu, 9 Apr 2015 08:50:25 +0900 Minchan Kim <[email protected]> wrote:

Bump.

I'm getting the feeling that MADV_FREE is out of control.

Below is the overall rollup of

mm-support-madvisemadv_free.patch mm-support-madvisemadv_free-fix.patch mm-support-madvisemadv_free-fix-2.patch mm-dont-split-thp-page-when-syscall-is-called.patch mm-dont-split-thp-page-when-syscall-is-called-fix.patch mm-dont-split-thp-page-when-syscall-is-called-fix-2.patch mm-free-swp_entry-in-madvise_free.patch mm-move-lazy-free-pages-to-inactive-list.patch mm-move-lazy-free-pages-to-inactive-list-fix.patch mm-move-lazy-free-pages-to-inactive-list-fix-fix.patch mm-move-lazy-free-pages-to-inactive-list-fix-fix-fix.patch mm-make-every-pte-dirty-on-do_swap_page.patch

It's pretty large and has its sticky little paws in all sorts of places.

The feature would need to be pretty darn useful to justify a mainline merge. Has any such usefulness been demonstrated?

Bascially, MADV_FREE relys on the pte dirty to decide whether it allows VM to discard the page. However, if there is swap-in, pte pointed out the page has no pte_dirty. So, MADV_FREE checks PageDirty and PageSwapCache for those pages to not discard it because swapped-in page could live on swap cache or PageDirty when it is removed from swapcache.

The problem in here is that anonymous pages can have PageDirty if it is removed from swapcache so that VM cannot parse those pages as freeable even if we did madvise_free. Look at below example.

ptr = malloc();
memset(ptr);
..
heavy memory pressure -> swap-out all of pages
..
out of memory pressure so there are lots of free pages
..
var = *ptr; -> swap-in page/remove the page from swapcache. so pte_clean
but SetPageDirty

madvise_free(ptr);
..
..
heavy memory pressure -> VM cannot discard the page by PageDirty.

PageDirty for anonymous page aims for avoiding duplicating swapping out. In other words, if a page have swapped-in but live swapcache(ie, !PageDirty), we could save swapout if the page is selected as victim by VM in future because swap device have kept previous swapped-out contents of the page.

So, rather than relying on the PG_dirty for working madvise_free, pte_dirty is more straightforward. Inherently, swapped-out page was pte_dirty so this patch restores the dirtiness when swap-in fault happens so madvise_free doesn't rely on the PageDirty any more.

Cc Hugh Dickins <[email protected]>

Cc Cyrill Gorcunov <[email protected]>

Cc Pavel Emelyanov <[email protected]>

Reported-by Yalin Wang <[email protected]>

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

---
 mm/madvise.c | 1 -
 mm/memory.c  | 9 +++++++--
 mm/rmap.c    | 2 +-
 mm/vmscan.c  | 3 +--
 4 files changed, 9 insertions(+), 6 deletions(-)

mm/madvise.c +0 -1

index 22e8f0c..a045798 100644
@@ -325,7 +325,6 @@ static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
 				continue;
 			}
 
-			ClearPageDirty(page);
 			unlock_page(page);
 		}
 

mm/memory.c +7 -2

index 0f96a4a..40428a5 100644
@@ -2521,9 +2521,14 @@ static int do_swap_page(struct mm_struct *mm, struct vm_area_struct *vma,
 
 	inc_mm_counter_fast(mm, MM_ANONPAGES);
 	dec_mm_counter_fast(mm, MM_SWAPENTS);
-	pte = mk_pte(page, vma->vm_page_prot);
+
+	/*
+	 * Every page swapped-out was pte_dirty so we make pte dirty again.
+	 * MADV_FREE relies on it.
+	 */
+	pte = pte_mkdirty(mk_pte(page, vma->vm_page_prot));
 	if ((flags & FAULT_FLAG_WRITE) && reuse_swap_page(page)) {
-		pte = maybe_mkwrite(pte_mkdirty(pte), vma);
+		pte = maybe_mkwrite(pte, vma);
 		flags &= ~FAULT_FLAG_WRITE;
 		ret |= VM_FAULT_WRITE;
 		exclusive = 1;

mm/rmap.c +1 -1

index 47b3ba8..34c1d66 100644
@@ -1268,7 +1268,7 @@ static int try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
 
 		if (flags & TTU_FREE) {
 			VM_BUG_ON_PAGE(PageSwapCache(page), page);
-			if (!dirty && !PageDirty(page)) {
+			if (!dirty) {
 				/* It's a freeable page by MADV_FREE */
 				dec_mm_counter(mm, MM_ANONPAGES);
 				goto discard;

mm/vmscan.c +1 -2

index 260c413..3357ffa 100644
@@ -805,8 +805,7 @@ static enum page_references page_check_references(struct page *page,
 		return PAGEREF_KEEP;
 	}
 
-	if (PageAnon(page) && !pte_dirty && !PageSwapCache(page) &&
-			!PageDirty(page))
+	if (PageAnon(page) && !pte_dirty && !PageSwapCache(page))
 		*freeable = true;
 
 	/* Reclaim if clean, defer dirty pages to writeback */

Signature · 1 line

1.9.3

Quoted 37 lines · Minchan Kim

Bascially, MADV_FREE relys on the pte dirty to decide whether it allows VM to discard the page. However, if there is swap-in, pte pointed out the page has no pte_dirty. So, MADV_FREE checks PageDirty and PageSwapCache for those pages to not discard it because swapped-in page could live on swap cache or PageDirty when it is removed from swapcache.

The problem in here is that anonymous pages can have PageDirty if it is removed from swapcache so that VM cannot parse those pages as freeable even if we did madvise_free. Look at below example.

ptr = malloc();
memset(ptr);
..
heavy memory pressure -> swap-out all of pages
..
out of memory pressure so there are lots of free pages
..
var = *ptr; -> swap-in page/remove the page from swapcache. so pte_clean
but SetPageDirty

madvise_free(ptr);
..
..
heavy memory pressure -> VM cannot discard the page by PageDirty.

PageDirty for anonymous page aims for avoiding duplicating swapping out. In other words, if a page have swapped-in but live swapcache(ie, !PageDirty), we could save swapout if the page is selected as victim by VM in future because swap device have kept previous swapped-out contents of the page.

So, rather than relying on the PG_dirty for working madvise_free, pte_dirty is more straightforward. Inherently, swapped-out page was pte_dirty so this patch restores the dirtiness when swap-in fault happens so madvise_free doesn't rely on the PageDirty any more.

Quoted 5 lines

Cc Hugh Dickins <[email protected]>

Cc Cyrill Gorcunov <[email protected]>

Cc Pavel Emelyanov <[email protected]>

Reported-by Yalin Wang <[email protected]>

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

Sorry, but NAK to this patch, mm-make-every-pte-dirty-on-do_swap_page.patch in akpm's mm tree (I hope it hasn't reached linux-next yet).

You may well be right that pte_dirty<->PageDirty can be handled differently, in a way more favourable to MADV_FREE. And this patch may be a step in the right direction, but I've barely given it thought.

As it stands, it segfaults more than any patch I've seen in years:
I just tried applying it to 4.0-rc7-mm1, and running kernel builds in low memory with swap. Even if I leave KSM out, and memcg out, and swapoff out, and THP out, and tmpfs out, it still SIGSEGVs very soon.

I have a choice: spend a few hours tracking down the errors, and post a fix patch on top of yours? But even then I'd want to spend a lot longer thinking through every dirty/Dirty in the source before I'd feel comfortable to give an ack.

This is users' data, and we need to be very careful with it: errors in MADV_FREE are one thing, for now that's easy to avoid; but in this patch you're changing the rules for Anon PageDirty for everyone.

I think for now I'll have to leave it to you to do much more source diligence and testing, before coming back with a corrected patch for us then to review, slowly and carefully.

Hello Hugh,

Quoted 63 lines · Hugh Dickins

Quoted 37 lines · Minchan Kim

Bascially, MADV_FREE relys on the pte dirty to decide whether it allows VM to discard the page. However, if there is swap-in, pte pointed out the page has no pte_dirty. So, MADV_FREE checks PageDirty and PageSwapCache for those pages to not discard it because swapped-in page could live on swap cache or PageDirty when it is removed from swapcache.

The problem in here is that anonymous pages can have PageDirty if it is removed from swapcache so that VM cannot parse those pages as freeable even if we did madvise_free. Look at below example.

ptr = malloc();
memset(ptr);
..
heavy memory pressure -> swap-out all of pages
..
out of memory pressure so there are lots of free pages
..
var = *ptr; -> swap-in page/remove the page from swapcache. so pte_clean
but SetPageDirty

madvise_free(ptr);
..
..
heavy memory pressure -> VM cannot discard the page by PageDirty.

PageDirty for anonymous page aims for avoiding duplicating swapping out. In other words, if a page have swapped-in but live swapcache(ie, !PageDirty), we could save swapout if the page is selected as victim by VM in future because swap device have kept previous swapped-out contents of the page.

So, rather than relying on the PG_dirty for working madvise_free, pte_dirty is more straightforward. Inherently, swapped-out page was pte_dirty so this patch restores the dirtiness when swap-in fault happens so madvise_free doesn't rely on the PageDirty any more.

Quoted 5 lines

Cc Hugh Dickins <[email protected]>

Cc Cyrill Gorcunov <[email protected]>

Cc Pavel Emelyanov <[email protected]>

Reported-by Yalin Wang <[email protected]>

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

Sorry, but NAK to this patch, mm-make-every-pte-dirty-on-do_swap_page.patch in akpm's mm tree (I hope it hasn't reached linux-next yet).

You may well be right that pte_dirty<->PageDirty can be handled differently, in a way more favourable to MADV_FREE. And this patch may be a step in the right direction, but I've barely given it thought.

As it stands, it segfaults more than any patch I've seen in years:
I just tried applying it to 4.0-rc7-mm1, and running kernel builds in low memory with swap. Even if I leave KSM out, and memcg out, and swapoff out, and THP out, and tmpfs out, it still SIGSEGVs very soon.

I have a choice: spend a few hours tracking down the errors, and post a fix patch on top of yours? But even then I'd want to spend a lot longer thinking through every dirty/Dirty in the source before I'd feel comfortable to give an ack.

Quoted 7 lines

This is users' data, and we need to be very careful with it: errors in MADV_FREE are one thing, for now that's easy to avoid; but in this patch you're changing the rules for Anon PageDirty for everyone.

I think for now I'll have to leave it to you to do much more source diligence and testing, before coming back with a corrected patch for us then to review, slowly and carefully.

Sorry for my bad. I will keep your advise in mind. I will investigate the problem as soon as I get back to work after vacation.

Thanks for the the review.

Signature · 3 lines

-- 
Kind regards,
Minchan Kim

When I test below piece of code with 12 processes(ie, 512M * 12 = 6G consume) on my (3G ram + 12 cpu + 8G swap, the madvise_free is siginficat slower (ie, 2x times) than madvise_dontneed.

loop = 5;
mmap(512M);

while (loop--) {
        memset(512M);
        madvise(MADV_FREE or MADV_DONTNEED);
}

The reason is lots of swapin.

1) dontneed: 1,612 swapin
2) madvfree: 879,585 swapin

If we find hinted pages were already swapped out when syscall is called, it's pointless to keep the swapped-out pages in pte. Instead, let's free the cold page because swapin is more expensive than (alloc page + zeroing).

With this patch, it reduced swapin from 879,585 to 1,878 so elapsed time

1) dontneed: 6.10user 233.50system 0:50.44elapsed
2) madvfree: 6.03user 401.17system 1:30.67elapsed
2) madvfree + below patch: 6.70user 339.14system 1:04.45elapsed

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

---
 mm/madvise.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

mm/madvise.c +25 -1

index 6d0fcb8..ebe692e 100644
@@ -274,7 +274,9 @@ static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
 	spinlock_t *ptl;
 	pte_t *pte, ptent;
 	struct page *page;
+	swp_entry_t entry;
 	unsigned long next;
+	int nr_swap = 0;
 
 	next = pmd_addr_end(addr, end);
 	if (pmd_trans_huge(*pmd)) {

@@ -293,8 +295,22 @@ static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
 	for (; addr != end; pte++, addr += PAGE_SIZE) {
 		ptent = *pte;
 
-		if (!pte_present(ptent))
+		if (pte_none(ptent))
 			continue;
+		/*
+		 * If the pte has swp_entry, just clear page table to
+		 * prevent swap-in which is more expensive rather than
+		 * (page allocation + zeroing).
+		 */
+		if (!pte_present(ptent)) {
+			entry = pte_to_swp_entry(ptent);
+			if (non_swap_entry(entry))
+				continue;
+			nr_swap--;
+			free_swap_and_cache(entry);
+			pte_clear_not_present_full(mm, addr, pte, tlb->fullmm);
+			continue;
+		}
 
 		page = vm_normal_page(vma, addr, ptent);
 		if (!page)

@@ -326,6 +342,14 @@ static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
 		set_pte_at(mm, addr, pte, ptent);
 		tlb_remove_tlb_entry(tlb, pte, addr);
 	}
+
+	if (nr_swap) {
+		if (current->mm == mm)
+			sync_mm_rss(mm);
+
+		add_mm_counter(mm, MM_SWAPENTS, nr_swap);
+	}
+
 	arch_leave_lazy_mmu_mode();
 	pte_unmap_unlock(pte - 1, ptl);
 next:

Signature · 1 line

1.9.3