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

Groups > linux.kernel > #1580787 > unrolled thread

[PATCH V3 0/7] mm: fix some MADV_FREE issues

Back to article view | Back to linux.kernel

Contents

  [PATCH V3 0/7] mm: fix some MADV_FREE issues Shaohua Li <shli@fb.com> - 2017-02-14 20:40 +0100
    [PATCH V3 3/7] mm: reclaim MADV_FREE pages Shaohua Li <shli@fb.com> - 2017-02-14 20:40 +0100
      Re: [PATCH V3 3/7] mm: reclaim MADV_FREE pages Johannes Weiner <hannes@cmpxchg.org> - 2017-02-16 19:50 +0100
        Re: [PATCH V3 3/7] mm: reclaim MADV_FREE pages Minchan Kim <minchan@kernel.org> - 2017-02-17 06:50 +0100
          Re: [PATCH V3 3/7] mm: reclaim MADV_FREE pages Johannes Weiner <hannes@cmpxchg.org> - 2017-02-17 17:20 +0100
        Re: [PATCH V3 3/7] mm: reclaim MADV_FREE pages Minchan Kim <minchan@kernel.org> - 2017-02-17 06:50 +0100
          Re: [PATCH V3 3/7] mm: reclaim MADV_FREE pages Minchan Kim <minchan@kernel.org> - 2017-02-17 10:30 +0100
          Re: [PATCH V3 3/7] mm: reclaim MADV_FREE pages Johannes Weiner <hannes@cmpxchg.org> - 2017-02-17 17:20 +0100
        Re: [PATCH V3 3/7] mm: reclaim MADV_FREE pages Johannes Weiner <hannes@cmpxchg.org> - 2017-02-17 17:10 +0100
          Re: [PATCH V3 3/7] mm: reclaim MADV_FREE pages Johannes Weiner <hannes@cmpxchg.org> - 2017-02-17 21:10 +0100
    [PATCH V3 2/7] mm: move MADV_FREE pages into LRU_INACTIVE_FILE list Shaohua Li <shli@fb.com> - 2017-02-14 20:40 +0100
      Re: [PATCH V3 2/7] mm: move MADV_FREE pages into LRU_INACTIVE_FILE  list Johannes Weiner <hannes@cmpxchg.org> - 2017-02-16 19:00 +0100
        Re: [PATCH V3 2/7] mm: move MADV_FREE pages into LRU_INACTIVE_FILE  list Johannes Weiner <hannes@cmpxchg.org> - 2017-02-17 17:30 +0100

#1580787 — [PATCH V3 0/7] mm: fix some MADV_FREE issues

Hi,

We are trying to use MADV_FREE in jemalloc. Several issues are found. Without
solving the issues, jemalloc can't use the MADV_FREE feature.
- Doesn't support system without swap enabled. Because if swap is off, we can't
  or can't efficiently age anonymous pages. And since MADV_FREE pages are mixed
  with other anonymous pages, we can't reclaim MADV_FREE pages. In current
  implementation, MADV_FREE will fallback to MADV_DONTNEED without swap enabled.
  But in our environment, a lot of machines don't enable swap. This will prevent
  our setup using MADV_FREE.
- Increases memory pressure. page reclaim bias file pages reclaim against
  anonymous pages. This doesn't make sense for MADV_FREE pages, because those
  pages could be freed easily and refilled with very slight penality. Even page
  reclaim doesn't bias file pages, there is still an issue, because MADV_FREE
  pages and other anonymous pages are mixed together. To reclaim a MADV_FREE
  page, we probably must scan a lot of other anonymous pages, which is
  inefficient. In our test, we usually see oom with MADV_FREE enabled and nothing
  without it.
- RSS accounting. MADV_FREE pages are accounted as normal anon pages and
  reclaimed lazily, so application's RSS becomes bigger. This confuses our
  workloads. We have monitoring daemon running and if it finds applications' RSS
  becomes abnormal, the daemon will kill the applications even kernel can reclaim
  the memory easily. Currently we don't export separate RSS accounting for
  MADV_FREE pages. This will prevent our setup using MADV_FREE too.

To address the first the two issues, we can either put MADV_FREE pages into a
separate LRU list (Minchan's previous patches and V1 patches), or put them into
LRU_INACTIVE_FILE list (suggested by Johannes). The patchset use the second
idea. The reason is LRU_INACTIVE_FILE list is tiny nowadays and should be full
of used once file pages. So we can still efficiently reclaim MADV_FREE pages
there without interference with other anon and active file pages. Putting the
pages into inactive file list also has an advantage which allows page reclaim
to prioritize MADV_FREE pages and used once file pages. MADV_FREE pages are put
into the lru list and clear SwapBacked flag, so PageAnon(page) &&
!PageSwapBacked(page) will indicate a MADV_FREE pages. These pages will
directly freed without pageout if they are clean, otherwise normal swap will
reclaim them.

For the third issue, we add a separate RSS count for MADV_FREE pages. The count
will be increased in madvise syscall and decreased in page reclaim (eg, unmap).
There is one limitation, the accounting doesn't work well for shared pages.
Please check the last patch. This probably isn't a big issue, because userspace
will write the pages before reusing them, which will break the page sharing
between two processes. And if two processes share a page, the page can't really
be lazyfreed.

Thanks,
Shaohua

V2->V3:
- rebase to latest -mm tree
- Address severl issues pointed out by Minchan
- Add more descriptions

V1->V2:
- Put MADV_FREE pages into LRU_INACTIVE_FILE list instead of adding a new lru
  list, suggested by Johannes
- Add RSS support
http://marc.info/?l=linux-mm&m=148616481928054&w=2

Minchan previous patches:
http://marc.info/?l=linux-mm&m=144800657002763&w=2
----------------------

Shaohua Li (7):
  mm: don't assume anonymous pages have SwapBacked flag
  mm: move MADV_FREE pages into LRU_INACTIVE_FILE list
  mm: reclaim MADV_FREE pages
  mm: enable MADV_FREE for swapless system
  mm: add vmstat account for MADV_FREE pages
  proc: show MADV_FREE pages info in smaps
  mm: add a separate RSS for MADV_FREE pages

 drivers/base/node.c           |  2 ++
 fs/proc/array.c               |  9 +++++---
 fs/proc/internal.h            |  3 ++-
 fs/proc/meminfo.c             |  1 +
 fs/proc/task_mmu.c            | 17 ++++++++++++---
 fs/proc/task_nommu.c          |  4 +++-
 include/linux/mm_inline.h     | 29 ++++++++++++++++++++++++
 include/linux/mm_types.h      |  1 +
 include/linux/mmzone.h        |  2 ++
 include/linux/page-flags.h    |  6 +++++
 include/linux/swap.h          |  2 +-
 include/linux/vm_event_item.h |  2 +-
 mm/gup.c                      |  2 ++
 mm/huge_memory.c              | 14 ++++++++----
 mm/khugepaged.c               | 10 ++++-----
 mm/madvise.c                  | 16 ++++++--------
 mm/memory.c                   | 13 +++++++++--
 mm/migrate.c                  |  5 ++++-
 mm/oom_kill.c                 | 10 +++++----
 mm/page_alloc.c               | 13 ++++++++---
 mm/rmap.c                     | 40 ++++++++++++++++++++++-----------
 mm/swap.c                     | 51 ++++++++++++++++++++++++-------------------
 mm/vmscan.c                   | 30 +++++++++++++++++--------
 mm/vmstat.c                   |  3 +++
 24 files changed, 203 insertions(+), 82 deletions(-)

-- 
2.9.3

[toc] | [next] | [standalone]

#1580789 — [PATCH V3 3/7] mm: reclaim MADV_FREE pages

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 <mhocko@suse.com>
Cc: Minchan Kim <minchan@kernel.org>
Cc: Hugh Dickins <hughd@google.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Rik van Riel <riel@redhat.com>
Cc: Mel Gorman <mgorman@techsingularity.net>
Cc: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Shaohua Li <shli@fb.com>
---
 mm/huge_memory.c |  2 ++
 mm/madvise.c     |  1 +
 mm/rmap.c        | 17 ++++++++++++-----
 mm/vmscan.c      | 30 +++++++++++++++++++++---------
 4 files changed, 36 insertions(+), 14 deletions(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 4ddda58..3bb5ad5 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -1571,6 +1571,8 @@ bool madvise_free_huge_pmd(struct mmu_gather *tlb, struct vm_area_struct *vma,
 		set_pmd_at(mm, addr, pmd, orig_pmd);
 		tlb_remove_pmd_tlb_entry(tlb, pmd, addr);
 	}
+
+	mark_page_lazyfree(page);
 	ret = true;
 out:
 	spin_unlock(ptl);
diff --git a/mm/madvise.c b/mm/madvise.c
index 639c476..2faed38 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -412,6 +412,7 @@ 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);
 		}
+		mark_page_lazyfree(page);
 	}
 out:
 	if (nr_swap) {
diff --git a/mm/rmap.c b/mm/rmap.c
index af50eae..2cbdada 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -1419,11 +1419,18 @@ static int try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
 			VM_BUG_ON_PAGE(!PageSwapCache(page) && PageSwapBacked(page),
 				page);
 
-			if (!PageDirty(page) && (flags & TTU_LZFREE)) {
-				/* It's a freeable page by MADV_FREE */
-				dec_mm_counter(mm, MM_ANONPAGES);
-				rp->lazyfreed++;
-				goto discard;
+			if (flags & TTU_LZFREE) {
+				if (!PageDirty(page)) {
+					/* It's a freeable page by MADV_FREE */
+					dec_mm_counter(mm, MM_ANONPAGES);
+					rp->lazyfreed++;
+					goto discard;
+				} else {
+					set_pte_at(mm, address, pvmw.pte, pteval);
+					ret = SWAP_FAIL;
+					page_vma_mapped_walk_done(&pvmw);
+					break;
+				}
 			}
 
 			if (swap_duplicate(entry) < 0) {
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 26c3b40..435149c 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -911,7 +911,7 @@ static void page_check_dirty_writeback(struct page *page,
 	 * Anonymous pages are not handled by flushers and must be written
 	 * from reclaim context. Do not stall reclaim based on them
 	 */
-	if (!page_is_file_cache(page)) {
+	if (!page_is_file_cache(page) || page_is_lazyfree(page)) {
 		*dirty = false;
 		*writeback = false;
 		return;
@@ -971,7 +971,7 @@ 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;
-		bool lazyfree = false;
+		bool lazyfree;
 		int ret = SWAP_SUCCESS;
 
 		cond_resched();
@@ -986,6 +986,8 @@ static unsigned long shrink_page_list(struct list_head *page_list,
 
 		sc->nr_scanned++;
 
+		lazyfree = page_is_lazyfree(page);
+
 		if (unlikely(!page_evictable(page)))
 			goto cull_mlocked;
 
@@ -993,7 +995,7 @@ static unsigned long shrink_page_list(struct list_head *page_list,
 			goto keep_locked;
 
 		/* Double the slab pressure for mapped and swapcache pages */
-		if (page_mapped(page) || PageSwapCache(page))
+		if ((page_mapped(page) || PageSwapCache(page)) && !lazyfree)
 			sc->nr_scanned++;
 
 		may_enter_fs = (sc->gfp_mask & __GFP_FS) ||
@@ -1119,13 +1121,13 @@ 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) && !lazyfree) {
 			if (!(sc->gfp_mask & __GFP_IO))
 				goto keep_locked;
 			if (!add_to_swap(page, page_list))
 				goto activate_locked;
-			lazyfree = true;
 			may_enter_fs = 1;
 
 			/* Adding to swap updated mapping */
@@ -1142,7 +1144,7 @@ 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.
 		 */
-		if (page_mapped(page) && mapping) {
+		if (page_mapped(page) && (mapping || lazyfree)) {
 			switch (ret = try_to_unmap(page, lazyfree ?
 				(ttu_flags | TTU_BATCH_FLUSH | TTU_LZFREE) :
 				(ttu_flags | TTU_BATCH_FLUSH))) {
@@ -1154,7 +1156,14 @@ static unsigned long shrink_page_list(struct list_head *page_list,
 			case SWAP_MLOCK:
 				goto cull_mlocked;
 			case SWAP_LZFREE:
-				goto lazyfree;
+				/* follow __remove_mapping for reference */
+				if (page_ref_freeze(page, 1)) {
+					if (!PageDirty(page))
+						goto lazyfree;
+					else
+						page_ref_unfreeze(page, 1);
+				}
+				goto keep_locked;
 			case SWAP_SUCCESS:
 				; /* try to free the page below */
 			}
@@ -1266,10 +1275,9 @@ static unsigned long shrink_page_list(struct list_head *page_list,
 			}
 		}
 
-lazyfree:
 		if (!mapping || !__remove_mapping(mapping, page, true))
 			goto keep_locked;
-
+lazyfree:
 		/*
 		 * At this point, we have no other references and there is
 		 * no way to pick any more up (removed from LRU, removed
@@ -1294,6 +1302,8 @@ static unsigned long shrink_page_list(struct list_head *page_list,
 cull_mlocked:
 		if (PageSwapCache(page))
 			try_to_free_swap(page);
+		if (lazyfree)
+			clear_page_lazyfree(page);
 		unlock_page(page);
 		list_add(&page->lru, &ret_pages);
 		continue;
@@ -1303,6 +1313,8 @@ static unsigned long shrink_page_list(struct list_head *page_list,
 		if (PageSwapCache(page) && mem_cgroup_swap_full(page))
 			try_to_free_swap(page);
 		VM_BUG_ON_PAGE(PageActive(page), page);
+		if (lazyfree)
+			clear_page_lazyfree(page);
 		SetPageActive(page);
 		pgactivate++;
 keep_locked:
-- 
2.9.3

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

#1582854 — Re: [PATCH V3 3/7] mm: reclaim MADV_FREE pages

On Tue, Feb 14, 2017 at 11:36:09AM -0800, Shaohua Li wrote:
> @@ -1419,11 +1419,18 @@ static int try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
>  			VM_BUG_ON_PAGE(!PageSwapCache(page) && PageSwapBacked(page),
>  				page);
>  
> -			if (!PageDirty(page) && (flags & TTU_LZFREE)) {
> -				/* It's a freeable page by MADV_FREE */
> -				dec_mm_counter(mm, MM_ANONPAGES);
> -				rp->lazyfreed++;
> -				goto discard;
> +			if (flags & TTU_LZFREE) {
> +				if (!PageDirty(page)) {
> +					/* It's a freeable page by MADV_FREE */
> +					dec_mm_counter(mm, MM_ANONPAGES);
> +					rp->lazyfreed++;
> +					goto discard;
> +				} else {
> +					set_pte_at(mm, address, pvmw.pte, pteval);
> +					ret = SWAP_FAIL;
> +					page_vma_mapped_walk_done(&pvmw);
> +					break;
> +				}

I don't understand why we need the TTU_LZFREE bit in general. More on
that below at the callsite.

> @@ -911,7 +911,7 @@ static void page_check_dirty_writeback(struct page *page,
>  	 * Anonymous pages are not handled by flushers and must be written
>  	 * from reclaim context. Do not stall reclaim based on them
>  	 */
> -	if (!page_is_file_cache(page)) {
> +	if (!page_is_file_cache(page) || page_is_lazyfree(page)) {

Do we need this? MADV_FREE clears the dirty bit off the page; we could
just let them go through with the function without any special-casing.

> @@ -986,6 +986,8 @@ static unsigned long shrink_page_list(struct list_head *page_list,
>  
>  		sc->nr_scanned++;
>  
> +		lazyfree = page_is_lazyfree(page);
> +
>  		if (unlikely(!page_evictable(page)))
>  			goto cull_mlocked;
>  
> @@ -993,7 +995,7 @@ static unsigned long shrink_page_list(struct list_head *page_list,
>  			goto keep_locked;
>  
>  		/* Double the slab pressure for mapped and swapcache pages */
> -		if (page_mapped(page) || PageSwapCache(page))
> +		if ((page_mapped(page) || PageSwapCache(page)) && !lazyfree)
>  			sc->nr_scanned++;
>  
>  		may_enter_fs = (sc->gfp_mask & __GFP_FS) ||
> @@ -1119,13 +1121,13 @@ 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) && !lazyfree) {

lazyfree duplicates the anon check. As per the previous email, IMO it
would be much preferable to get rid of that "lazyfree" obscuring here.

This would simply be:

		if (PageAnon(page) && PageSwapBacked && !PageSwapCache)

> @@ -1142,7 +1144,7 @@ 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.
>  		 */
> -		if (page_mapped(page) && mapping) {
> +		if (page_mapped(page) && (mapping || lazyfree)) {

Do we actually need to filter for mapping || lazyfree? If we fail to
allocate swap, we don't reach here. If the page is a truncated file
page, ttu returns pretty much instantly with SWAP_AGAIN. We should be
able to just check for page_mapped() alone, no?

>  			switch (ret = try_to_unmap(page, lazyfree ?
>  				(ttu_flags | TTU_BATCH_FLUSH | TTU_LZFREE) :
>  				(ttu_flags | TTU_BATCH_FLUSH))) {

That bit I don't understand. Why do we need to pass TTU_LZFREE? What
information does that carry that cannot be gathered from inside ttu?

I.e. when ttu runs into PageAnon, can it simply check !PageSwapBacked?
And if it's still clean, it can lazyfreed++; goto discard.

Am I overlooking something?

> @@ -1154,7 +1156,14 @@ static unsigned long shrink_page_list(struct list_head *page_list,
>  			case SWAP_MLOCK:
>  				goto cull_mlocked;
>  			case SWAP_LZFREE:
> -				goto lazyfree;
> +				/* follow __remove_mapping for reference */
> +				if (page_ref_freeze(page, 1)) {
> +					if (!PageDirty(page))
> +						goto lazyfree;
> +					else
> +						page_ref_unfreeze(page, 1);
> +				}
> +				goto keep_locked;
>  			case SWAP_SUCCESS:
>  				; /* try to free the page below */

This is a similar situation.

Can we let the page go through the regular __remove_mapping() process
and simply have that function check for PageAnon && !PageSwapBacked?

> @@ -1266,10 +1275,9 @@ static unsigned long shrink_page_list(struct list_head *page_list,
>  			}
>  		}
>  
> -lazyfree:
>  		if (!mapping || !__remove_mapping(mapping, page, true))
>  			goto keep_locked;
> -
> +lazyfree:

... eliminating this special casing.

> @@ -1294,6 +1302,8 @@ static unsigned long shrink_page_list(struct list_head *page_list,
>  cull_mlocked:
>  		if (PageSwapCache(page))
>  			try_to_free_swap(page);
> +		if (lazyfree)
> +			clear_page_lazyfree(page);

Why cancel the MADV_FREE state? The combination seems non-sensical,
but we can simply retain the invalidated state while the page goes to
the unevictable list; munlock should move it back to inactive_file.

>  		unlock_page(page);
>  		list_add(&page->lru, &ret_pages);
>  		continue;
> @@ -1303,6 +1313,8 @@ static unsigned long shrink_page_list(struct list_head *page_list,
>  		if (PageSwapCache(page) && mem_cgroup_swap_full(page))
>  			try_to_free_swap(page);
>  		VM_BUG_ON_PAGE(PageActive(page), page);
> +		if (lazyfree)
> +			clear_page_lazyfree(page);

This is similar too.

Can we leave simply leave the page alone here? The only way we get to
this point is if somebody is reading the invalidated page. It's weird
for a lazyfreed page to become active, but it doesn't seem to warrant
active intervention here.

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

#1583104 — Re: [PATCH V3 3/7] mm: reclaim MADV_FREE pages

Hi Shaohua,

On Thu, Feb 16, 2017 at 04:27:18PM -0800, Shaohua Li wrote:
> On Thu, Feb 16, 2017 at 01:40:18PM -0500, Johannes Weiner wrote:
> > On Tue, Feb 14, 2017 at 11:36:09AM -0800, Shaohua Li wrote:
> > > @@ -1419,11 +1419,18 @@ static int try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
> > >  			VM_BUG_ON_PAGE(!PageSwapCache(page) && PageSwapBacked(page),
> > >  				page);
> > >  
> > > -			if (!PageDirty(page) && (flags & TTU_LZFREE)) {
> > > -				/* It's a freeable page by MADV_FREE */
> > > -				dec_mm_counter(mm, MM_ANONPAGES);
> > > -				rp->lazyfreed++;
> > > -				goto discard;
> > > +			if (flags & TTU_LZFREE) {
> > > +				if (!PageDirty(page)) {
> > > +					/* It's a freeable page by MADV_FREE */
> > > +					dec_mm_counter(mm, MM_ANONPAGES);
> > > +					rp->lazyfreed++;
> > > +					goto discard;
> > > +				} else {
> > > +					set_pte_at(mm, address, pvmw.pte, pteval);
> > > +					ret = SWAP_FAIL;
> > > +					page_vma_mapped_walk_done(&pvmw);
> > > +					break;
> > > +				}
> > 
> > I don't understand why we need the TTU_LZFREE bit in general. More on
> > that below at the callsite.
> 
> Sounds useless flag, don't see any reason we shouldn't free the MADV_FREE page
> in places other than reclaim. Looks TTU_UNMAP is useless too..

Agree on TTU_UNMAP but for example, THP split doesn't mean free lazyfree pages,
I think.

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

#1583575 — Re: [PATCH V3 3/7] mm: reclaim MADV_FREE pages

Hi Minchan,

On Fri, Feb 17, 2017 at 02:45:55PM +0900, Minchan Kim wrote:
> On Thu, Feb 16, 2017 at 04:27:18PM -0800, Shaohua Li wrote:
> > On Thu, Feb 16, 2017 at 01:40:18PM -0500, Johannes Weiner wrote:
> > > On Tue, Feb 14, 2017 at 11:36:09AM -0800, Shaohua Li wrote:
> > > > @@ -1419,11 +1419,18 @@ static int try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
> > > >  			VM_BUG_ON_PAGE(!PageSwapCache(page) && PageSwapBacked(page),
> > > >  				page);
> > > >  
> > > > -			if (!PageDirty(page) && (flags & TTU_LZFREE)) {
> > > > -				/* It's a freeable page by MADV_FREE */
> > > > -				dec_mm_counter(mm, MM_ANONPAGES);
> > > > -				rp->lazyfreed++;
> > > > -				goto discard;
> > > > +			if (flags & TTU_LZFREE) {
> > > > +				if (!PageDirty(page)) {
> > > > +					/* It's a freeable page by MADV_FREE */
> > > > +					dec_mm_counter(mm, MM_ANONPAGES);
> > > > +					rp->lazyfreed++;
> > > > +					goto discard;
> > > > +				} else {
> > > > +					set_pte_at(mm, address, pvmw.pte, pteval);
> > > > +					ret = SWAP_FAIL;
> > > > +					page_vma_mapped_walk_done(&pvmw);
> > > > +					break;
> > > > +				}
> > > 
> > > I don't understand why we need the TTU_LZFREE bit in general. More on
> > > that below at the callsite.
> > 
> > Sounds useless flag, don't see any reason we shouldn't free the MADV_FREE page
> > in places other than reclaim. Looks TTU_UNMAP is useless too..
> 
> Agree on TTU_UNMAP but for example, THP split doesn't mean free lazyfree pages,
> I think.

Anon THP splitting uses the migration branch, so we should be fine.

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

#1583106 — Re: [PATCH V3 3/7] mm: reclaim MADV_FREE pages

Hi Johannes,

On Thu, Feb 16, 2017 at 01:40:18PM -0500, Johannes Weiner wrote:
> On Tue, Feb 14, 2017 at 11:36:09AM -0800, Shaohua Li wrote:
> > @@ -1419,11 +1419,18 @@ static int try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
> >  			VM_BUG_ON_PAGE(!PageSwapCache(page) && PageSwapBacked(page),
> >  				page);
> >  
> > -			if (!PageDirty(page) && (flags & TTU_LZFREE)) {
> > -				/* It's a freeable page by MADV_FREE */
> > -				dec_mm_counter(mm, MM_ANONPAGES);
> > -				rp->lazyfreed++;
> > -				goto discard;
> > +			if (flags & TTU_LZFREE) {
> > +				if (!PageDirty(page)) {
> > +					/* It's a freeable page by MADV_FREE */
> > +					dec_mm_counter(mm, MM_ANONPAGES);
> > +					rp->lazyfreed++;
> > +					goto discard;
> > +				} else {
> > +					set_pte_at(mm, address, pvmw.pte, pteval);
> > +					ret = SWAP_FAIL;
> > +					page_vma_mapped_walk_done(&pvmw);
> > +					break;
> > +				}
> 
> I don't understand why we need the TTU_LZFREE bit in general. More on
> that below at the callsite.

The reason I introduced it was ttu is used for migration/THP split path
as well as reclaim. It's clear to discard them in reclaim path because
it means surely memory pressure now but not sure with other path.

If you guys think it's always win to discard them in try_to_unmap
unconditionally, I think it would be better to be separate patch.

> 
> > @@ -911,7 +911,7 @@ static void page_check_dirty_writeback(struct page *page,
> >  	 * Anonymous pages are not handled by flushers and must be written
> >  	 * from reclaim context. Do not stall reclaim based on them
> >  	 */
> > -	if (!page_is_file_cache(page)) {
> > +	if (!page_is_file_cache(page) || page_is_lazyfree(page)) {
> 
> Do we need this? MADV_FREE clears the dirty bit off the page; we could
> just let them go through with the function without any special-casing.

I thought some driver potentially can do GUP with FOLL_TOUCH so that the
lazyfree page can have PG_dirty with !PG_swapbacked. In this case,
throttling logic of shrink_page_list can be confused?

> 
> > @@ -986,6 +986,8 @@ static unsigned long shrink_page_list(struct list_head *page_list,
> >  
> >  		sc->nr_scanned++;
> >  
> > +		lazyfree = page_is_lazyfree(page);
> > +
> >  		if (unlikely(!page_evictable(page)))
> >  			goto cull_mlocked;
> >  
> > @@ -993,7 +995,7 @@ static unsigned long shrink_page_list(struct list_head *page_list,
> >  			goto keep_locked;
> >  
> >  		/* Double the slab pressure for mapped and swapcache pages */
> > -		if (page_mapped(page) || PageSwapCache(page))
> > +		if ((page_mapped(page) || PageSwapCache(page)) && !lazyfree)
> >  			sc->nr_scanned++;
> >  
> >  		may_enter_fs = (sc->gfp_mask & __GFP_FS) ||
> > @@ -1119,13 +1121,13 @@ 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) && !lazyfree) {
> 
> lazyfree duplicates the anon check. As per the previous email, IMO it
> would be much preferable to get rid of that "lazyfree" obscuring here.
> 
> This would simply be:
> 
> 		if (PageAnon(page) && PageSwapBacked && !PageSwapCache)

Agree.

> 
> > @@ -1142,7 +1144,7 @@ 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.
> >  		 */
> > -		if (page_mapped(page) && mapping) {
> > +		if (page_mapped(page) && (mapping || lazyfree)) {
> 
> Do we actually need to filter for mapping || lazyfree? If we fail to
> allocate swap, we don't reach here. If the page is a truncated file
> page, ttu returns pretty much instantly with SWAP_AGAIN. We should be
> able to just check for page_mapped() alone, no?

try_to_unmap_one assumes every anonymous pages reached will have swp_entry
so it should be changed to check PageSwapCache if we go to the way.

> 
> >  			switch (ret = try_to_unmap(page, lazyfree ?
> >  				(ttu_flags | TTU_BATCH_FLUSH | TTU_LZFREE) :
> >  				(ttu_flags | TTU_BATCH_FLUSH))) {
> 
> That bit I don't understand. Why do we need to pass TTU_LZFREE? What
> information does that carry that cannot be gathered from inside ttu?
> 
> I.e. when ttu runs into PageAnon, can it simply check !PageSwapBacked?
> And if it's still clean, it can lazyfreed++; goto discard.
> 
> Am I overlooking something?

As I said above, TTU_LZFREE signals when we should discard the page and
in my implementation, I thought it was only shrink_page_list which is
event for memory pressure.

Thanks.

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

#1583237 — Re: [PATCH V3 3/7] mm: reclaim MADV_FREE pages

On Fri, Feb 17, 2017 at 02:41:08PM +0900, Minchan Kim wrote:
> Hi Johannes,
> 
> On Thu, Feb 16, 2017 at 01:40:18PM -0500, Johannes Weiner wrote:
> > On Tue, Feb 14, 2017 at 11:36:09AM -0800, Shaohua Li wrote:
> > > @@ -1419,11 +1419,18 @@ static int try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
> > >  			VM_BUG_ON_PAGE(!PageSwapCache(page) && PageSwapBacked(page),
> > >  				page);
> > >  
> > > -			if (!PageDirty(page) && (flags & TTU_LZFREE)) {
> > > -				/* It's a freeable page by MADV_FREE */
> > > -				dec_mm_counter(mm, MM_ANONPAGES);
> > > -				rp->lazyfreed++;
> > > -				goto discard;
> > > +			if (flags & TTU_LZFREE) {
> > > +				if (!PageDirty(page)) {
> > > +					/* It's a freeable page by MADV_FREE */
> > > +					dec_mm_counter(mm, MM_ANONPAGES);
> > > +					rp->lazyfreed++;
> > > +					goto discard;
> > > +				} else {
> > > +					set_pte_at(mm, address, pvmw.pte, pteval);
> > > +					ret = SWAP_FAIL;
> > > +					page_vma_mapped_walk_done(&pvmw);
> > > +					break;
> > > +				}
> > 
> > I don't understand why we need the TTU_LZFREE bit in general. More on
> > that below at the callsite.
> 
> The reason I introduced it was ttu is used for migration/THP split path
> as well as reclaim. It's clear to discard them in reclaim path because
> it means surely memory pressure now but not sure with other path.
> 
> If you guys think it's always win to discard them in try_to_unmap
> unconditionally, I think it would be better to be separate patch.

I was totally wrong.

Anon page with THP split/migration/HWPoison will not reach to discard path
in try_to_unmap_one so Johannes is right. We don't need TTU_LZFREE.

Sorry for the noise.

Thanks.

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

#1583569 — Re: [PATCH V3 3/7] mm: reclaim MADV_FREE pages

On Fri, Feb 17, 2017 at 02:41:08PM +0900, Minchan Kim wrote:
> Hi Johannes,
> 
> On Thu, Feb 16, 2017 at 01:40:18PM -0500, Johannes Weiner wrote:
> > On Tue, Feb 14, 2017 at 11:36:09AM -0800, Shaohua Li wrote:
> > > @@ -911,7 +911,7 @@ static void page_check_dirty_writeback(struct page *page,
> > >  	 * Anonymous pages are not handled by flushers and must be written
> > >  	 * from reclaim context. Do not stall reclaim based on them
> > >  	 */
> > > -	if (!page_is_file_cache(page)) {
> > > +	if (!page_is_file_cache(page) || page_is_lazyfree(page)) {
> > 
> > Do we need this? MADV_FREE clears the dirty bit off the page; we could
> > just let them go through with the function without any special-casing.
> 
> I thought some driver potentially can do GUP with FOLL_TOUCH so that the
> lazyfree page can have PG_dirty with !PG_swapbacked. In this case,
> throttling logic of shrink_page_list can be confused?

Yep, agreed. We should filter these pages here.

> > > @@ -1142,7 +1144,7 @@ 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.
> > >  		 */
> > > -		if (page_mapped(page) && mapping) {
> > > +		if (page_mapped(page) && (mapping || lazyfree)) {
> > 
> > Do we actually need to filter for mapping || lazyfree? If we fail to
> > allocate swap, we don't reach here. If the page is a truncated file
> > page, ttu returns pretty much instantly with SWAP_AGAIN. We should be
> > able to just check for page_mapped() alone, no?
> 
> try_to_unmap_one assumes every anonymous pages reached will have swp_entry
> so it should be changed to check PageSwapCache if we go to the way.

Yep, I think it should check page_mapping(). To me that would make the
most sense, see other email: "Don't unmap a ram page with valid data
when there is no secondary storage mapping to maintain integrity."

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

#1583550 — Re: [PATCH V3 3/7] mm: reclaim MADV_FREE pages

On Thu, Feb 16, 2017 at 04:27:18PM -0800, Shaohua Li wrote:
> On Thu, Feb 16, 2017 at 01:40:18PM -0500, Johannes Weiner wrote:
> > On Tue, Feb 14, 2017 at 11:36:09AM -0800, Shaohua Li wrote:
> > > @@ -911,7 +911,7 @@ static void page_check_dirty_writeback(struct page *page,
> > >  	 * Anonymous pages are not handled by flushers and must be written
> > >  	 * from reclaim context. Do not stall reclaim based on them
> > >  	 */
> > > -	if (!page_is_file_cache(page)) {
> > > +	if (!page_is_file_cache(page) || page_is_lazyfree(page)) {
> > 
> > Do we need this? MADV_FREE clears the dirty bit off the page; we could
> > just let them go through with the function without any special-casing.
> 
> this is just to zero dirty and writeback

Okay, I assumed that the page would always be !dirty && !writeback
here anyway, so we might as well fall through and check those bits.

But a previously failed TTU might have moved a pte dirty bit to the
page, so yes, we do need to filter for anon && !swapbacked here.

> > > @@ -1142,7 +1144,7 @@ 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.
> > >  		 */
> > > -		if (page_mapped(page) && mapping) {
> > > +		if (page_mapped(page) && (mapping || lazyfree)) {
> > 
> > Do we actually need to filter for mapping || lazyfree? If we fail to
> > allocate swap, we don't reach here. If the page is a truncated file
> > page, ttu returns pretty much instantly with SWAP_AGAIN. We should be
> > able to just check for page_mapped() alone, no?
> 
> checking the mapping is faster than running into try_to_unamp, right?

!mapping should be a rare case. In reclaim code, I think it's better
to keep it simple than to optimize away the rare function call.

> > > @@ -1154,7 +1156,14 @@ static unsigned long shrink_page_list(struct list_head *page_list,
> > >  			case SWAP_MLOCK:
> > >  				goto cull_mlocked;
> > >  			case SWAP_LZFREE:
> > > -				goto lazyfree;
> > > +				/* follow __remove_mapping for reference */
> > > +				if (page_ref_freeze(page, 1)) {
> > > +					if (!PageDirty(page))
> > > +						goto lazyfree;
> > > +					else
> > > +						page_ref_unfreeze(page, 1);
> > > +				}
> > > +				goto keep_locked;
> > >  			case SWAP_SUCCESS:
> > >  				; /* try to free the page below */
> > 
> > This is a similar situation.
> > 
> > Can we let the page go through the regular __remove_mapping() process
> > and simply have that function check for PageAnon && !PageSwapBacked?
> 
> That will make the code more complicated. We don't call __remove_mapping if
> !mapping. And we need to do bypass in __remove_mapping, for example, avoid
> taking mapping->lock.

True, we won't get around a separate freeing path as long as the
refcount handling is intertwined with the mapping removal like that :/

What we should be able to do, however, is remove at least SWAP_LZFREE
and stick with SWAP_SUCCESS. On success, we can fall through up until
we do the __remove_mapping call. The page isn't dirty, so we skip that
PageDirty block; the page doesn't have private data, so we skip that
block too. And then we can branch on PageAnon && !PageSwapBacked that
does our alternate freeing path or __remove_mapping for others.

> > > @@ -1294,6 +1302,8 @@ static unsigned long shrink_page_list(struct list_head *page_list,
> > >  cull_mlocked:
> > >  		if (PageSwapCache(page))
> > >  			try_to_free_swap(page);
> > > +		if (lazyfree)
> > > +			clear_page_lazyfree(page);
> > 
> > Why cancel the MADV_FREE state? The combination seems non-sensical,
> > but we can simply retain the invalidated state while the page goes to
> > the unevictable list; munlock should move it back to inactive_file.
> 
> This depends on the policy. If user locks the page, I think it's reasonable to
> assume the page is hot, so it doesn't make sense to treat the page lazyfree.

I think the key issue is whether the page contains valid data, not
whether it is hot. When we clear the dirty bits along with
PageSwapBacked, we're declaring the data in the page invalid. There is
no practical usecase to mlock a page with invalid data, sure, but the
act of mlocking a page doesn't make its contents suddenly valid again.

I.e. I'd stick with the pure data integrity perspective here. That's
clearer and less error prone than intermingling it with eviction
policy, to avoid accidents where we lose valid data.

> > >  		unlock_page(page);
> > >  		list_add(&page->lru, &ret_pages);
> > >  		continue;
> > > @@ -1303,6 +1313,8 @@ static unsigned long shrink_page_list(struct list_head *page_list,
> > >  		if (PageSwapCache(page) && mem_cgroup_swap_full(page))
> > >  			try_to_free_swap(page);
> > >  		VM_BUG_ON_PAGE(PageActive(page), page);
> > > +		if (lazyfree)
> > > +			clear_page_lazyfree(page);
> > 
> > This is similar too.
> > 
> > Can we leave simply leave the page alone here? The only way we get to
> > this point is if somebody is reading the invalidated page. It's weird
> > for a lazyfreed page to become active, but it doesn't seem to warrant
> > active intervention here.
> 
> So the unmap fails here probably because the page is dirty, which means the
> page is written recently. It makes sense to assume the page is hot.

Ah, good point.

But can we handle that explicitly please? Like above, I don't want to
undo the data invalidation just because somebody read the invalid data
a bunch of times and it has the access bits set. We should only re-set
the PageSwapBacked based on whether the page is actually dirty.

Maybe along the lines of SWAP_MLOCK we could add SWAP_DIRTY when TTU
fails because the page is dirty, and then have a cull_dirty: label in
shrink_page_list handle the lazy rescue of a reused MADV_FREE page?

This should work well with removing the mapping || lazyfree check when
calling TTU. Then TTU can fail on dirty && !mapping, which is a much
more obvious way of expressing it IMO - "This page contains valid data
but there is no mapping that backs it once we unmap it. Abort."

That's mostly why I'm in favor of removing the idea of a "lazyfree"
page as much as possible. IMO this whole thing becomes much more
understandable - and less bolted on to the side of the VM - when we
express it in existing concepts the VM uses for data integrity.

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

#1583709 — Re: [PATCH V3 3/7] mm: reclaim MADV_FREE pages

On Fri, Feb 17, 2017 at 10:43:41AM -0800, Shaohua Li wrote:
> On Fri, Feb 17, 2017 at 11:01:54AM -0500, Johannes Weiner wrote:
> > On Thu, Feb 16, 2017 at 04:27:18PM -0800, Shaohua Li wrote:
> > > On Thu, Feb 16, 2017 at 01:40:18PM -0500, Johannes Weiner wrote:
> > > > On Tue, Feb 14, 2017 at 11:36:09AM -0800, Shaohua Li wrote:
> > > > >  		unlock_page(page);
> > > > >  		list_add(&page->lru, &ret_pages);
> > > > >  		continue;
> > > > > @@ -1303,6 +1313,8 @@ static unsigned long shrink_page_list(struct list_head *page_list,
> > > > >  		if (PageSwapCache(page) && mem_cgroup_swap_full(page))
> > > > >  			try_to_free_swap(page);
> > > > >  		VM_BUG_ON_PAGE(PageActive(page), page);
> > > > > +		if (lazyfree)
> > > > > +			clear_page_lazyfree(page);
> > > > 
> > > > Can we leave simply leave the page alone here? The only way we get to
> > > > this point is if somebody is reading the invalidated page. It's weird
> > > > for a lazyfreed page to become active, but it doesn't seem to warrant
> > > > active intervention here.
> > > 
> > > So the unmap fails here probably because the page is dirty, which means the
> > > page is written recently. It makes sense to assume the page is hot.
> > 
> > Ah, good point.
> > 
> > But can we handle that explicitly please? Like above, I don't want to
> > undo the data invalidation just because somebody read the invalid data
> > a bunch of times and it has the access bits set. We should only re-set
> > the PageSwapBacked based on whether the page is actually dirty.
> > 
> > Maybe along the lines of SWAP_MLOCK we could add SWAP_DIRTY when TTU
> > fails because the page is dirty, and then have a cull_dirty: label in
> > shrink_page_list handle the lazy rescue of a reused MADV_FREE page?
> > 
> > This should work well with removing the mapping || lazyfree check when
> > calling TTU. Then TTU can fail on dirty && !mapping, which is a much
> > more obvious way of expressing it IMO - "This page contains valid data
> > but there is no mapping that backs it once we unmap it. Abort."
> > 
> > That's mostly why I'm in favor of removing the idea of a "lazyfree"
> > page as much as possible. IMO this whole thing becomes much more
> > understandable - and less bolted on to the side of the VM - when we
> > express it in existing concepts the VM uses for data integrity.
> 
> Ok, it makes sense to only reset the PageSwapBacked bit for dirty page. In this
> way, we jump to activate_locked for SWAP_DIRTY || (SWAP_FAIL && pagelazyfree)
> and jump to activate_locked for SWAP_FAIL && !pagelazyfree. Is this what you
> want to do? This will add extra checks for SWAP_FAIL. I'm not sure if this is
> really worthy because it's rare the MADV_FREE page is read.

Yes, for SWAP_DIRTY jump to activate_locked or have its own label that
sets PG_swapbacked again and moves the page back to the proper LRU.

SWAP_FAIL of an anon && !swapbacked && !dirty && referenced page can
be ignored IMO. This happens only when the user is reading invalid
data over and over, I see no reason to optimize for that. We activate
a MADV_FREE page, which is weird, but not a correctness issue, right?

Just to clarify, right now we have this:

---

SWAP_FAIL (failure on pte, swap, lazyfree):
  if pagelazyfree:
    clear pagelazyfree
  activate

SWAP_SUCCESS:
  regular reclaim

SWAP_LZFREE (success on lazyfree when page and ptes are all clean):
  free page

---

What I'm proposing is to separate lazyfree failure out from SWAP_FAIL
into its own branch. Then merge lazyfree success into SWAP_SUCCESS:

---

SWAP_FAIL (failure on pte, swap):
  activate

SWAP_SUCCESS:
  if anon && !swapbacked:
    free manually
  else:
    __remove_mapping()

SWAP_DIRTY (anon && !swapbacked && dirty):
  set swapbacked
  putback/activate

---

This way we have a mostly unified success path (we might later be able
to refactor __remove_mapping to split refcounting from mapping stuff
to remove the last trace of difference), and SWAP_DIRTY follows the
same type of delayed LRU fixup as we do for SWAP_MLOCK right now.

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

#1580793 — [PATCH V3 2/7] mm: move MADV_FREE pages into LRU_INACTIVE_FILE list

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 <mhocko@suse.com>
Cc: Minchan Kim <minchan@kernel.org>
Cc: Hugh Dickins <hughd@google.com>
Cc: Rik van Riel <riel@redhat.com>
Cc: Mel Gorman <mgorman@techsingularity.net>
Cc: Andrew Morton <akpm@linux-foundation.org>
Suggested-by: Johannes Weiner <hannes@cmpxchg.org>
Signed-off-by: Shaohua Li <shli@fb.com>
---
 include/linux/mm_inline.h     | 20 +++++++++++++++++
 include/linux/swap.h          |  2 +-
 include/linux/vm_event_item.h |  2 +-
 mm/huge_memory.c              |  3 ---
 mm/madvise.c                  |  2 --
 mm/swap.c                     | 51 ++++++++++++++++++++++++-------------------
 mm/vmstat.c                   |  1 +
 7 files changed, 52 insertions(+), 29 deletions(-)

diff --git a/include/linux/mm_inline.h b/include/linux/mm_inline.h
index e030a68..e6e3af1 100644
--- a/include/linux/mm_inline.h
+++ b/include/linux/mm_inline.h
@@ -126,4 +126,24 @@ static __always_inline enum lru_list page_lru(struct page *page)
 
 #define lru_to_page(head) (list_entry((head)->prev, struct page, lru))
 
+/*
+ * lazyfree pages are clean anonymous pages. They have SwapBacked flag cleared
+ * to destinguish normal anonymous pages.
+ */
+static inline void set_page_lazyfree(struct page *page)
+{
+	VM_BUG_ON_PAGE(!PageAnon(page) || !PageSwapBacked(page), page);
+	ClearPageSwapBacked(page);
+}
+
+static inline void clear_page_lazyfree(struct page *page)
+{
+	VM_BUG_ON_PAGE(!PageAnon(page) || PageSwapBacked(page), page);
+	SetPageSwapBacked(page);
+}
+
+static inline bool page_is_lazyfree(struct page *page)
+{
+	return PageAnon(page) && !PageSwapBacked(page);
+}
 #endif
diff --git a/include/linux/swap.h b/include/linux/swap.h
index 45e91dd..486494e 100644
--- a/include/linux/swap.h
+++ b/include/linux/swap.h
@@ -279,7 +279,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 mark_page_lazyfree(struct page *page);
 extern void swap_setup(void);
 
 extern void add_page_to_unevictable_list(struct page *page);
diff --git a/include/linux/vm_event_item.h b/include/linux/vm_event_item.h
index 6aa1b6c..94e58da 100644
--- a/include/linux/vm_event_item.h
+++ b/include/linux/vm_event_item.h
@@ -25,7 +25,7 @@ enum vm_event_item { PGPGIN, PGPGOUT, PSWPIN, PSWPOUT,
 		FOR_ALL_ZONES(PGALLOC),
 		FOR_ALL_ZONES(ALLOCSTALL),
 		FOR_ALL_ZONES(PGSCAN_SKIP),
-		PGFREE, PGACTIVATE, PGDEACTIVATE,
+		PGFREE, PGACTIVATE, PGDEACTIVATE, PGLAZYFREE,
 		PGFAULT, PGMAJFAULT,
 		PGLAZYFREED,
 		PGREFILL,
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index e602265..4ddda58 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -1562,9 +1562,6 @@ bool madvise_free_huge_pmd(struct mmu_gather *tlb, struct vm_area_struct *vma,
 		ClearPageDirty(page);
 	unlock_page(page);
 
-	if (PageActive(page))
-		deactivate_page(page);
-
 	if (pmd_young(orig_pmd) || pmd_dirty(orig_pmd)) {
 		orig_pmd = pmdp_huge_get_and_clear_full(tlb->mm, addr, pmd,
 			tlb->fullmm);
diff --git a/mm/madvise.c b/mm/madvise.c
index 11fc65f..639c476 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -410,8 +410,6 @@ 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);
 			tlb_remove_tlb_entry(tlb, pte, addr);
 		}
 	}
diff --git a/mm/swap.c b/mm/swap.c
index c4910f1..9305c23 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -46,7 +46,7 @@ int page_cluster;
 static DEFINE_PER_CPU(struct pagevec, lru_add_pvec);
 static DEFINE_PER_CPU(struct pagevec, lru_rotate_pvecs);
 static DEFINE_PER_CPU(struct pagevec, lru_deactivate_file_pvecs);
-static DEFINE_PER_CPU(struct pagevec, lru_deactivate_pvecs);
+static DEFINE_PER_CPU(struct pagevec, lru_lazyfree_pvecs);
 #ifdef CONFIG_SMP
 static DEFINE_PER_CPU(struct pagevec, activate_page_pvecs);
 #endif
@@ -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 (page_is_lazyfree(page)) {
+			clear_page_lazyfree(page);
+			/* charge to anon scanned/rotated reclaim_stat */
+			file = 0;
+			lru = LRU_INACTIVE_ANON;
+		}
 		SetPageActive(page);
 		lru += LRU_ACTIVE;
 		add_page_to_lru_list(page, lruvec, lru);
@@ -561,20 +567,21 @@ 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);
 
-		del_page_from_lru_list(page, lruvec, lru + LRU_ACTIVE);
+		del_page_from_lru_list(page, lruvec, LRU_INACTIVE_ANON + active);
 		ClearPageActive(page);
 		ClearPageReferenced(page);
-		add_page_to_lru_list(page, lruvec, lru);
+		set_page_lazyfree(page);
+		add_page_to_lru_list(page, lruvec, LRU_INACTIVE_FILE);
 
-		__count_vm_event(PGDEACTIVATE);
-		update_page_reclaim_stat(lruvec, file, 0);
+		__count_vm_events(PGLAZYFREE, hpage_nr_pages(page));
+		update_page_reclaim_stat(lruvec, 1, 0);
 	}
 }
 
@@ -604,9 +611,9 @@ void lru_add_drain_cpu(int cpu)
 	if (pagevec_count(pvec))
 		pagevec_lru_move_fn(pvec, lru_deactivate_file_fn, NULL);
 
-	pvec = &per_cpu(lru_deactivate_pvecs, cpu);
+	pvec = &per_cpu(lru_lazyfree_pvecs, cpu);
 	if (pagevec_count(pvec))
-		pagevec_lru_move_fn(pvec, lru_deactivate_fn, NULL);
+		pagevec_lru_move_fn(pvec, lru_lazyfree_fn, NULL);
 
 	activate_page_drain(cpu);
 }
@@ -638,22 +645,22 @@ void deactivate_file_page(struct page *page)
 }
 
 /**
- * deactivate_page - deactivate a page
+ * mark_page_lazyfree - make an anon page lazyfree
  * @page: page to deactivate
  *
- * deactivate_page() moves @page to the inactive list if @page was on the active
- * list and was not an unevictable page.  This is done to accelerate the reclaim
- * of @page.
+ * mark_page_lazyfree() moves @page to the inactive file list.
+ * This is done to accelerate the reclaim of @page.
  */
-void deactivate_page(struct page *page)
-{
-	if (PageLRU(page) && PageActive(page) && !PageUnevictable(page)) {
-		struct pagevec *pvec = &get_cpu_var(lru_deactivate_pvecs);
+void mark_page_lazyfree(struct page *page)
+ {
+	if (PageLRU(page) && PageAnon(page) && PageSwapBacked(page) &&
+	    !PageUnevictable(page)) {
+		struct pagevec *pvec = &get_cpu_var(lru_lazyfree_pvecs);
 
 		get_page(page);
 		if (!pagevec_add(pvec, page) || PageCompound(page))
-			pagevec_lru_move_fn(pvec, lru_deactivate_fn, NULL);
-		put_cpu_var(lru_deactivate_pvecs);
+			pagevec_lru_move_fn(pvec, lru_lazyfree_fn, NULL);
+		put_cpu_var(lru_lazyfree_pvecs);
 	}
 }
 
@@ -704,7 +711,7 @@ void lru_add_drain_all(void)
 		if (pagevec_count(&per_cpu(lru_add_pvec, cpu)) ||
 		    pagevec_count(&per_cpu(lru_rotate_pvecs, cpu)) ||
 		    pagevec_count(&per_cpu(lru_deactivate_file_pvecs, cpu)) ||
-		    pagevec_count(&per_cpu(lru_deactivate_pvecs, cpu)) ||
+		    pagevec_count(&per_cpu(lru_lazyfree_pvecs, cpu)) ||
 		    need_activate_page_drain(cpu)) {
 			INIT_WORK(work, lru_add_drain_per_cpu);
 			queue_work_on(cpu, lru_add_drain_wq, work);
diff --git a/mm/vmstat.c b/mm/vmstat.c
index 69f9aff..7774196 100644
--- a/mm/vmstat.c
+++ b/mm/vmstat.c
@@ -992,6 +992,7 @@ const char * const vmstat_text[] = {
 	"pgfree",
 	"pgactivate",
 	"pgdeactivate",
+	"pglazyfree",
 
 	"pgfault",
 	"pgmajfault",
-- 
2.9.3

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

#1582771 — Re: [PATCH V3 2/7] mm: move MADV_FREE pages into LRU_INACTIVE_FILE list

On Tue, Feb 14, 2017 at 11:36:08AM -0800, Shaohua Li wrote:
> @@ -126,4 +126,24 @@ static __always_inline enum lru_list page_lru(struct page *page)
>  
>  #define lru_to_page(head) (list_entry((head)->prev, struct page, lru))
>  
> +/*
> + * lazyfree pages are clean anonymous pages. They have SwapBacked flag cleared
> + * to destinguish normal anonymous pages.
> + */
> +static inline void set_page_lazyfree(struct page *page)
> +{
> +	VM_BUG_ON_PAGE(!PageAnon(page) || !PageSwapBacked(page), page);
> +	ClearPageSwapBacked(page);
> +}
> +
> +static inline void clear_page_lazyfree(struct page *page)
> +{
> +	VM_BUG_ON_PAGE(!PageAnon(page) || PageSwapBacked(page), page);
> +	SetPageSwapBacked(page);
> +}
> +
> +static inline bool page_is_lazyfree(struct page *page)
> +{
> +	return PageAnon(page) && !PageSwapBacked(page);
> +}

Sorry for not getting to v2 in time, but I have to say I strongly
agree with your first iterations and would much prefer this to be
open-coded.

IMO this needlessly introduces a new state opaquely called "lazyfree",
when really that's just anonymous pages that don't need to be swapped
before reclaim - PageAnon && !PageSwapBacked. Very simple MM concept.

That especially shows when we later combine it with page_is_file_cache
checks like the next patch does.

The rest of the patch looks good to me.

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

#1583584 — Re: [PATCH V3 2/7] mm: move MADV_FREE pages into LRU_INACTIVE_FILE list

On Thu, Feb 16, 2017 at 04:35:25PM -0800, Shaohua Li wrote:
> On Thu, Feb 16, 2017 at 12:52:53PM -0500, Johannes Weiner wrote:
> > On Tue, Feb 14, 2017 at 11:36:08AM -0800, Shaohua Li wrote:
> > > @@ -126,4 +126,24 @@ static __always_inline enum lru_list page_lru(struct page *page)
> > >  
> > >  #define lru_to_page(head) (list_entry((head)->prev, struct page, lru))
> > >  
> > > +/*
> > > + * lazyfree pages are clean anonymous pages. They have SwapBacked flag cleared
> > > + * to destinguish normal anonymous pages.
> > > + */
> > > +static inline void set_page_lazyfree(struct page *page)
> > > +{
> > > +	VM_BUG_ON_PAGE(!PageAnon(page) || !PageSwapBacked(page), page);
> > > +	ClearPageSwapBacked(page);
> > > +}
> > > +
> > > +static inline void clear_page_lazyfree(struct page *page)
> > > +{
> > > +	VM_BUG_ON_PAGE(!PageAnon(page) || PageSwapBacked(page), page);
> > > +	SetPageSwapBacked(page);
> > > +}
> > > +
> > > +static inline bool page_is_lazyfree(struct page *page)
> > > +{
> > > +	return PageAnon(page) && !PageSwapBacked(page);
> > > +}
> > 
> > Sorry for not getting to v2 in time, but I have to say I strongly
> > agree with your first iterations and would much prefer this to be
> > open-coded.
> > 
> > IMO this needlessly introduces a new state opaquely called "lazyfree",
> > when really that's just anonymous pages that don't need to be swapped
> > before reclaim - PageAnon && !PageSwapBacked. Very simple MM concept.
> > 
> > That especially shows when we later combine it with page_is_file_cache
> > checks like the next patch does.
> > 
> > The rest of the patch looks good to me.
> 
> Thanks! I do agree checking PageSwapBacked is clearer, but Minchan convinced me
> because of the accounting issue. Where do you suggest we should put the
> accounting to?

I now proposed quite a few changes to the setting and clearing sites,
so it's harder to judge, but AFAICT once those sites are consolidated,
open-coding the stat updates as well shouldn't be too bad, right?

One site to clear during MADV_FREE, one site to set during reclaim.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web