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

Groups > linux.kernel > #1250364 > unrolled thread

[PATCH 0/5] MADV_FREE refactoring and fix KSM page

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/5] MADV_FREE refactoring and fix KSM page Minchan Kim <minchan@kernel.org> - 2015-10-19 08:30 +0200
    [PATCH 2/5] mm: skip huge zero page in MADV_FREE Minchan Kim <minchan@kernel.org> - 2015-10-19 08:30 +0200
    [PATCH 1/5] mm: MADV_FREE trivial clean up Minchan Kim <minchan@kernel.org> - 2015-10-19 08:30 +0200
    [PATCH 5/5] mm: mark stable page dirty in KSM Minchan Kim <minchan@kernel.org> - 2015-10-19 08:30 +0200
      Re: [PATCH 5/5] mm: mark stable page dirty in KSM Hugh Dickins <hughd@google.com> - 2015-10-27 03:30 +0100
        Re: [PATCH 5/5] mm: mark stable page dirty in KSM Minchan Kim <minchan@kernel.org> - 2015-10-27 08:00 +0100
    [PATCH 3/5] mm: clear PG_dirty to mark page freeable Minchan Kim <minchan@kernel.org> - 2015-10-19 08:30 +0200
      Re: [PATCH 3/5] mm: clear PG_dirty to mark page freeable Hugh Dickins <hughd@google.com> - 2015-10-27 02:30 +0100
        Re: [PATCH 3/5] mm: clear PG_dirty to mark page freeable Minchan Kim <minchan@kernel.org> - 2015-10-27 08:00 +0100
    [PATCH 4/5] mm: simplify reclaim path for MADV_FREE Minchan Kim <minchan@kernel.org> - 2015-10-19 08:40 +0200
      Re: [PATCH 4/5] mm: simplify reclaim path for MADV_FREE Hugh Dickins <hughd@google.com> - 2015-10-27 03:10 +0100
        Re: [PATCH 4/5] mm: simplify reclaim path for MADV_FREE yalin wang <yalin.wang2010@gmail.com> - 2015-10-27 04:50 +0100
          Re: [PATCH 4/5] mm: simplify reclaim path for MADV_FREE Minchan Kim <minchan@kernel.org> - 2015-10-27 08:10 +0100
            Re: [PATCH 4/5] mm: simplify reclaim path for MADV_FREE yalin wang <yalin.wang2010@gmail.com> - 2015-10-27 08:40 +0100
              Re: [PATCH 4/5] mm: simplify reclaim path for MADV_FREE Minchan Kim <minchan@kernel.org> - 2015-10-27 09:20 +0100
                Re: [PATCH 4/5] mm: simplify reclaim path for MADV_FREE yalin wang <yalin.wang2010@gmail.com> - 2015-10-27 10:00 +0100
                  Re: [PATCH 4/5] mm: simplify reclaim path for MADV_FREE yalin wang <yalin.wang2010@gmail.com> - 2015-10-28 05:10 +0100
        Re: [PATCH 4/5] mm: simplify reclaim path for MADV_FREE Minchan Kim <minchan@kernel.org> - 2015-10-27 08:00 +0100
    Re: [PATCH 0/5] MADV_FREE refactoring and fix KSM page Minchan Kim <minchan@kernel.org> - 2015-10-19 12:00 +0200
      Re: [PATCH 0/5] MADV_FREE refactoring and fix KSM page Minchan Kim <minchan@kernel.org> - 2015-10-20 09:30 +0200
      Re: [PATCH 0/5] MADV_FREE refactoring and fix KSM page Minchan Kim <minchan@kernel.org> - 2015-10-20 09:30 +0200
        Re: [PATCH 0/5] MADV_FREE refactoring and fix KSM page Andrew Morton <akpm@linux-foundation.org> - 2015-10-20 23:40 +0200
          Re: [PATCH 0/5] MADV_FREE refactoring and fix KSM page "Kirill A. Shutemov" <kirill@shutemov.name> - 2015-10-21 00:50 +0200
            Re: [PATCH 0/5] MADV_FREE refactoring and fix KSM page Minchan Kim <minchan@kernel.org> - 2015-10-21 07:20 +0200
              Re: [PATCH 0/5] MADV_FREE refactoring and fix KSM page "Kirill A. Shutemov" <kirill@shutemov.name> - 2015-10-21 10:00 +0200

Page 1 of 2  [1] 2  Next page →

#1250364 — [PATCH 0/5] MADV_FREE refactoring and fix KSM page

Hello, it's too late since I sent previos patch.
https://lkml.org/lkml/2015/6/3/37

This patch is alomost new compared to previos approach.
I think this is more simple, clear and easy to review.

One thing I should notice is that I have tested this patch
and couldn't find any critical problem so I rebased patchset
onto recent mmotm(ie, mmotm-2015-10-15-15-20) to send formal
patchset. Unfortunately, I start to see sudden discarding of
the page we shouldn't do. IOW, application's valid anonymous page
was disappeared suddenly.

When I look through THP changes, I think we could lose
dirty bit of pte between freeze_page and unfreeze_page
when we mark it as migration entry and restore it.
So, I added below simple code without enough considering
and cannot see the problem any more.
I hope it's good hint to find right fix this problem.

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index d5ea516ffb54..e881c04f5950 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -3138,6 +3138,9 @@ static void unfreeze_page_vma(struct vm_area_struct *vma, struct page *page,
 		if (is_write_migration_entry(swp_entry))
 			entry = maybe_mkwrite(entry, vma);
 
+		if (PageDirty(page))
+			SetPageDirty(page);
+
 		flush_dcache_page(page);
 		set_pte_at(vma->vm_mm, address, pte + i, entry);
 

Although it fixes abvove problem, I can encounter below another bug
in several hours.

	BUG: Bad rss-counter state mm:ffff88007fc28000 idx:1 val:439
	BUG: Bad rss-counter state mm:ffff88007fc28000 idx:2 val:73

Or

	BUG: Bad rss-counter state mm:ffff88007fc28000 idx:1 val:512

It seems we are zapping THP page without decreasing MM_ANONPAGES
and MM_SWAPENTS. Of course, it could be a bug of MADV_FREE and
recent changes of THP reveals it. What I can say is I couldn't see
any problem until mmotm-2015-10-06-16-30 so I guess there is some
conflict with THP-refcount redesign of Kirill or it makes to reveal
MADV_FREE's hidden bug.

I will hunt it down but I hope Kirill might catch it up earlier than me.

Major thing with this patch is two things.

1. Work with MADV_FREE on PG_dirty page.

So far, MADV_FREE doesn't work with page which is not in swap cache
but has PG_dirty(ex, swapped-in page). Details are in [3/5].

2. Make MADV_FREE discard path simple

Current logic for discarding hinted page is really mess
so [4/5] makes it simple and clean.

3. Fix with KSM page

A process can have KSM page which is no dirty bit in page table
entry and no PG_dirty in page->flags so VM could discard it wrongly.
[5/5] fixes it.

Minchan Kim (5):
  [1/5] mm: MADV_FREE trivial clean up
  [2/5] mm: skip huge zero page in MADV_FREE
  [3/5] mm: clear PG_dirty to mark page freeable
  [4/5] mm: simplify reclaim path for MADV_FREE
  [5/5] mm: mark stable page dirty in KSM

 include/linux/rmap.h |  6 +----
 mm/huge_memory.c     |  9 ++++----
 mm/ksm.c             | 12 ++++++++++
 mm/madvise.c         | 29 +++++++++++-------------
 mm/rmap.c            | 46 +++++++------------------------------
 mm/swap_state.c      |  5 ++--
 mm/vmscan.c          | 64 ++++++++++++++++------------------------------------
 7 files changed, 60 insertions(+), 111 deletions(-)

-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1250367 — [PATCH 2/5] mm: skip huge zero page in MADV_FREE

It is pointless to mark huge zero page as freeable.
Let's skip it.

Signed-off-by: Minchan Kim <minchan@kernel.org>
---
 mm/huge_memory.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index f1de4ce583a6..269ed99493f0 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -1542,6 +1542,9 @@ int madvise_free_huge_pmd(struct mmu_gather *tlb, struct vm_area_struct *vma,
 		struct page *page;
 		pmd_t orig_pmd;
 
+		if (is_huge_zero_pmd(*pmd))
+			goto out;
+
 		orig_pmd = pmdp_huge_get_and_clear(mm, addr, pmd);
 
 		/* No hugepage in swapcache */
@@ -1553,6 +1556,7 @@ int 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);
+out:
 		spin_unlock(ptl);
 		ret = 0;
 	}
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1250368 — [PATCH 1/5] mm: MADV_FREE trivial clean up

1. Page table waker already pass the vma it is processing
so we don't need to pass vma.

2. If page table entry is dirty in try_to_unmap_one, the dirtiness
should propagate to PG_dirty of the page. So, it's enough to check
only PageDirty without other pte dirty bit checking.

Signed-off-by: Minchan Kim <minchan@kernel.org>
---
 mm/madvise.c | 17 +++--------------
 mm/rmap.c    |  6 ++----
 2 files changed, 5 insertions(+), 18 deletions(-)

diff --git a/mm/madvise.c b/mm/madvise.c
index 7835bc1eaccb..fdfb14a78c60 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -24,11 +24,6 @@
 
 #include <asm/tlb.h>
 
-struct madvise_free_private {
-	struct vm_area_struct *vma;
-	struct mmu_gather *tlb;
-};
-
 /*
  * Any behaviour which results in changes to the vma->vm_flags needs to
  * take mmap_sem for writing. Others, which simply traverse vmas, need
@@ -269,10 +264,9 @@ static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
 				unsigned long end, struct mm_walk *walk)
 
 {
-	struct madvise_free_private *fp = walk->private;
-	struct mmu_gather *tlb = fp->tlb;
+	struct mmu_gather *tlb = walk->private;
 	struct mm_struct *mm = tlb->mm;
-	struct vm_area_struct *vma = fp->vma;
+	struct vm_area_struct *vma = walk->vma;
 	spinlock_t *ptl;
 	pte_t *pte, ptent;
 	struct page *page;
@@ -365,15 +359,10 @@ static void madvise_free_page_range(struct mmu_gather *tlb,
 			     struct vm_area_struct *vma,
 			     unsigned long addr, unsigned long end)
 {
-	struct madvise_free_private fp = {
-		.vma = vma,
-		.tlb = tlb,
-	};
-
 	struct mm_walk free_walk = {
 		.pmd_entry = madvise_free_pte_range,
 		.mm = vma->vm_mm,
-		.private = &fp,
+		.private = tlb,
 	};
 
 	BUG_ON(addr >= end);
diff --git a/mm/rmap.c b/mm/rmap.c
index 6f0f9331a20f..94ee372e238b 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -1380,7 +1380,6 @@ static int try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
 	spinlock_t *ptl;
 	int ret = SWAP_AGAIN;
 	enum ttu_flags flags = (enum ttu_flags)arg;
-	int dirty = 0;
 
 	pte = page_check_address(page, mm, address, &ptl, 0);
 	if (!pte)
@@ -1423,8 +1422,7 @@ static int try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
 	}
 
 	/* Move the dirty bit to the physical page now the pte is gone. */
-	dirty = pte_dirty(pteval);
-	if (dirty)
+	if (pte_dirty(pteval))
 		set_page_dirty(page);
 
 	/* Update high watermark before we lower rss */
@@ -1457,7 +1455,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 (!PageDirty(page)) {
 				/* It's a freeable page by MADV_FREE */
 				dec_mm_counter(mm, MM_ANONPAGES);
 				goto discard;
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1250370 — [PATCH 5/5] mm: mark stable page dirty in KSM

Stable page could be shared by several processes and last process
could own the page among them after CoW or zapping for every process
except last process happens. Then, page table entry of the page
in last process can have no dirty bit and PG_dirty flag in page->flags.
In this case, MADV_FREE could discard the page wrongly.
For preventing it, we mark stable page dirty.

Cc: Hugh Dickins <hughd@google.com>
Signed-off-by: Minchan Kim <minchan@kernel.org>
---
 mm/ksm.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/mm/ksm.c b/mm/ksm.c
index 8f0faf809bf5..659e2b5119c0 100644
--- a/mm/ksm.c
+++ b/mm/ksm.c
@@ -1050,6 +1050,18 @@ static int try_to_merge_one_page(struct vm_area_struct *vma,
 			 */
 			set_page_stable_node(page, NULL);
 			mark_page_accessed(page);
+			/*
+			 * Stable page could be shared by several processes
+			 * and last process could own the page among them after
+			 * CoW or zapping for every process except last process
+			 * happens. Then, page table entry of the page
+			 * in last process can have no dirty bit.
+			 * In this case, MADV_FREE could discard the page
+			 * wrongly.
+			 * For preventing it, we mark stable page dirty.
+			 */
+			if (!PageDirty(page))
+				SetPageDirty(page);
 			err = 0;
 		} else if (pages_identical(page, kpage))
 			err = replace_page(vma, page, kpage, orig_pte);
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1256410 — Re: [PATCH 5/5] mm: mark stable page dirty in KSM

On Mon, 19 Oct 2015, Minchan Kim wrote:

> Stable page could be shared by several processes and last process
> could own the page among them after CoW or zapping for every process
> except last process happens. Then, page table entry of the page
> in last process can have no dirty bit and PG_dirty flag in page->flags.
> In this case, MADV_FREE could discard the page wrongly.
> For preventing it, we mark stable page dirty.

I agree with the change, but found that comment (repeated in the source)
rather hard to follow.  And it doesn't really do justice to the changes
you have made.

This is not now a MADV_FREE thing, it's more general than that, even
if MADV_FREE is the only thing that takes advantage of it.  I like
very much that you've made page reclaim sane, freeing non-dirty
anonymous pages instead of swapping them out, without having to
think of whether it's for MADV_FREE or not.

Would you mind if we replace your patch by a re-commented version?

[PATCH] mm: mark stable page dirty in KSM

The MADV_FREE patchset changes page reclaim to simply free a clean
anonymous page with no dirty ptes, instead of swapping it out; but
KSM uses clean write-protected ptes to reference the stable ksm page.
So be sure to mark that page dirty, so it's never mistakenly discarded.

Signed-off-by: Minchan Kim <minchan@kernel.org>
Signed-off-by: Hugh Dickins <hughd@google.com>
---

 mm/ksm.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff -puN mm/ksm.c~mm-mark-stable-page-dirty-in-ksm mm/ksm.c
--- a/mm/ksm.c~mm-mark-stable-page-dirty-in-ksm
+++ a/mm/ksm.c
@@ -1050,6 +1050,12 @@ static int try_to_merge_one_page(struct
 			 */
 			set_page_stable_node(page, NULL);
 			mark_page_accessed(page);
+			/*
+			 * Page reclaim just frees a clean page with no dirty
+			 * ptes: make sure that the ksm page would be swapped.
+			 */
+			if (!PageDirty(page))
+				SetPageDirty(page);
 			err = 0;
 		} else if (pages_identical(page, kpage))
 			err = replace_page(vma, page, kpage, orig_pte);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1256508 — Re: [PATCH 5/5] mm: mark stable page dirty in KSM

On Mon, Oct 26, 2015 at 07:23:12PM -0700, Hugh Dickins wrote:
> On Mon, 19 Oct 2015, Minchan Kim wrote:
> 
> > Stable page could be shared by several processes and last process
> > could own the page among them after CoW or zapping for every process
> > except last process happens. Then, page table entry of the page
> > in last process can have no dirty bit and PG_dirty flag in page->flags.
> > In this case, MADV_FREE could discard the page wrongly.
> > For preventing it, we mark stable page dirty.
> 
> I agree with the change, but found that comment (repeated in the source)
> rather hard to follow.  And it doesn't really do justice to the changes
> you have made.
> 
> This is not now a MADV_FREE thing, it's more general than that, even
> if MADV_FREE is the only thing that takes advantage of it.  I like
> very much that you've made page reclaim sane, freeing non-dirty
> anonymous pages instead of swapping them out, without having to
> think of whether it's for MADV_FREE or not.
> 
> Would you mind if we replace your patch by a re-commented version?
> 
> [PATCH] mm: mark stable page dirty in KSM
> 
> The MADV_FREE patchset changes page reclaim to simply free a clean
> anonymous page with no dirty ptes, instead of swapping it out; but
> KSM uses clean write-protected ptes to reference the stable ksm page.
> So be sure to mark that page dirty, so it's never mistakenly discarded.
> 
> Signed-off-by: Minchan Kim <minchan@kernel.org>
> Signed-off-by: Hugh Dickins <hughd@google.com>

Looks better than mine.
I will include this in my patchset when I respin.

Thanks!
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1250372 — [PATCH 3/5] mm: clear PG_dirty to mark page freeable

Basically, MADV_FREE relies on dirty bit in page table entry
to decide whether VM allows to discard the page or not.
IOW, if page table entry includes marked dirty bit, VM shouldn't
discard the page.

However, as a example, if swap-in by read fault happens,
page table entry doesn't have dirty bit so MADV_FREE could discard
the page wrongly.

For avoiding the problem, MADV_FREE did more checks with PageDirty
and PageSwapCache. It worked out because swapped-in page lives on
swap cache and since it is evicted from the swap cache, the page has
PG_dirty flag. So both page flags check effectively prevent
wrong discarding by MADV_FREE.

However, a problem in above logic is that swapped-in page has
PG_dirty still after they are removed from swap cache so VM cannot
consider the page as freeable any more even if madvise_free is
called in future.

Look at below example for detail.

    ptr = malloc();
    memset(ptr);
    ..
    ..
    .. heavy memory pressure so all of pages are swapped out
    ..
    ..
    var = *ptr; -> a page swapped-in and could be removed from
                   swapcache. Then, page table doesn't mark
                   dirty bit and page descriptor includes PG_dirty
    ..
    ..
    madvise_free(ptr); -> It doesn't clear PG_dirty of the page.
    ..
    ..
    ..
    .. heavy memory pressure again.
    .. In this time, VM cannot discard the page because the page
    .. has *PG_dirty*

To solve the problem, this patch clears PG_dirty if only the page
is owned exclusively by current process when madvise is called
because PG_dirty represents ptes's dirtiness in several processes
so we could clear it only if we own it exclusively.

Cc: Hugh Dickins <hughd@google.com>
Signed-off-by: Minchan Kim <minchan@kernel.org>
---
 mm/madvise.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/mm/madvise.c b/mm/madvise.c
index fdfb14a78c60..5db546431285 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -312,11 +312,19 @@ static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
 		if (!page)
 			continue;
 
-		if (PageSwapCache(page)) {
+		if (PageSwapCache(page) || PageDirty(page)) {
 			if (!trylock_page(page))
 				continue;
+			/*
+			 * If page is shared with others, we couldn't clear
+			 * PG_dirty of the page.
+			 */
+			if (page_count(page) != 1 + !!PageSwapCache(page)) {
+				unlock_page(page);
+				continue;
+			}
 
-			if (!try_to_free_swap(page)) {
+			if (PageSwapCache(page) && !try_to_free_swap(page)) {
 				unlock_page(page);
 				continue;
 			}
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1256382 — Re: [PATCH 3/5] mm: clear PG_dirty to mark page freeable

On Mon, 19 Oct 2015, Minchan Kim wrote:

> Basically, MADV_FREE relies on dirty bit in page table entry
> to decide whether VM allows to discard the page or not.
> IOW, if page table entry includes marked dirty bit, VM shouldn't
> discard the page.
> 
> However, as a example, if swap-in by read fault happens,
> page table entry doesn't have dirty bit so MADV_FREE could discard
> the page wrongly.
> 
> For avoiding the problem, MADV_FREE did more checks with PageDirty
> and PageSwapCache. It worked out because swapped-in page lives on
> swap cache and since it is evicted from the swap cache, the page has
> PG_dirty flag. So both page flags check effectively prevent
> wrong discarding by MADV_FREE.
> 
> However, a problem in above logic is that swapped-in page has
> PG_dirty still after they are removed from swap cache so VM cannot
> consider the page as freeable any more even if madvise_free is
> called in future.
> 
> Look at below example for detail.
> 
>     ptr = malloc();
>     memset(ptr);
>     ..
>     ..
>     .. heavy memory pressure so all of pages are swapped out
>     ..
>     ..
>     var = *ptr; -> a page swapped-in and could be removed from
>                    swapcache. Then, page table doesn't mark
>                    dirty bit and page descriptor includes PG_dirty
>     ..
>     ..
>     madvise_free(ptr); -> It doesn't clear PG_dirty of the page.
>     ..
>     ..
>     ..
>     .. heavy memory pressure again.
>     .. In this time, VM cannot discard the page because the page
>     .. has *PG_dirty*
> 
> To solve the problem, this patch clears PG_dirty if only the page
> is owned exclusively by current process when madvise is called
> because PG_dirty represents ptes's dirtiness in several processes
> so we could clear it only if we own it exclusively.
> 
> Cc: Hugh Dickins <hughd@google.com>
> Signed-off-by: Minchan Kim <minchan@kernel.org>

Acked-by: Hugh Dickins <hughd@google.com>

(and patches 1/5 and 2/5 too if you like)

> ---
>  mm/madvise.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/madvise.c b/mm/madvise.c
> index fdfb14a78c60..5db546431285 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -312,11 +312,19 @@ static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
>  		if (!page)
>  			continue;
>  
> -		if (PageSwapCache(page)) {
> +		if (PageSwapCache(page) || PageDirty(page)) {
>  			if (!trylock_page(page))
>  				continue;
> +			/*
> +			 * If page is shared with others, we couldn't clear
> +			 * PG_dirty of the page.
> +			 */
> +			if (page_count(page) != 1 + !!PageSwapCache(page)) {
> +				unlock_page(page);
> +				continue;
> +			}
>  
> -			if (!try_to_free_swap(page)) {
> +			if (PageSwapCache(page) && !try_to_free_swap(page)) {
>  				unlock_page(page);
>  				continue;
>  			}
> -- 
> 1.9.1
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1256506 — Re: [PATCH 3/5] mm: clear PG_dirty to mark page freeable

On Mon, Oct 26, 2015 at 06:28:13PM -0700, Hugh Dickins wrote:
> On Mon, 19 Oct 2015, Minchan Kim wrote:
> 
> > Basically, MADV_FREE relies on dirty bit in page table entry
> > to decide whether VM allows to discard the page or not.
> > IOW, if page table entry includes marked dirty bit, VM shouldn't
> > discard the page.
> > 
> > However, as a example, if swap-in by read fault happens,
> > page table entry doesn't have dirty bit so MADV_FREE could discard
> > the page wrongly.
> > 
> > For avoiding the problem, MADV_FREE did more checks with PageDirty
> > and PageSwapCache. It worked out because swapped-in page lives on
> > swap cache and since it is evicted from the swap cache, the page has
> > PG_dirty flag. So both page flags check effectively prevent
> > wrong discarding by MADV_FREE.
> > 
> > However, a problem in above logic is that swapped-in page has
> > PG_dirty still after they are removed from swap cache so VM cannot
> > consider the page as freeable any more even if madvise_free is
> > called in future.
> > 
> > Look at below example for detail.
> > 
> >     ptr = malloc();
> >     memset(ptr);
> >     ..
> >     ..
> >     .. heavy memory pressure so all of pages are swapped out
> >     ..
> >     ..
> >     var = *ptr; -> a page swapped-in and could be removed from
> >                    swapcache. Then, page table doesn't mark
> >                    dirty bit and page descriptor includes PG_dirty
> >     ..
> >     ..
> >     madvise_free(ptr); -> It doesn't clear PG_dirty of the page.
> >     ..
> >     ..
> >     ..
> >     .. heavy memory pressure again.
> >     .. In this time, VM cannot discard the page because the page
> >     .. has *PG_dirty*
> > 
> > To solve the problem, this patch clears PG_dirty if only the page
> > is owned exclusively by current process when madvise is called
> > because PG_dirty represents ptes's dirtiness in several processes
> > so we could clear it only if we own it exclusively.
> > 
> > Cc: Hugh Dickins <hughd@google.com>
> > Signed-off-by: Minchan Kim <minchan@kernel.org>
> 
> Acked-by: Hugh Dickins <hughd@google.com>
> 
> (and patches 1/5 and 2/5 too if you like)
> 

Thanks for the review, Hugh!

I will rebase all series from the beginning as you suggested
and will add your Acked-by because I feel you just reviewed
all of MADV_FREE code line and you have no found any problem.

If something happens(ie, I abuse your Acked-by) wrong, please
shout me.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1250378 — [PATCH 4/5] mm: simplify reclaim path for MADV_FREE

I made reclaim path mess to check and free MADV_FREEed page.
This patch simplify it with tweaking add_to_swap.

So far, we mark page as PG_dirty when we add the page into
swap cache(ie, add_to_swap) to page out to swap device but
this patch moves PG_dirty marking under try_to_unmap_one
when we decide to change pte from anon to swapent so if
any process's pte has swapent for the page, the page must
be swapped out. IOW, there should be no funcional behavior
change. It makes relcaim path really simple for MADV_FREE
because we just need to check PG_dirty of page to decide
discarding the page or not.

Other thing this patch does is to pass TTU_BATCH_FLUSH to
try_to_unmap when we handle freeable page because I don't
see any reason to prevent it.

Cc: Hugh Dickins <hughd@google.com>
Cc: Mel Gorman <mgorman@suse.de>
Signed-off-by: Minchan Kim <minchan@kernel.org>
---
 include/linux/rmap.h |  6 +----
 mm/huge_memory.c     |  5 ----
 mm/rmap.c            | 42 ++++++----------------------------
 mm/swap_state.c      |  5 ++--
 mm/vmscan.c          | 64 ++++++++++++++++------------------------------------
 5 files changed, 30 insertions(+), 92 deletions(-)

diff --git a/include/linux/rmap.h b/include/linux/rmap.h
index 6b6233fafb53..978f65066fd5 100644
--- a/include/linux/rmap.h
+++ b/include/linux/rmap.h
@@ -193,8 +193,7 @@ static inline void page_dup_rmap(struct page *page, bool compound)
  * Called from mm/vmscan.c to handle paging out
  */
 int page_referenced(struct page *, int is_locked,
-			struct mem_cgroup *memcg, unsigned long *vm_flags,
-			int *is_pte_dirty);
+			struct mem_cgroup *memcg, unsigned long *vm_flags);
 
 #define TTU_ACTION(x) ((x) & TTU_ACTION_MASK)
 
@@ -272,11 +271,8 @@ int rmap_walk(struct page *page, struct rmap_walk_control *rwc);
 static inline int page_referenced(struct page *page, int is_locked,
 				  struct mem_cgroup *memcg,
 				  unsigned long *vm_flags,
-				  int *is_pte_dirty)
 {
 	*vm_flags = 0;
-	if (is_pte_dirty)
-		*is_pte_dirty = 0;
 	return 0;
 }
 
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 269ed99493f0..adccfb48ce57 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -1753,11 +1753,6 @@ pmd_t *page_check_address_pmd(struct page *page,
 	return NULL;
 }
 
-int pmd_freeable(pmd_t pmd)
-{
-	return !pmd_dirty(pmd);
-}
-
 #define VM_NO_THP (VM_SPECIAL | VM_HUGETLB | VM_SHARED | VM_MAYSHARE)
 
 int hugepage_madvise(struct vm_area_struct *vma,
diff --git a/mm/rmap.c b/mm/rmap.c
index 94ee372e238b..fd64f79c87c4 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -797,7 +797,6 @@ int page_mapped_in_vma(struct page *page, struct vm_area_struct *vma)
 }
 
 struct page_referenced_arg {
-	int dirtied;
 	int mapcount;
 	int referenced;
 	unsigned long vm_flags;
@@ -812,7 +811,6 @@ static int page_referenced_one(struct page *page, struct vm_area_struct *vma,
 	struct mm_struct *mm = vma->vm_mm;
 	spinlock_t *ptl;
 	int referenced = 0;
-	int dirty = 0;
 	struct page_referenced_arg *pra = arg;
 
 	if (unlikely(PageTransHuge(page))) {
@@ -835,14 +833,6 @@ static int page_referenced_one(struct page *page, struct vm_area_struct *vma,
 		if (pmdp_clear_flush_young_notify(vma, address, pmd))
 			referenced++;
 
-		/*
-		 * Use pmd_freeable instead of raw pmd_dirty because in some
-		 * of architecture, pmd_dirty is not defined unless
-		 * CONFIG_TRANSPARENT_HUGEPAGE is enabled
-		 */
-		if (!pmd_freeable(*pmd))
-			dirty++;
-
 		spin_unlock(ptl);
 	} else {
 		pte_t *pte;
@@ -873,9 +863,6 @@ static int page_referenced_one(struct page *page, struct vm_area_struct *vma,
 				referenced++;
 		}
 
-		if (pte_dirty(*pte))
-			dirty++;
-
 		pte_unmap_unlock(pte, ptl);
 	}
 
@@ -889,9 +876,6 @@ static int page_referenced_one(struct page *page, struct vm_area_struct *vma,
 		pra->vm_flags |= vma->vm_flags;
 	}
 
-	if (dirty)
-		pra->dirtied++;
-
 	pra->mapcount--;
 	if (!pra->mapcount)
 		return SWAP_SUCCESS; /* To break the loop */
@@ -916,7 +900,6 @@ static bool invalid_page_referenced_vma(struct vm_area_struct *vma, void *arg)
  * @is_locked: caller holds lock on the page
  * @memcg: target memory cgroup
  * @vm_flags: collect encountered vma->vm_flags who actually referenced the page
- * @is_pte_dirty: ptes which have marked dirty bit - used for lazyfree page
  *
  * Quick test_and_clear_referenced for all mappings to a page,
  * returns the number of ptes which referenced the page.
@@ -924,8 +907,7 @@ static bool invalid_page_referenced_vma(struct vm_area_struct *vma, void *arg)
 int page_referenced(struct page *page,
 		    int is_locked,
 		    struct mem_cgroup *memcg,
-		    unsigned long *vm_flags,
-		    int *is_pte_dirty)
+		    unsigned long *vm_flags)
 {
 	int ret;
 	int we_locked = 0;
@@ -940,8 +922,6 @@ int page_referenced(struct page *page,
 	};
 
 	*vm_flags = 0;
-	if (is_pte_dirty)
-		*is_pte_dirty = 0;
 
 	if (!page_mapped(page))
 		return 0;
@@ -970,9 +950,6 @@ int page_referenced(struct page *page,
 	if (we_locked)
 		unlock_page(page);
 
-	if (is_pte_dirty)
-		*is_pte_dirty = pra.dirtied;
-
 	return pra.referenced;
 }
 
@@ -1453,17 +1430,10 @@ static int try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
 		swp_entry_t entry = { .val = page_private(page) };
 		pte_t swp_pte;
 
-		if (flags & TTU_FREE) {
-			VM_BUG_ON_PAGE(PageSwapCache(page), page);
-			if (!PageDirty(page)) {
-				/* It's a freeable page by MADV_FREE */
-				dec_mm_counter(mm, MM_ANONPAGES);
-				goto discard;
-			} else {
-				set_pte_at(mm, address, pte, pteval);
-				ret = SWAP_FAIL;
-				goto out_unmap;
-			}
+		if (!PageDirty(page) && (flags & TTU_FREE)) {
+			/* It's a freeable page by MADV_FREE */
+			dec_mm_counter(mm, MM_ANONPAGES);
+			goto discard;
 		}
 
 		if (PageSwapCache(page)) {
@@ -1476,6 +1446,8 @@ static int try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
 				ret = SWAP_FAIL;
 				goto out_unmap;
 			}
+			if (!PageDirty(page))
+				SetPageDirty(page);
 			if (list_empty(&mm->mmlist)) {
 				spin_lock(&mmlist_lock);
 				if (list_empty(&mm->mmlist))
diff --git a/mm/swap_state.c b/mm/swap_state.c
index d783872d746c..676ff2991380 100644
--- a/mm/swap_state.c
+++ b/mm/swap_state.c
@@ -185,13 +185,12 @@ int add_to_swap(struct page *page, struct list_head *list)
 	 * deadlock in the swap out path.
 	 */
 	/*
-	 * Add it to the swap cache and mark it dirty
+	 * Add it to the swap cache.
 	 */
 	err = add_to_swap_cache(page, entry,
 			__GFP_HIGH|__GFP_NOMEMALLOC|__GFP_NOWARN);
 
-	if (!err) {	/* Success */
-		SetPageDirty(page);
+	if (!err) {
 		return 1;
 	} else {	/* -ENOMEM radix-tree allocation failure */
 		/*
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 27d580b5e853..9b52ecf91194 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -791,17 +791,15 @@ enum page_references {
 };
 
 static enum page_references page_check_references(struct page *page,
-						  struct scan_control *sc,
-						  bool *freeable)
+						  struct scan_control *sc)
 {
 	int referenced_ptes, referenced_page;
 	unsigned long vm_flags;
-	int pte_dirty;
 
 	VM_BUG_ON_PAGE(!PageLocked(page), page);
 
 	referenced_ptes = page_referenced(page, 1, sc->target_mem_cgroup,
-					  &vm_flags, &pte_dirty);
+					  &vm_flags);
 	referenced_page = TestClearPageReferenced(page);
 
 	/*
@@ -842,10 +840,6 @@ static enum page_references page_check_references(struct page *page,
 		return PAGEREF_KEEP;
 	}
 
-	if (PageAnon(page) && !pte_dirty && !PageSwapCache(page) &&
-			!PageDirty(page))
-		*freeable = true;
-
 	/* Reclaim if clean, defer dirty pages to writeback */
 	if (referenced_page && !PageSwapBacked(page))
 		return PAGEREF_RECLAIM_CLEAN;
@@ -1037,8 +1031,7 @@ static unsigned long shrink_page_list(struct list_head *page_list,
 		}
 
 		if (!force_reclaim)
-			references = page_check_references(page, sc,
-							&freeable);
+			references = page_check_references(page, sc);
 
 		switch (references) {
 		case PAGEREF_ACTIVATE:
@@ -1055,31 +1048,24 @@ static unsigned long shrink_page_list(struct list_head *page_list,
 		 * Try to allocate it some swap space here.
 		 */
 		if (PageAnon(page) && !PageSwapCache(page)) {
-			if (!freeable) {
-				if (!(sc->gfp_mask & __GFP_IO))
-					goto keep_locked;
-				if (!add_to_swap(page, page_list))
-					goto activate_locked;
-				may_enter_fs = 1;
-				/* Adding to swap updated mapping */
-				mapping = page_mapping(page);
-			} else {
-				if (likely(!PageTransHuge(page)))
-					goto unmap;
-				/* try_to_unmap isn't aware of THP page */
-				if (unlikely(split_huge_page_to_list(page,
-								page_list)))
-					goto keep_locked;
-			}
+			if (!(sc->gfp_mask & __GFP_IO))
+				goto keep_locked;
+			if (!add_to_swap(page, page_list))
+				goto activate_locked;
+			freeable = true;
+			may_enter_fs = 1;
+			/* Adding to swap updated mapping */
+			mapping = page_mapping(page);
 		}
-unmap:
+
 		/*
 		 * The page is mapped into the page tables of one or more
 		 * processes. Try to unmap it here.
 		 */
-		if (page_mapped(page) && (mapping || freeable)) {
+		if (page_mapped(page) && mapping) {
 			switch (try_to_unmap(page, freeable ?
-					TTU_FREE : ttu_flags|TTU_BATCH_FLUSH)) {
+					ttu_flags | TTU_BATCH_FLUSH | TTU_FREE :
+					ttu_flags | TTU_BATCH_FLUSH)) {
 			case SWAP_FAIL:
 				goto activate_locked;
 			case SWAP_AGAIN:
@@ -1087,20 +1073,7 @@ static unsigned long shrink_page_list(struct list_head *page_list,
 			case SWAP_MLOCK:
 				goto cull_mlocked;
 			case SWAP_SUCCESS:
-				/* try to free the page below */
-				if (!freeable)
-					break;
-				/*
-				 * Freeable anon page doesn't have mapping
-				 * due to skipping of swapcache so we free
-				 * page in here rather than __remove_mapping.
-				 */
-				VM_BUG_ON_PAGE(PageSwapCache(page), page);
-				if (!page_freeze_refs(page, 1))
-					goto keep_locked;
-				__ClearPageLocked(page);
-				count_vm_event(PGLAZYFREED);
-				goto free_it;
+				; /* try to free the page below */
 			}
 		}
 
@@ -1217,6 +1190,9 @@ static unsigned long shrink_page_list(struct list_head *page_list,
 		 */
 		__ClearPageLocked(page);
 free_it:
+		if (freeable && !PageDirty(page))
+			count_vm_event(PGLAZYFREED);
+
 		nr_reclaimed++;
 
 		/*
@@ -1847,7 +1823,7 @@ static void shrink_active_list(unsigned long nr_to_scan,
 		}
 
 		if (page_referenced(page, 0, sc->target_mem_cgroup,
-				    &vm_flags, NULL)) {
+				    &vm_flags)) {
 			nr_rotated += hpage_nr_pages(page);
 			/*
 			 * Identify referenced, file-backed active pages and
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1256401 — Re: [PATCH 4/5] mm: simplify reclaim path for MADV_FREE

On Mon, 19 Oct 2015, Minchan Kim wrote:

> I made reclaim path mess to check and free MADV_FREEed page.
> This patch simplify it with tweaking add_to_swap.
> 
> So far, we mark page as PG_dirty when we add the page into
> swap cache(ie, add_to_swap) to page out to swap device but
> this patch moves PG_dirty marking under try_to_unmap_one
> when we decide to change pte from anon to swapent so if
> any process's pte has swapent for the page, the page must
> be swapped out. IOW, there should be no funcional behavior
> change. It makes relcaim path really simple for MADV_FREE
> because we just need to check PG_dirty of page to decide
> discarding the page or not.
> 
> Other thing this patch does is to pass TTU_BATCH_FLUSH to
> try_to_unmap when we handle freeable page because I don't
> see any reason to prevent it.
> 
> Cc: Hugh Dickins <hughd@google.com>
> Cc: Mel Gorman <mgorman@suse.de>
> Signed-off-by: Minchan Kim <minchan@kernel.org>

Acked-by: Hugh Dickins <hughd@google.com>

This is sooooooo much nicer than the code it replaces!  Really good.
Kudos also to Hannes for suggesting this approach originally, I think.

I hope this implementation satisfies a good proportion of the people
who have been wanting MADV_FREE: I'm not among them, and have long
lost touch with those discussions, so won't judge how usable it is.

I assume you'll refactor the series again before it goes to Linus,
so the previous messier implementations vanish?  I notice Andrew
has this "mm: simplify reclaim path for MADV_FREE" in mmotm as
mm-dont-split-thp-page-when-syscall-is-called-fix-6.patch:
I guess it all got much too messy to divide up in a hurry.

I've noticed no problems in testing (unlike the first time you moved
to working with pte_dirty); though of course I've not been using
MADV_FREE itself at all.

One aspect has worried me for a while, but I think I've reached the
conclusion that it doesn't matter at all.  The swap that's allocated
in add_to_swap() would normally get freed again (after try_to_unmap
found it was a MADV_FREE !pte_dirty !PageDirty case) at the bottom
of shrink_page_list(), in __remove_mapping(), yes?

The bit that worried me is that on rare occasions, something unknown
might take a speculative reference to the page, and __remove_mapping()
fail to freeze refs for that reason.  Much too rare to worry over not
freeing that page immediately, but it leaves us with a PageUptodate
PageSwapCache !PageDirty page, yet its contents are not the contents
of that location on swap.

But since this can only happen when you have *not* inserted the
corresponding swapent anywhere, I cannot think of anything that would
have a legitimate interest in its contents matching that location on swap.
So I don't think it's worth looking for somewhere to add a SetPageDirty
(or a delete_from_swap_cache) just to regularize that case.

> ---
>  include/linux/rmap.h |  6 +----
>  mm/huge_memory.c     |  5 ----
>  mm/rmap.c            | 42 ++++++----------------------------
>  mm/swap_state.c      |  5 ++--
>  mm/vmscan.c          | 64 ++++++++++++++++------------------------------------
>  5 files changed, 30 insertions(+), 92 deletions(-)
> 
> diff --git a/include/linux/rmap.h b/include/linux/rmap.h
> index 6b6233fafb53..978f65066fd5 100644
> --- a/include/linux/rmap.h
> +++ b/include/linux/rmap.h
> @@ -193,8 +193,7 @@ static inline void page_dup_rmap(struct page *page, bool compound)
>   * Called from mm/vmscan.c to handle paging out
>   */
>  int page_referenced(struct page *, int is_locked,
> -			struct mem_cgroup *memcg, unsigned long *vm_flags,
> -			int *is_pte_dirty);
> +			struct mem_cgroup *memcg, unsigned long *vm_flags);
>  
>  #define TTU_ACTION(x) ((x) & TTU_ACTION_MASK)
>  
> @@ -272,11 +271,8 @@ int rmap_walk(struct page *page, struct rmap_walk_control *rwc);
>  static inline int page_referenced(struct page *page, int is_locked,
>  				  struct mem_cgroup *memcg,
>  				  unsigned long *vm_flags,
> -				  int *is_pte_dirty)
>  {
>  	*vm_flags = 0;
> -	if (is_pte_dirty)
> -		*is_pte_dirty = 0;
>  	return 0;
>  }
>  
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 269ed99493f0..adccfb48ce57 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -1753,11 +1753,6 @@ pmd_t *page_check_address_pmd(struct page *page,
>  	return NULL;
>  }
>  
> -int pmd_freeable(pmd_t pmd)
> -{
> -	return !pmd_dirty(pmd);
> -}
> -
>  #define VM_NO_THP (VM_SPECIAL | VM_HUGETLB | VM_SHARED | VM_MAYSHARE)
>  
>  int hugepage_madvise(struct vm_area_struct *vma,
> diff --git a/mm/rmap.c b/mm/rmap.c
> index 94ee372e238b..fd64f79c87c4 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -797,7 +797,6 @@ int page_mapped_in_vma(struct page *page, struct vm_area_struct *vma)
>  }
>  
>  struct page_referenced_arg {
> -	int dirtied;
>  	int mapcount;
>  	int referenced;
>  	unsigned long vm_flags;
> @@ -812,7 +811,6 @@ static int page_referenced_one(struct page *page, struct vm_area_struct *vma,
>  	struct mm_struct *mm = vma->vm_mm;
>  	spinlock_t *ptl;
>  	int referenced = 0;
> -	int dirty = 0;
>  	struct page_referenced_arg *pra = arg;
>  
>  	if (unlikely(PageTransHuge(page))) {
> @@ -835,14 +833,6 @@ static int page_referenced_one(struct page *page, struct vm_area_struct *vma,
>  		if (pmdp_clear_flush_young_notify(vma, address, pmd))
>  			referenced++;
>  
> -		/*
> -		 * Use pmd_freeable instead of raw pmd_dirty because in some
> -		 * of architecture, pmd_dirty is not defined unless
> -		 * CONFIG_TRANSPARENT_HUGEPAGE is enabled
> -		 */
> -		if (!pmd_freeable(*pmd))
> -			dirty++;
> -
>  		spin_unlock(ptl);
>  	} else {
>  		pte_t *pte;
> @@ -873,9 +863,6 @@ static int page_referenced_one(struct page *page, struct vm_area_struct *vma,
>  				referenced++;
>  		}
>  
> -		if (pte_dirty(*pte))
> -			dirty++;
> -
>  		pte_unmap_unlock(pte, ptl);
>  	}
>  
> @@ -889,9 +876,6 @@ static int page_referenced_one(struct page *page, struct vm_area_struct *vma,
>  		pra->vm_flags |= vma->vm_flags;
>  	}
>  
> -	if (dirty)
> -		pra->dirtied++;
> -
>  	pra->mapcount--;
>  	if (!pra->mapcount)
>  		return SWAP_SUCCESS; /* To break the loop */
> @@ -916,7 +900,6 @@ static bool invalid_page_referenced_vma(struct vm_area_struct *vma, void *arg)
>   * @is_locked: caller holds lock on the page
>   * @memcg: target memory cgroup
>   * @vm_flags: collect encountered vma->vm_flags who actually referenced the page
> - * @is_pte_dirty: ptes which have marked dirty bit - used for lazyfree page
>   *
>   * Quick test_and_clear_referenced for all mappings to a page,
>   * returns the number of ptes which referenced the page.
> @@ -924,8 +907,7 @@ static bool invalid_page_referenced_vma(struct vm_area_struct *vma, void *arg)
>  int page_referenced(struct page *page,
>  		    int is_locked,
>  		    struct mem_cgroup *memcg,
> -		    unsigned long *vm_flags,
> -		    int *is_pte_dirty)
> +		    unsigned long *vm_flags)
>  {
>  	int ret;
>  	int we_locked = 0;
> @@ -940,8 +922,6 @@ int page_referenced(struct page *page,
>  	};
>  
>  	*vm_flags = 0;
> -	if (is_pte_dirty)
> -		*is_pte_dirty = 0;
>  
>  	if (!page_mapped(page))
>  		return 0;
> @@ -970,9 +950,6 @@ int page_referenced(struct page *page,
>  	if (we_locked)
>  		unlock_page(page);
>  
> -	if (is_pte_dirty)
> -		*is_pte_dirty = pra.dirtied;
> -
>  	return pra.referenced;
>  }
>  
> @@ -1453,17 +1430,10 @@ static int try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
>  		swp_entry_t entry = { .val = page_private(page) };
>  		pte_t swp_pte;
>  
> -		if (flags & TTU_FREE) {
> -			VM_BUG_ON_PAGE(PageSwapCache(page), page);
> -			if (!PageDirty(page)) {
> -				/* It's a freeable page by MADV_FREE */
> -				dec_mm_counter(mm, MM_ANONPAGES);
> -				goto discard;
> -			} else {
> -				set_pte_at(mm, address, pte, pteval);
> -				ret = SWAP_FAIL;
> -				goto out_unmap;
> -			}
> +		if (!PageDirty(page) && (flags & TTU_FREE)) {
> +			/* It's a freeable page by MADV_FREE */
> +			dec_mm_counter(mm, MM_ANONPAGES);
> +			goto discard;
>  		}
>  
>  		if (PageSwapCache(page)) {
> @@ -1476,6 +1446,8 @@ static int try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
>  				ret = SWAP_FAIL;
>  				goto out_unmap;
>  			}
> +			if (!PageDirty(page))
> +				SetPageDirty(page);
>  			if (list_empty(&mm->mmlist)) {
>  				spin_lock(&mmlist_lock);
>  				if (list_empty(&mm->mmlist))
> diff --git a/mm/swap_state.c b/mm/swap_state.c
> index d783872d746c..676ff2991380 100644
> --- a/mm/swap_state.c
> +++ b/mm/swap_state.c
> @@ -185,13 +185,12 @@ int add_to_swap(struct page *page, struct list_head *list)
>  	 * deadlock in the swap out path.
>  	 */
>  	/*
> -	 * Add it to the swap cache and mark it dirty
> +	 * Add it to the swap cache.
>  	 */
>  	err = add_to_swap_cache(page, entry,
>  			__GFP_HIGH|__GFP_NOMEMALLOC|__GFP_NOWARN);
>  
> -	if (!err) {	/* Success */
> -		SetPageDirty(page);
> +	if (!err) {
>  		return 1;
>  	} else {	/* -ENOMEM radix-tree allocation failure */
>  		/*
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 27d580b5e853..9b52ecf91194 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -791,17 +791,15 @@ enum page_references {
>  };
>  
>  static enum page_references page_check_references(struct page *page,
> -						  struct scan_control *sc,
> -						  bool *freeable)
> +						  struct scan_control *sc)
>  {
>  	int referenced_ptes, referenced_page;
>  	unsigned long vm_flags;
> -	int pte_dirty;
>  
>  	VM_BUG_ON_PAGE(!PageLocked(page), page);
>  
>  	referenced_ptes = page_referenced(page, 1, sc->target_mem_cgroup,
> -					  &vm_flags, &pte_dirty);
> +					  &vm_flags);
>  	referenced_page = TestClearPageReferenced(page);
>  
>  	/*
> @@ -842,10 +840,6 @@ static enum page_references page_check_references(struct page *page,
>  		return PAGEREF_KEEP;
>  	}
>  
> -	if (PageAnon(page) && !pte_dirty && !PageSwapCache(page) &&
> -			!PageDirty(page))
> -		*freeable = true;
> -
>  	/* Reclaim if clean, defer dirty pages to writeback */
>  	if (referenced_page && !PageSwapBacked(page))
>  		return PAGEREF_RECLAIM_CLEAN;
> @@ -1037,8 +1031,7 @@ static unsigned long shrink_page_list(struct list_head *page_list,
>  		}
>  
>  		if (!force_reclaim)
> -			references = page_check_references(page, sc,
> -							&freeable);
> +			references = page_check_references(page, sc);
>  
>  		switch (references) {
>  		case PAGEREF_ACTIVATE:
> @@ -1055,31 +1048,24 @@ static unsigned long shrink_page_list(struct list_head *page_list,
>  		 * Try to allocate it some swap space here.
>  		 */
>  		if (PageAnon(page) && !PageSwapCache(page)) {
> -			if (!freeable) {
> -				if (!(sc->gfp_mask & __GFP_IO))
> -					goto keep_locked;
> -				if (!add_to_swap(page, page_list))
> -					goto activate_locked;
> -				may_enter_fs = 1;
> -				/* Adding to swap updated mapping */
> -				mapping = page_mapping(page);
> -			} else {
> -				if (likely(!PageTransHuge(page)))
> -					goto unmap;
> -				/* try_to_unmap isn't aware of THP page */
> -				if (unlikely(split_huge_page_to_list(page,
> -								page_list)))
> -					goto keep_locked;
> -			}
> +			if (!(sc->gfp_mask & __GFP_IO))
> +				goto keep_locked;
> +			if (!add_to_swap(page, page_list))
> +				goto activate_locked;
> +			freeable = true;
> +			may_enter_fs = 1;
> +			/* Adding to swap updated mapping */
> +			mapping = page_mapping(page);
>  		}
> -unmap:
> +
>  		/*
>  		 * The page is mapped into the page tables of one or more
>  		 * processes. Try to unmap it here.
>  		 */
> -		if (page_mapped(page) && (mapping || freeable)) {
> +		if (page_mapped(page) && mapping) {
>  			switch (try_to_unmap(page, freeable ?
> -					TTU_FREE : ttu_flags|TTU_BATCH_FLUSH)) {
> +					ttu_flags | TTU_BATCH_FLUSH | TTU_FREE :
> +					ttu_flags | TTU_BATCH_FLUSH)) {
>  			case SWAP_FAIL:
>  				goto activate_locked;
>  			case SWAP_AGAIN:
> @@ -1087,20 +1073,7 @@ static unsigned long shrink_page_list(struct list_head *page_list,
>  			case SWAP_MLOCK:
>  				goto cull_mlocked;
>  			case SWAP_SUCCESS:
> -				/* try to free the page below */
> -				if (!freeable)
> -					break;
> -				/*
> -				 * Freeable anon page doesn't have mapping
> -				 * due to skipping of swapcache so we free
> -				 * page in here rather than __remove_mapping.
> -				 */
> -				VM_BUG_ON_PAGE(PageSwapCache(page), page);
> -				if (!page_freeze_refs(page, 1))
> -					goto keep_locked;
> -				__ClearPageLocked(page);
> -				count_vm_event(PGLAZYFREED);
> -				goto free_it;
> +				; /* try to free the page below */
>  			}
>  		}
>  
> @@ -1217,6 +1190,9 @@ static unsigned long shrink_page_list(struct list_head *page_list,
>  		 */
>  		__ClearPageLocked(page);
>  free_it:
> +		if (freeable && !PageDirty(page))
> +			count_vm_event(PGLAZYFREED);
> +
>  		nr_reclaimed++;
>  
>  		/*
> @@ -1847,7 +1823,7 @@ static void shrink_active_list(unsigned long nr_to_scan,
>  		}
>  
>  		if (page_referenced(page, 0, sc->target_mem_cgroup,
> -				    &vm_flags, NULL)) {
> +				    &vm_flags)) {
>  			nr_rotated += hpage_nr_pages(page);
>  			/*
>  			 * Identify referenced, file-backed active pages and
> -- 
> 1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1256448 — Re: [PATCH 4/5] mm: simplify reclaim path for MADV_FREE

> On Oct 27, 2015, at 10:09, Hugh Dickins <hughd@google.com> wrote:
> 
> On Mon, 19 Oct 2015, Minchan Kim wrote:
> 
>> I made reclaim path mess to check and free MADV_FREEed page.
>> This patch simplify it with tweaking add_to_swap.
>> 
>> So far, we mark page as PG_dirty when we add the page into
>> swap cache(ie, add_to_swap) to page out to swap device but
>> this patch moves PG_dirty marking under try_to_unmap_one
>> when we decide to change pte from anon to swapent so if
>> any process's pte has swapent for the page, the page must
>> be swapped out. IOW, there should be no funcional behavior
>> change. It makes relcaim path really simple for MADV_FREE
>> because we just need to check PG_dirty of page to decide
>> discarding the page or not.
>> 
>> Other thing this patch does is to pass TTU_BATCH_FLUSH to
>> try_to_unmap when we handle freeable page because I don't
>> see any reason to prevent it.
>> 
>> Cc: Hugh Dickins <hughd@google.com>
>> Cc: Mel Gorman <mgorman@suse.de>
>> Signed-off-by: Minchan Kim <minchan@kernel.org>
> 
> Acked-by: Hugh Dickins <hughd@google.com>
> 
> This is sooooooo much nicer than the code it replaces!  Really good.
> Kudos also to Hannes for suggesting this approach originally, I think.
> 
> I hope this implementation satisfies a good proportion of the people
> who have been wanting MADV_FREE: I'm not among them, and have long
> lost touch with those discussions, so won't judge how usable it is.
> 
> I assume you'll refactor the series again before it goes to Linus,
> so the previous messier implementations vanish?  I notice Andrew
> has this "mm: simplify reclaim path for MADV_FREE" in mmotm as
> mm-dont-split-thp-page-when-syscall-is-called-fix-6.patch:
> I guess it all got much too messy to divide up in a hurry.
> 
> I've noticed no problems in testing (unlike the first time you moved
> to working with pte_dirty); though of course I've not been using
> MADV_FREE itself at all.
> 
> One aspect has worried me for a while, but I think I've reached the
> conclusion that it doesn't matter at all.  The swap that's allocated
> in add_to_swap() would normally get freed again (after try_to_unmap
> found it was a MADV_FREE !pte_dirty !PageDirty case) at the bottom
> of shrink_page_list(), in __remove_mapping(), yes?
> 
> The bit that worried me is that on rare occasions, something unknown
> might take a speculative reference to the page, and __remove_mapping()
> fail to freeze refs for that reason.  Much too rare to worry over not
> freeing that page immediately, but it leaves us with a PageUptodate
> PageSwapCache !PageDirty page, yet its contents are not the contents
> of that location on swap.
> 
> But since this can only happen when you have *not* inserted the
> corresponding swapent anywhere, I cannot think of anything that would
> have a legitimate interest in its contents matching that location on swap.
> So I don't think it's worth looking for somewhere to add a SetPageDirty
> (or a delete_from_swap_cache) just to regularize that case.
> 
>> ---
>> include/linux/rmap.h |  6 +----
>> mm/huge_memory.c     |  5 ----
>> mm/rmap.c            | 42 ++++++----------------------------
>> mm/swap_state.c      |  5 ++--
>> mm/vmscan.c          | 64 ++++++++++++++++------------------------------------
>> 5 files changed, 30 insertions(+), 92 deletions(-)
>> 
>> diff --git a/include/linux/rmap.h b/include/linux/rmap.h
>> index 6b6233fafb53..978f65066fd5 100644
>> --- a/include/linux/rmap.h
>> +++ b/include/linux/rmap.h
>> @@ -193,8 +193,7 @@ static inline void page_dup_rmap(struct page *page, bool compound)
>>  * Called from mm/vmscan.c to handle paging out
>>  */
>> int page_referenced(struct page *, int is_locked,
>> -			struct mem_cgroup *memcg, unsigned long *vm_flags,
>> -			int *is_pte_dirty);
>> +			struct mem_cgroup *memcg, unsigned long *vm_flags);
>> 
>> #define TTU_ACTION(x) ((x) & TTU_ACTION_MASK)
>> 
>> @@ -272,11 +271,8 @@ int rmap_walk(struct page *page, struct rmap_walk_control *rwc);
>> static inline int page_referenced(struct page *page, int is_locked,
>> 				  struct mem_cgroup *memcg,
>> 				  unsigned long *vm_flags,
>> -				  int *is_pte_dirty)
>> {
>> 	*vm_flags = 0;
>> -	if (is_pte_dirty)
>> -		*is_pte_dirty = 0;
>> 	return 0;
>> }
>> 
>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>> index 269ed99493f0..adccfb48ce57 100644
>> --- a/mm/huge_memory.c
>> +++ b/mm/huge_memory.c
>> @@ -1753,11 +1753,6 @@ pmd_t *page_check_address_pmd(struct page *page,
>> 	return NULL;
>> }
>> 
>> -int pmd_freeable(pmd_t pmd)
>> -{
>> -	return !pmd_dirty(pmd);
>> -}
>> -
>> #define VM_NO_THP (VM_SPECIAL | VM_HUGETLB | VM_SHARED | VM_MAYSHARE)
>> 
>> int hugepage_madvise(struct vm_area_struct *vma,
>> diff --git a/mm/rmap.c b/mm/rmap.c
>> index 94ee372e238b..fd64f79c87c4 100644
>> --- a/mm/rmap.c
>> +++ b/mm/rmap.c
>> @@ -797,7 +797,6 @@ int page_mapped_in_vma(struct page *page, struct vm_area_struct *vma)
>> }
>> 
>> struct page_referenced_arg {
>> -	int dirtied;
>> 	int mapcount;
>> 	int referenced;
>> 	unsigned long vm_flags;
>> @@ -812,7 +811,6 @@ static int page_referenced_one(struct page *page, struct vm_area_struct *vma,
>> 	struct mm_struct *mm = vma->vm_mm;
>> 	spinlock_t *ptl;
>> 	int referenced = 0;
>> -	int dirty = 0;
>> 	struct page_referenced_arg *pra = arg;
>> 
>> 	if (unlikely(PageTransHuge(page))) {
>> @@ -835,14 +833,6 @@ static int page_referenced_one(struct page *page, struct vm_area_struct *vma,
>> 		if (pmdp_clear_flush_young_notify(vma, address, pmd))
>> 			referenced++;
>> 
>> -		/*
>> -		 * Use pmd_freeable instead of raw pmd_dirty because in some
>> -		 * of architecture, pmd_dirty is not defined unless
>> -		 * CONFIG_TRANSPARENT_HUGEPAGE is enabled
>> -		 */
>> -		if (!pmd_freeable(*pmd))
>> -			dirty++;
>> -
>> 		spin_unlock(ptl);
>> 	} else {
>> 		pte_t *pte;
>> @@ -873,9 +863,6 @@ static int page_referenced_one(struct page *page, struct vm_area_struct *vma,
>> 				referenced++;
>> 		}
>> 
>> -		if (pte_dirty(*pte))
>> -			dirty++;
>> -
>> 		pte_unmap_unlock(pte, ptl);
>> 	}
>> 
>> @@ -889,9 +876,6 @@ static int page_referenced_one(struct page *page, struct vm_area_struct *vma,
>> 		pra->vm_flags |= vma->vm_flags;
>> 	}
>> 
>> -	if (dirty)
>> -		pra->dirtied++;
>> -
>> 	pra->mapcount--;
>> 	if (!pra->mapcount)
>> 		return SWAP_SUCCESS; /* To break the loop */
>> @@ -916,7 +900,6 @@ static bool invalid_page_referenced_vma(struct vm_area_struct *vma, void *arg)
>>  * @is_locked: caller holds lock on the page
>>  * @memcg: target memory cgroup
>>  * @vm_flags: collect encountered vma->vm_flags who actually referenced the page
>> - * @is_pte_dirty: ptes which have marked dirty bit - used for lazyfree page
>>  *
>>  * Quick test_and_clear_referenced for all mappings to a page,
>>  * returns the number of ptes which referenced the page.
>> @@ -924,8 +907,7 @@ static bool invalid_page_referenced_vma(struct vm_area_struct *vma, void *arg)
>> int page_referenced(struct page *page,
>> 		    int is_locked,
>> 		    struct mem_cgroup *memcg,
>> -		    unsigned long *vm_flags,
>> -		    int *is_pte_dirty)
>> +		    unsigned long *vm_flags)
>> {
>> 	int ret;
>> 	int we_locked = 0;
>> @@ -940,8 +922,6 @@ int page_referenced(struct page *page,
>> 	};
>> 
>> 	*vm_flags = 0;
>> -	if (is_pte_dirty)
>> -		*is_pte_dirty = 0;
>> 
>> 	if (!page_mapped(page))
>> 		return 0;
>> @@ -970,9 +950,6 @@ int page_referenced(struct page *page,
>> 	if (we_locked)
>> 		unlock_page(page);
>> 
>> -	if (is_pte_dirty)
>> -		*is_pte_dirty = pra.dirtied;
>> -
>> 	return pra.referenced;
>> }
>> 
>> @@ -1453,17 +1430,10 @@ static int try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
>> 		swp_entry_t entry = { .val = page_private(page) };
>> 		pte_t swp_pte;
>> 
>> -		if (flags & TTU_FREE) {
>> -			VM_BUG_ON_PAGE(PageSwapCache(page), page);
>> -			if (!PageDirty(page)) {
>> -				/* It's a freeable page by MADV_FREE */
>> -				dec_mm_counter(mm, MM_ANONPAGES);
>> -				goto discard;
>> -			} else {
>> -				set_pte_at(mm, address, pte, pteval);
>> -				ret = SWAP_FAIL;
>> -				goto out_unmap;
>> -			}
>> +		if (!PageDirty(page) && (flags & TTU_FREE)) {
>> +			/* It's a freeable page by MADV_FREE */
>> +			dec_mm_counter(mm, MM_ANONPAGES);
>> +			goto discard;
>> 		}
>> 
>> 		if (PageSwapCache(page)) {
>> @@ -1476,6 +1446,8 @@ static int try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
>> 				ret = SWAP_FAIL;
>> 				goto out_unmap;
>> 			}
>> +			if (!PageDirty(page))
>> +				SetPageDirty(page);
>> 			if (list_empty(&mm->mmlist)) {
>> 				spin_lock(&mmlist_lock);
>> 				if (list_empty(&mm->mmlist))
>> diff --git a/mm/swap_state.c b/mm/swap_state.c
>> index d783872d746c..676ff2991380 100644
>> --- a/mm/swap_state.c
>> +++ b/mm/swap_state.c
>> @@ -185,13 +185,12 @@ int add_to_swap(struct page *page, struct list_head *list)
>> 	 * deadlock in the swap out path.
>> 	 */
>> 	/*
>> -	 * Add it to the swap cache and mark it dirty
>> +	 * Add it to the swap cache.
>> 	 */
>> 	err = add_to_swap_cache(page, entry,
>> 			__GFP_HIGH|__GFP_NOMEMALLOC|__GFP_NOWARN);
>> 
>> -	if (!err) {	/* Success */
>> -		SetPageDirty(page);
>> +	if (!err) {
>> 		return 1;
>> 	} else {	/* -ENOMEM radix-tree allocation failure */
>> 		/*
>> diff --git a/mm/vmscan.c b/mm/vmscan.c
>> index 27d580b5e853..9b52ecf91194 100644
>> --- a/mm/vmscan.c
>> +++ b/mm/vmscan.c
>> @@ -791,17 +791,15 @@ enum page_references {
>> };
>> 
>> static enum page_references page_check_references(struct page *page,
>> -						  struct scan_control *sc,
>> -						  bool *freeable)
>> +						  struct scan_control *sc)
>> {
>> 	int referenced_ptes, referenced_page;
>> 	unsigned long vm_flags;
>> -	int pte_dirty;
>> 
>> 	VM_BUG_ON_PAGE(!PageLocked(page), page);
>> 
>> 	referenced_ptes = page_referenced(page, 1, sc->target_mem_cgroup,
>> -					  &vm_flags, &pte_dirty);
>> +					  &vm_flags);
>> 	referenced_page = TestClearPageReferenced(page);
>> 
>> 	/*
>> @@ -842,10 +840,6 @@ static enum page_references page_check_references(struct page *page,
>> 		return PAGEREF_KEEP;
>> 	}
>> 
>> -	if (PageAnon(page) && !pte_dirty && !PageSwapCache(page) &&
>> -			!PageDirty(page))
>> -		*freeable = true;
>> -
>> 	/* Reclaim if clean, defer dirty pages to writeback */
>> 	if (referenced_page && !PageSwapBacked(page))
>> 		return PAGEREF_RECLAIM_CLEAN;
>> @@ -1037,8 +1031,7 @@ static unsigned long shrink_page_list(struct list_head *page_list,
>> 		}
>> 
>> 		if (!force_reclaim)
>> -			references = page_check_references(page, sc,
>> -							&freeable);
>> +			references = page_check_references(page, sc);
>> 
>> 		switch (references) {
>> 		case PAGEREF_ACTIVATE:
>> @@ -1055,31 +1048,24 @@ static unsigned long shrink_page_list(struct list_head *page_list,
>> 		 * Try to allocate it some swap space here.
>> 		 */
>> 		if (PageAnon(page) && !PageSwapCache(page)) {
>> -			if (!freeable) {
>> -				if (!(sc->gfp_mask & __GFP_IO))
>> -					goto keep_locked;
>> -				if (!add_to_swap(page, page_list))
>> -					goto activate_locked;
>> -				may_enter_fs = 1;
>> -				/* Adding to swap updated mapping */
>> -				mapping = page_mapping(page);
>> -			} else {
>> -				if (likely(!PageTransHuge(page)))
>> -					goto unmap;
>> -				/* try_to_unmap isn't aware of THP page */
>> -				if (unlikely(split_huge_page_to_list(page,
>> -								page_list)))
>> -					goto keep_locked;
>> -			}
>> +			if (!(sc->gfp_mask & __GFP_IO))
>> +				goto keep_locked;
>> +			if (!add_to_swap(page, page_list))
>> +				goto activate_locked;
>> +			freeable = true;
>> +			may_enter_fs = 1;
>> +			/* Adding to swap updated mapping */
>> +			mapping = page_mapping(page);
>> 		}
>> -unmap:
>> +
>> 		/*
>> 		 * The page is mapped into the page tables of one or more
>> 		 * processes. Try to unmap it here.
>> 		 */
>> -		if (page_mapped(page) && (mapping || freeable)) {
>> +		if (page_mapped(page) && mapping) {
>> 			switch (try_to_unmap(page, freeable ?
>> -					TTU_FREE : ttu_flags|TTU_BATCH_FLUSH)) {
>> +					ttu_flags | TTU_BATCH_FLUSH | TTU_FREE :
>> +					ttu_flags | TTU_BATCH_FLUSH)) {
>> 			case SWAP_FAIL:
>> 				goto activate_locked;
>> 			case SWAP_AGAIN:
>> @@ -1087,20 +1073,7 @@ static unsigned long shrink_page_list(struct list_head *page_list,
>> 			case SWAP_MLOCK:
>> 				goto cull_mlocked;
>> 			case SWAP_SUCCESS:
>> -				/* try to free the page below */
>> -				if (!freeable)
>> -					break;
>> -				/*
>> -				 * Freeable anon page doesn't have mapping
>> -				 * due to skipping of swapcache so we free
>> -				 * page in here rather than __remove_mapping.
>> -				 */
>> -				VM_BUG_ON_PAGE(PageSwapCache(page), page);
>> -				if (!page_freeze_refs(page, 1))
>> -					goto keep_locked;
>> -				__ClearPageLocked(page);
>> -				count_vm_event(PGLAZYFREED);
>> -				goto free_it;
>> +				; /* try to free the page below */
>> 			}
>> 		}
>> 
>> @@ -1217,6 +1190,9 @@ static unsigned long shrink_page_list(struct list_head *page_list,
>> 		 */
>> 		__ClearPageLocked(page);
>> free_it:
>> +		if (freeable && !PageDirty(page))
>> +			count_vm_event(PGLAZYFREED);
>> +
>> 		nr_reclaimed++;
>> 
>> 		/*
>> @@ -1847,7 +1823,7 @@ static void shrink_active_list(unsigned long nr_to_scan,
>> 		}
>> 
>> 		if (page_referenced(page, 0, sc->target_mem_cgroup,
>> -				    &vm_flags, NULL)) {
>> +				    &vm_flags)) {
>> 			nr_rotated += hpage_nr_pages(page);
>> 			/*
>> 			 * Identify referenced, file-backed active pages and
>> -- 
>> 1.9.1
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
it is wrong here if you only check PageDirty() to decide if the page is freezable or not .
The Anon page are shared by multiple process, _mapcount > 1 ,
so you must check all pt_dirty bit during page_referenced() function,
see this mail thread:
http://ns1.ske-art.com/lists/kernel/msg1934021.html
Thanks









--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1256514 — Re: [PATCH 4/5] mm: simplify reclaim path for MADV_FREE

Hello Yalin,

Sorry for missing you in Cc list.
IIRC, mails to send your previous mail address(Yalin.Wang@sonymobile.com)
were returned.

On Tue, Oct 27, 2015 at 11:44:09AM +0800, yalin wang wrote:
> 
> > On Oct 27, 2015, at 10:09, Hugh Dickins <hughd@google.com> wrote:
> > 
> > On Mon, 19 Oct 2015, Minchan Kim wrote:
> > 
> >> I made reclaim path mess to check and free MADV_FREEed page.
> >> This patch simplify it with tweaking add_to_swap.
> >> 
> >> So far, we mark page as PG_dirty when we add the page into
> >> swap cache(ie, add_to_swap) to page out to swap device but
> >> this patch moves PG_dirty marking under try_to_unmap_one
> >> when we decide to change pte from anon to swapent so if
> >> any process's pte has swapent for the page, the page must
> >> be swapped out. IOW, there should be no funcional behavior
> >> change. It makes relcaim path really simple for MADV_FREE
> >> because we just need to check PG_dirty of page to decide
> >> discarding the page or not.
> >> 
> >> Other thing this patch does is to pass TTU_BATCH_FLUSH to
> >> try_to_unmap when we handle freeable page because I don't
> >> see any reason to prevent it.
> >> 
> >> Cc: Hugh Dickins <hughd@google.com>
> >> Cc: Mel Gorman <mgorman@suse.de>
> >> Signed-off-by: Minchan Kim <minchan@kernel.org>
> > 
> > Acked-by: Hugh Dickins <hughd@google.com>
> > 
> > This is sooooooo much nicer than the code it replaces!  Really good.
> > Kudos also to Hannes for suggesting this approach originally, I think.
> > 
> > I hope this implementation satisfies a good proportion of the people
> > who have been wanting MADV_FREE: I'm not among them, and have long
> > lost touch with those discussions, so won't judge how usable it is.
> > 
> > I assume you'll refactor the series again before it goes to Linus,
> > so the previous messier implementations vanish?  I notice Andrew
> > has this "mm: simplify reclaim path for MADV_FREE" in mmotm as
> > mm-dont-split-thp-page-when-syscall-is-called-fix-6.patch:
> > I guess it all got much too messy to divide up in a hurry.
> > 
> > I've noticed no problems in testing (unlike the first time you moved
> > to working with pte_dirty); though of course I've not been using
> > MADV_FREE itself at all.
> > 
> > One aspect has worried me for a while, but I think I've reached the
> > conclusion that it doesn't matter at all.  The swap that's allocated
> > in add_to_swap() would normally get freed again (after try_to_unmap
> > found it was a MADV_FREE !pte_dirty !PageDirty case) at the bottom
> > of shrink_page_list(), in __remove_mapping(), yes?
> > 
> > The bit that worried me is that on rare occasions, something unknown
> > might take a speculative reference to the page, and __remove_mapping()
> > fail to freeze refs for that reason.  Much too rare to worry over not
> > freeing that page immediately, but it leaves us with a PageUptodate
> > PageSwapCache !PageDirty page, yet its contents are not the contents
> > of that location on swap.
> > 
> > But since this can only happen when you have *not* inserted the
> > corresponding swapent anywhere, I cannot think of anything that would
> > have a legitimate interest in its contents matching that location on swap.
> > So I don't think it's worth looking for somewhere to add a SetPageDirty
> > (or a delete_from_swap_cache) just to regularize that case.
> > 
> >> ---
> >> include/linux/rmap.h |  6 +----
> >> mm/huge_memory.c     |  5 ----
> >> mm/rmap.c            | 42 ++++++----------------------------
> >> mm/swap_state.c      |  5 ++--
> >> mm/vmscan.c          | 64 ++++++++++++++++------------------------------------
> >> 5 files changed, 30 insertions(+), 92 deletions(-)
> >> 

<snip>

You added comment bottom line so I'm not sure what PageDirty you meant.

> it is wrong here if you only check PageDirty() to decide if the page is freezable or not .
> The Anon page are shared by multiple process, _mapcount > 1 ,
> so you must check all pt_dirty bit during page_referenced() function,
> see this mail thread:
> http://ns1.ske-art.com/lists/kernel/msg1934021.html

If one of pte among process sharing the page was dirty, the dirtiness should
be propagated from pte to PG_dirty by try_to_unmap_one.
IOW, if the page doesn't have PG_dirty flag, it means all of process did
MADV_FREE.

Am I missing something from you question?
If so, could you show exact scenario I am missing?

Thanks for the interest.


> Thanks
> 
> 
> 
> 
> 
> 
> 
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1256529 — Re: [PATCH 4/5] mm: simplify reclaim path for MADV_FREE

> On Oct 27, 2015, at 15:09, Minchan Kim <minchan@kernel.org> wrote:
> 
> Hello Yalin,
> 
> Sorry for missing you in Cc list.
> IIRC, mails to send your previous mail address(Yalin.Wang@sonymobile.com)
> were returned.
> 
> You added comment bottom line so I'm not sure what PageDirty you meant.
> 
>> it is wrong here if you only check PageDirty() to decide if the page is freezable or not .
>> The Anon page are shared by multiple process, _mapcount > 1 ,
>> so you must check all pt_dirty bit during page_referenced() function,
>> see this mail thread:
>> http://ns1.ske-art.com/lists/kernel/msg1934021.html
> 
> If one of pte among process sharing the page was dirty, the dirtiness should
> be propagated from pte to PG_dirty by try_to_unmap_one.
> IOW, if the page doesn't have PG_dirty flag, it means all of process did
> MADV_FREE.
> 
> Am I missing something from you question?
> If so, could you show exact scenario I am missing?
> 
> Thanks for the interest.
oh, yeah , that is right , i miss that , pte_dirty will propagate to PG_dirty ,
so that is correct .
Generic to say this patch move set_page_dirty() from add_to_swap() to 
try_to_unmap(), i think can change a little about this patch:

@@ -1476,6 +1446,8 @@ static int try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
				ret = SWAP_FAIL;
				goto out_unmap;
			}
+			if (!PageDirty(page))
+				SetPageDirty(page);
			if (list_empty(&mm->mmlist)) {
				spin_lock(&mmlist_lock);
				if (list_empty(&mm->mmlist))

i think this 2 lines can be removed ,
since  pte_dirty have propagated to set_page_dirty() , we don’t need this line here ,
otherwise you will always dirty a AnonPage, even it is clean,
then we will page out this clean page to swap partition one more , this is not needed.
am i understanding correctly ?

By the way, please change my mail address to yalin.wang2010@gmail.com in CC list .
Thanks a lot. :) 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1256548 — Re: [PATCH 4/5] mm: simplify reclaim path for MADV_FREE

On Tue, Oct 27, 2015 at 03:39:16PM +0800, yalin wang wrote:
> 
> > On Oct 27, 2015, at 15:09, Minchan Kim <minchan@kernel.org> wrote:
> > 
> > Hello Yalin,
> > 
> > Sorry for missing you in Cc list.
> > IIRC, mails to send your previous mail address(Yalin.Wang@sonymobile.com)
> > were returned.
> > 
> > You added comment bottom line so I'm not sure what PageDirty you meant.
> > 
> >> it is wrong here if you only check PageDirty() to decide if the page is freezable or not .
> >> The Anon page are shared by multiple process, _mapcount > 1 ,
> >> so you must check all pt_dirty bit during page_referenced() function,
> >> see this mail thread:
> >> http://ns1.ske-art.com/lists/kernel/msg1934021.html
> > 
> > If one of pte among process sharing the page was dirty, the dirtiness should
> > be propagated from pte to PG_dirty by try_to_unmap_one.
> > IOW, if the page doesn't have PG_dirty flag, it means all of process did
> > MADV_FREE.
> > 
> > Am I missing something from you question?
> > If so, could you show exact scenario I am missing?
> > 
> > Thanks for the interest.
> oh, yeah , that is right , i miss that , pte_dirty will propagate to PG_dirty ,
> so that is correct .
> Generic to say this patch move set_page_dirty() from add_to_swap() to 
> try_to_unmap(), i think can change a little about this patch:
> 
> @@ -1476,6 +1446,8 @@ static int try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
> 				ret = SWAP_FAIL;
> 				goto out_unmap;
> 			}
> +			if (!PageDirty(page))
> +				SetPageDirty(page);
> 			if (list_empty(&mm->mmlist)) {
> 				spin_lock(&mmlist_lock);
> 				if (list_empty(&mm->mmlist))
> 
> i think this 2 lines can be removed ,
> since  pte_dirty have propagated to set_page_dirty() , we don’t need this line here ,
> otherwise you will always dirty a AnonPage, even it is clean,
> then we will page out this clean page to swap partition one more , this is not needed.
> am i understanding correctly ?

Your understanding is correct.
I will fix it in next spin.

> 
> By the way, please change my mail address to yalin.wang2010@gmail.com in CC list .
> Thanks a lot. :) 

Thanks for the review!

> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1256587 — Re: [PATCH 4/5] mm: simplify reclaim path for MADV_FREE

> On Oct 27, 2015, at 16:10, Minchan Kim <minchan@kernel.org> wrote:
> 
> On Tue, Oct 27, 2015 at 03:39:16PM +0800, yalin wang wrote:
>> 
>>> On Oct 27, 2015, at 15:09, Minchan Kim <minchan@kernel.org> wrote:
>>> 
>>> Hello Yalin,
>>> 
>>> Sorry for missing you in Cc list.
>>> IIRC, mails to send your previous mail address(Yalin.Wang@sonymobile.com)
>>> were returned.
>>> 
>>> You added comment bottom line so I'm not sure what PageDirty you meant.
>>> 
>>>> it is wrong here if you only check PageDirty() to decide if the page is freezable or not .
>>>> The Anon page are shared by multiple process, _mapcount > 1 ,
>>>> so you must check all pt_dirty bit during page_referenced() function,
>>>> see this mail thread:
>>>> http://ns1.ske-art.com/lists/kernel/msg1934021.html
>>> 
>>> If one of pte among process sharing the page was dirty, the dirtiness should
>>> be propagated from pte to PG_dirty by try_to_unmap_one.
>>> IOW, if the page doesn't have PG_dirty flag, it means all of process did
>>> MADV_FREE.
>>> 
>>> Am I missing something from you question?
>>> If so, could you show exact scenario I am missing?
>>> 
>>> Thanks for the interest.
>> oh, yeah , that is right , i miss that , pte_dirty will propagate to PG_dirty ,
>> so that is correct .
>> Generic to say this patch move set_page_dirty() from add_to_swap() to 
>> try_to_unmap(), i think can change a little about this patch:
>> 
>> @@ -1476,6 +1446,8 @@ static int try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
>> 				ret = SWAP_FAIL;
>> 				goto out_unmap;
>> 			}
>> +			if (!PageDirty(page))
>> +				SetPageDirty(page);
>> 			if (list_empty(&mm->mmlist)) {
>> 				spin_lock(&mmlist_lock);
>> 				if (list_empty(&mm->mmlist))
>> 
>> i think this 2 lines can be removed ,
>> since  pte_dirty have propagated to set_page_dirty() , we don’t need this line here ,
>> otherwise you will always dirty a AnonPage, even it is clean,
>> then we will page out this clean page to swap partition one more , this is not needed.
>> am i understanding correctly ?
> 
> Your understanding is correct.
> I will fix it in next spin.
> 
>> 
>> By the way, please change my mail address to yalin.wang2010@gmail.com in CC list .
>> Thanks a lot. :) 
> 
> Thanks for the review!

i have a look at the old mail list , i recall the scenario that multiple processes share a AnonPage 
special case :

for example Process A have a AnonPage map like this:
	! pte_dirty() && PageDirty()==1   (this is possible after read fault happened on swap entry, and try_to_free_swap() succeed.)
Process A  do a fork() , New process is called B .
Then A  syscall(MADV_FREE) on the page .
At this time, page table like this:

A  ! pte_dirty() && PageDirty() == 0  && PageSwapCache() == 0

B ! pte_dirty() && PageDirty() == 0  && PageSwapCache() == 0

This means this page is freeable , and can be freed during page reclaim.
This is not fair for Process B . Since B don’t call syscall(MADV_FREE) ,
its page should not be discard .  Will cause some strange behaviour if happened .

This is discussed by 
http://www.serverphorums.com/read.php?12,1220840
but i don’t know why the patch is not merged .

Thanks 












--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1257647 — Re: [PATCH 4/5] mm: simplify reclaim path for MADV_FREE

> On Oct 27, 2015, at 16:52, yalin wang <yalin.wang2010@gmail.com> wrote:
> 
> 
>> On Oct 27, 2015, at 16:10, Minchan Kim <minchan@kernel.org> wrote:
>> 
>> On Tue, Oct 27, 2015 at 03:39:16PM +0800, yalin wang wrote:
>>> 
>>>> On Oct 27, 2015, at 15:09, Minchan Kim <minchan@kernel.org> wrote:
>>>> 
>>>> Hello Yalin,
>>>> 
>>>> Sorry for missing you in Cc list.
>>>> IIRC, mails to send your previous mail address(Yalin.Wang@sonymobile.com)
>>>> were returned.
>>>> 
>>>> You added comment bottom line so I'm not sure what PageDirty you meant.
>>>> 
>>>>> it is wrong here if you only check PageDirty() to decide if the page is freezable or not .
>>>>> The Anon page are shared by multiple process, _mapcount > 1 ,
>>>>> so you must check all pt_dirty bit during page_referenced() function,
>>>>> see this mail thread:
>>>>> http://ns1.ske-art.com/lists/kernel/msg1934021.html
>>>> 
>>>> If one of pte among process sharing the page was dirty, the dirtiness should
>>>> be propagated from pte to PG_dirty by try_to_unmap_one.
>>>> IOW, if the page doesn't have PG_dirty flag, it means all of process did
>>>> MADV_FREE.
>>>> 
>>>> Am I missing something from you question?
>>>> If so, could you show exact scenario I am missing?
>>>> 
>>>> Thanks for the interest.
>>> oh, yeah , that is right , i miss that , pte_dirty will propagate to PG_dirty ,
>>> so that is correct .
>>> Generic to say this patch move set_page_dirty() from add_to_swap() to 
>>> try_to_unmap(), i think can change a little about this patch:
>>> 
>>> @@ -1476,6 +1446,8 @@ static int try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
>>> 				ret = SWAP_FAIL;
>>> 				goto out_unmap;
>>> 			}
>>> +			if (!PageDirty(page))
>>> +				SetPageDirty(page);
>>> 			if (list_empty(&mm->mmlist)) {
>>> 				spin_lock(&mmlist_lock);
>>> 				if (list_empty(&mm->mmlist))
>>> 
>>> i think this 2 lines can be removed ,
>>> since  pte_dirty have propagated to set_page_dirty() , we don’t need this line here ,
>>> otherwise you will always dirty a AnonPage, even it is clean,
>>> then we will page out this clean page to swap partition one more , this is not needed.
>>> am i understanding correctly ?
>> 
>> Your understanding is correct.
>> I will fix it in next spin.
>> 
>>> 
>>> By the way, please change my mail address to yalin.wang2010@gmail.com in CC list .
>>> Thanks a lot. :) 
>> 
>> Thanks for the review!
> 
> i have a look at the old mail list , i recall the scenario that multiple processes share a AnonPage 
> special case :
> 
> for example Process A have a AnonPage map like this:
> 	! pte_dirty() && PageDirty()==1   (this is possible after read fault happened on swap entry, and try_to_free_swap() succeed.)
> Process A  do a fork() , New process is called B .
> Then A  syscall(MADV_FREE) on the page .
> At this time, page table like this:
> 
> A  ! pte_dirty() && PageDirty() == 0  && PageSwapCache() == 0
> 
> B ! pte_dirty() && PageDirty() == 0  && PageSwapCache() == 0
> 
> This means this page is freeable , and can be freed during page reclaim.
> This is not fair for Process B . Since B don’t call syscall(MADV_FREE) ,
> its page should not be discard .  Will cause some strange behaviour if happened .
> 
> This is discussed by 
> http://www.serverphorums.com/read.php?12,1220840
> but i don’t know why the patch is not merged .
> 
> Thanks 
oh, i have see 0b502297d1cc26e09b98955b4efa728be1c48921
this commit merged , then this problem should be fixed by this method.
ignore this mail. :)

Thanks a lot .





--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1256507 — Re: [PATCH 4/5] mm: simplify reclaim path for MADV_FREE

On Mon, Oct 26, 2015 at 07:09:15PM -0700, Hugh Dickins wrote:
> On Mon, 19 Oct 2015, Minchan Kim wrote:
> 
> > I made reclaim path mess to check and free MADV_FREEed page.
> > This patch simplify it with tweaking add_to_swap.
> > 
> > So far, we mark page as PG_dirty when we add the page into
> > swap cache(ie, add_to_swap) to page out to swap device but
> > this patch moves PG_dirty marking under try_to_unmap_one
> > when we decide to change pte from anon to swapent so if
> > any process's pte has swapent for the page, the page must
> > be swapped out. IOW, there should be no funcional behavior
> > change. It makes relcaim path really simple for MADV_FREE
> > because we just need to check PG_dirty of page to decide
> > discarding the page or not.
> > 
> > Other thing this patch does is to pass TTU_BATCH_FLUSH to
> > try_to_unmap when we handle freeable page because I don't
> > see any reason to prevent it.
> > 
> > Cc: Hugh Dickins <hughd@google.com>
> > Cc: Mel Gorman <mgorman@suse.de>
> > Signed-off-by: Minchan Kim <minchan@kernel.org>
> 
> Acked-by: Hugh Dickins <hughd@google.com>
> 
> This is sooooooo much nicer than the code it replaces!  Really good.

Thanks!

> Kudos also to Hannes for suggesting this approach originally, I think.

I should buy beer or soju if Hannes likes.

> 
> I hope this implementation satisfies a good proportion of the people
> who have been wanting MADV_FREE: I'm not among them, and have long
> lost touch with those discussions, so won't judge how usable it is.
> 
> I assume you'll refactor the series again before it goes to Linus,
> so the previous messier implementations vanish?  I notice Andrew

Actutally, I didn't think about that but once you mentioned it,
I realized that would be better. Thanks for the suggestion.

> has this "mm: simplify reclaim path for MADV_FREE" in mmotm as
> mm-dont-split-thp-page-when-syscall-is-called-fix-6.patch:
> I guess it all got much too messy to divide up in a hurry.

Yeb, I will rebase all series from the beginning based on recent mmtom
so I will vanish the mess in git-blame.

When I rebases it in mmotm, I will do it before reaching THP refcount
new design if Andrew and Kirill don't mind it because it makes to fail
my test as I reported. I don't know it's long time unknown bug or
something THP-ref new introduces. Anyway, I want to test smoothly.

> 
> I've noticed no problems in testing (unlike the first time you moved
> to working with pte_dirty); though of course I've not been using

Thanks for testing!

> MADV_FREE itself at all.
> 
> One aspect has worried me for a while, but I think I've reached the
> conclusion that it doesn't matter at all.  The swap that's allocated
> in add_to_swap() would normally get freed again (after try_to_unmap
> found it was a MADV_FREE !pte_dirty !PageDirty case) at the bottom
> of shrink_page_list(), in __remove_mapping(), yes?

Right.

> 
> The bit that worried me is that on rare occasions, something unknown
> might take a speculative reference to the page, and __remove_mapping()
> fail to freeze refs for that reason.  Much too rare to worry over not
> freeing that page immediately, but it leaves us with a PageUptodate
> PageSwapCache !PageDirty page, yet its contents are not the contents
> of that location on swap.
> 
> But since this can only happen when you have *not* inserted the
> corresponding swapent anywhere, I cannot think of anything that would
> have a legitimate interest in its contents matching that location on swap.
> So I don't think it's worth looking for somewhere to add a SetPageDirty
> (or a delete_from_swap_cache) just to regularize that case.


Exactly.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1250533

On Mon, Oct 19, 2015 at 03:31:42PM +0900, Minchan Kim wrote:
> Hello, it's too late since I sent previos patch.
> https://lkml.org/lkml/2015/6/3/37
> 
> This patch is alomost new compared to previos approach.
> I think this is more simple, clear and easy to review.
> 
> One thing I should notice is that I have tested this patch
> and couldn't find any critical problem so I rebased patchset
> onto recent mmotm(ie, mmotm-2015-10-15-15-20) to send formal
> patchset. Unfortunately, I start to see sudden discarding of
> the page we shouldn't do. IOW, application's valid anonymous page
> was disappeared suddenly.
> 
> When I look through THP changes, I think we could lose
> dirty bit of pte between freeze_page and unfreeze_page
> when we mark it as migration entry and restore it.
> So, I added below simple code without enough considering
> and cannot see the problem any more.
> I hope it's good hint to find right fix this problem.
> 
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index d5ea516ffb54..e881c04f5950 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -3138,6 +3138,9 @@ static void unfreeze_page_vma(struct vm_area_struct *vma, struct page *page,
>  		if (is_write_migration_entry(swp_entry))
>  			entry = maybe_mkwrite(entry, vma);
>  
> +		if (PageDirty(page))
> +			SetPageDirty(page);

The condition of PageDirty was typo. I didn't add the condition.
Just added.

                SetPageDirty(page);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1251339

On Tue, Oct 20, 2015 at 04:21:09PM +0900, Minchan Kim wrote:
> On Mon, Oct 19, 2015 at 07:01:50PM +0900, Minchan Kim wrote:
> > On Mon, Oct 19, 2015 at 03:31:42PM +0900, Minchan Kim wrote:
> > > Hello, it's too late since I sent previos patch.
> > > https://lkml.org/lkml/2015/6/3/37
> > > 
> > > This patch is alomost new compared to previos approach.
> > > I think this is more simple, clear and easy to review.
> > > 
> > > One thing I should notice is that I have tested this patch
> > > and couldn't find any critical problem so I rebased patchset
> > > onto recent mmotm(ie, mmotm-2015-10-15-15-20) to send formal
> > > patchset. Unfortunately, I start to see sudden discarding of
> > > the page we shouldn't do. IOW, application's valid anonymous page
> > > was disappeared suddenly.
> > > 
> > > When I look through THP changes, I think we could lose
> > > dirty bit of pte between freeze_page and unfreeze_page
> > > when we mark it as migration entry and restore it.
> > > So, I added below simple code without enough considering
> > > and cannot see the problem any more.
> > > I hope it's good hint to find right fix this problem.
> > > 
> > > diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> > > index d5ea516ffb54..e881c04f5950 100644
> > > --- a/mm/huge_memory.c
> > > +++ b/mm/huge_memory.c
> > > @@ -3138,6 +3138,9 @@ static void unfreeze_page_vma(struct vm_area_struct *vma, struct page *page,
> > >  		if (is_write_migration_entry(swp_entry))
> > >  			entry = maybe_mkwrite(entry, vma);
> > >  
> > > +		if (PageDirty(page))
> > > +			SetPageDirty(page);
> > 
> > The condition of PageDirty was typo. I didn't add the condition.
> > Just added.
> > 
> >                 SetPageDirty(page);
> 
> I reviewed THP refcount redesign patch and It seems below patch fixes
> MADV_FREE problem. It works well for hours.
> 
> From 104a0940b4c0f97e61de9fee0fd602926ff28312 Mon Sep 17 00:00:00 2001
> From: Minchan Kim <minchan@kernel.org>
> Date: Tue, 20 Oct 2015 16:00:52 +0900
> Subject: [PATCH] mm: mark head page dirty in split_huge_page
> 
> In thp split in old THP refcount, we mappped all of pages
> (ie, head + tails) to pte_mkdirty and mark PG_flags to every
> tail pages.
> 
> But with THP refcount redesign, we can lose dirty bit in page table
> and PG_dirty for head page if we want to free the THP page using
 
typo.
                                           freeze

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web