mm: change mm_advise_free to clear page dirty

RFC Minchan Kim 3 participants 7 messages Mar 3, 2015 – Mar 9, 2015 Memory management

Share & tools

Minchan Kim

author

2015-03-03 03:26 UTC #0 lore raw

Could you separte this patch in this patchset thread? It's tackling differnt problem.

As well, I had a question to previous thread about why shared page has a problem now but you didn't answer and send a new patchset. It makes reviewers/maintainer time waste/confuse. Please, don't hurry to send a code. Before that, resolve reviewers's comments.

Quoted 6 lines · Yalin

This patch add ClearPageDirty() to clear AnonPage dirty flag, if not clear page dirty for this anon page, the page will never be treated as freeable. We also make sure the shared AnonPage is not freeable, we implement it by dirty all copyed AnonPage pte, so that make sure the Anonpage will not become freeable, unless all process which shared this page call madvise_free syscall.

Please, spend more time to make description clear. I really doubt who understand this description without code inspection. :( Of course, I'm not a person to write description clear like native , either but just I'm sure I spend a more time to write description rather than coding, at least. :)

Quoted 31 lines

Signed-off-by Yalin Wang <[email protected]>

---
 mm/madvise.c | 16 +++++++++-------
 mm/memory.c  | 12 ++++++++++--
 2 files changed, 19 insertions(+), 9 deletions(-)

mm/madvise.c +1 -7

index 6d0fcb8..b61070d 100644
@@ -297,23 +297,25 @@ static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
 			continue;
 
 		page = vm_normal_page(vma, addr, ptent);
-		if (!page)
+		if (!page || !trylock_page(page))
 			continue;
 
 		if (PageSwapCache(page)) {
-			if (!trylock_page(page))
-				continue;
-
 			if (!try_to_free_swap(page)) {
 				unlock_page(page);
 				continue;
 			}
-
-			ClearPageDirty(page);
-			unlock_page(page);
 		}

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

Quoted 10 lines

mm/madvise.c +8 -0

 
 		/*
+		 * we clear page dirty flag for AnonPage, no matter if this
+		 * page is in swapcahce or not, AnonPage not in swapcache also set
+		 * dirty flag sometimes, this happened when a AnonPage is removed
+		 * from swapcahce by try_to_free_swap()
+		 */
+		ClearPageDirty(page);
+		unlock_page(page);
+		/*

Parent:

ptrP = malloc();
*ptrP = 'a';
fork(); -> child process pte has dirty by your patch
..
memory pressure -> So, swapped out the page.
..
..
Child: var = *ptrP; assert(var =='a') -> So, swapin happens and child has pte_clean
parent: var = *ptrP; aasert(var == 'a') -> So, swapin happens and parent has pte_clean ..
..
Parent:
madvise_free -> remove PageDirty
So, both parent and child has pte_clean and !PageDirty, which
is target for VM to discard a page.
..
VM discard the page by memory pressure.
..
Child: var = *ptrP: assert(var == 'a'); <---- oops.

And blindly ClearPageDirty makes duplicates swap out.

Quoted 10 lines

mm/madvise.c +0 -0

 		 * Some of architecture(ex, PPC) don't update TLB
 		 * with set_pte_at and tlb_remove_tlb_entry so for
 		 * the portability, remap the pte with old|clean

mm/memory.c +0 -0

index 8068893..3d949b3 100644
@@ -874,10 +874,18 @@ copy_one_pte(struct mm_struct *dst_mm, struct mm_struct *src_mm,
 	if (page) {
 		get_page(page);

mm/memory.c copy_one_pte(struct mm_struct *dst_mm, struct mm_struct *src_mm, · line 884

Quoted 10 lines

mm/memory.c +8 -1

 		page_dup_rmap(page);
-		if (PageAnon(page))
+		if (PageAnon(page)) {
+			/*
+			 * we dirty the copyed pte for anon page,
+			 * this is useful for madvise_free_pte_range(),
+			 * this can prevent shared anon page freed by madvise_free
+			 * syscall
+			 */
+			pte = pte_mkdirty(pte);

It made every MADV_FREE hinted page void. IOW, if a process called MADV_FREE calls fork, VM cannot discard pages if child doesn't free pages or calls madvise_free. Then, if parent calls madvise_free before fork, we couldn't free those pages. IOW, you are ignoring below example.

parent:

ptr1 = malloc(len);
        -> allocator calls mmap(len);
memset(ptr1, 'a', len);
free(ptr1);
        -> allocator calls madvise_free(ptr1, len);
fork();
..
..
        -> VM discard hinted pages

child:

ptr2 = malloc(len)

        -> allocator reuses the chunk allocated from parent.

so, child will see zero pages from ptr2 but he doesn't write anything so garbage|zero page anything is okay to him.

As well, you are adding new instructions in fork which is very frequent syscall so I'd like to find another way to avoid adding instructions in such hot path.

I will send different patch. Please review it.

So, my suggestion is below. It always makes pte dirty so let's Cc Cyrill to take care of softdirty and Hugh who is Mr.Swap.

Wang, Yalin

2015-03-03 03:59 UTC #1 lore raw

Quoted 25 lines

-----Original Message-----
From: Minchan Kim [mailto:[email protected]] On Behalf Of Minchan Kim
Sent: Tuesday, March 03, 2015 11:26 AM
To: Wang, Yalin
Cc: 'Michal Hocko'; 'Andrew Morton'; '[email protected]'; '[email protected]'; 'Rik van Riel'; 'Johannes Weiner'; 'Mel Gorman'; 'Shaohua Li'; Hugh Dickins; Cyrill Gorcunov
Subject: Re: [RFC V3] mm: change mm_advise_free to clear page dirty
Could you separte this patch in this patchset thread? It's tackling differnt problem.
As well, I had a question to previous thread about why shared page has a problem now but you didn't answer and send a new patchset. It makes reviewers/maintainer time waste/confuse. Please, don't hurry to send a code. Before that, resolve reviewers's comments.
Quoted 6 lines · Yalin
This patch add ClearPageDirty() to clear AnonPage dirty flag, if not clear page dirty for this anon page, the page will never be treated as freeable. We also make sure the shared AnonPage is not freeable, we implement it by dirty all copyed AnonPage pte, so that make sure the Anonpage will not become freeable, unless all process which shared this page call madvise_free syscall.

Quoted 6 lines

Please, spend more time to make description clear. I really doubt who understand this description without code inspection. :( Of course, I'm not a person to write description clear like native , either but just I'm sure I spend a more time to write description rather than coding, at least. :)

I see, I will send another mail for file private map pages. Sorry for my English expressions.
I think your solution is ok,
Your patch will make sure the anonpage pte will always be dirty. I add some comments for your patch:

Quoted 26 lines

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

mm/madvise.c +0 -1

index 6d0fcb8..d64200e 100644
@@ -309,7 +309,6 @@ static int madvise_free_pte_range(pmd_t *pmd, unsigned
 long addr,
 				continue;
 			}
 
-			ClearPageDirty(page);
 			unlock_page(page);
 		}

mm/memory.c +0 -0

index 8ae52c9..2f45e77 100644
@@ -2460,9 +2460,14 @@ static int do_swap_page(struct mm_struct *mm, struct
 vm_area_struct *vma,

mm/memory.c static int do_swap_page(struct mm_struct *mm, struct · line 2469

Quoted 10 lines

mm/memory.c +6 -1

 
 	inc_mm_counter_fast(mm, MM_ANONPAGES);
 	dec_mm_counter_fast(mm, MM_SWAPENTS);
-	pte = mk_pte(page, vma->vm_page_prot);
+
+	/*
+	 * Every page swapped-out was pte_dirty so we makes pte dirty again.
+	 * MADV_FREE relys on it.
+	 */
+	pte = mk_pte(pte_mkdirty(page), vma->vm_page_prot);

pte_mkdirty() usage seems wrong here.

Quoted 34 lines

mm/memory.c +1 -1

 	if ((flags & FAULT_FLAG_WRITE) && reuse_swap_page(page)) {
-		pte = maybe_mkwrite(pte_mkdirty(pte), vma);
+		pte = maybe_mkwrite(pte, vma);
 		flags &= ~FAULT_FLAG_WRITE;
 		ret |= VM_FAULT_WRITE;

mm/memory.c +0 -0

 		exclusive = 1;

mm/rmap.c +1 -1

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

mm/rmap.c +0 -0

 				goto discard;

mm/vmscan.c +1 -2

index 671e47e..7f520c9 100644
@@ -805,8 +805,7 @@ static enum page_references
 page_check_references(struct page *page,
 		return PAGEREF_KEEP;
 	}
 
-	if (PageAnon(page) && !pte_dirty && !PageSwapCache(page) &&
-			!PageDirty(page))
+	if (PageAnon(page) && !pte_dirty && !PageSwapCache(page))
 		*freeable = true;

Quoted 3 lines

/* Reclaim if clean, defer dirty pages to writeback */
--
1.9.3

Could we remove SetPageDirty(page); in try_to_free_swap() function based on this patch? Because your patch will make sure the pte is always dirty,
We don't need setpagedirty(),
The try_to_unmap() path will re-dirty the page during reclaim path,
Isn't it?

Thanks

Minchan Kim

author

2015-03-03 04:14 UTC #2 lore raw

Quoted 64 lines · Yalin

Quoted 25 lines
-----Original Message-----
From: Minchan Kim [mailto:[email protected]] On Behalf Of Minchan Kim
Sent: Tuesday, March 03, 2015 11:26 AM
To: Wang, Yalin
Cc: 'Michal Hocko'; 'Andrew Morton'; '[email protected]'; '[email protected]'; 'Rik van Riel'; 'Johannes Weiner'; 'Mel Gorman'; 'Shaohua Li'; Hugh Dickins; Cyrill Gorcunov
Subject: Re: [RFC V3] mm: change mm_advise_free to clear page dirty
Could you separte this patch in this patchset thread? It's tackling differnt problem.
As well, I had a question to previous thread about why shared page has a problem now but you didn't answer and send a new patchset. It makes reviewers/maintainer time waste/confuse. Please, don't hurry to send a code. Before that, resolve reviewers's comments.
Quoted 6 lines · Yalin
This patch add ClearPageDirty() to clear AnonPage dirty flag, if not clear page dirty for this anon page, the page will never be treated as freeable. We also make sure the shared AnonPage is not freeable, we implement it by dirty all copyed AnonPage pte, so that make sure the Anonpage will not become freeable, unless all process which shared this page call madvise_free syscall.
Quoted 6 lines
Please, spend more time to make description clear. I really doubt who understand this description without code inspection. :( Of course, I'm not a person to write description clear like native , either but just I'm sure I spend a more time to write description rather than coding, at least. :)
I see, I will send another mail for file private map pages. Sorry for my English expressions.
I think your solution is ok,
Your patch will make sure the anonpage pte will always be dirty. I add some comments for your patch:
Quoted 27 lines
---
 mm/madvise.c | 1 -
 mm/memory.c  | 9 +++++++--
 mm/rmap.c    | 2 +-
 mm/vmscan.c  | 3 +--
 4 files changed, 9 insertions(+), 6 deletions(-)
mm/madvise.c +0 -1
index 6d0fcb8..d64200e 100644
@@ -309,7 +309,6 @@ static int madvise_free_pte_range(pmd_t *pmd, unsigned
 long addr,
 				continue;
 			}
 
-			ClearPageDirty(page);
 			unlock_page(page);
 		}
mm/memory.c +0 -0
index 8ae52c9..2f45e77 100644
@@ -2460,9 +2460,14 @@ static int do_swap_page(struct mm_struct *mm, struct
 vm_area_struct *vma,
 

Quoted 10 lines

mm/memory.c static int do_swap_page(struct mm_struct *mm, struct · line 2469

Quoted 9 lines

mm/memory.c +6 -1

 	inc_mm_counter_fast(mm, MM_ANONPAGES);
 	dec_mm_counter_fast(mm, MM_SWAPENTS);
-	pte = mk_pte(page, vma->vm_page_prot);
+
+	/*
+	 * Every page swapped-out was pte_dirty so we makes pte dirty again.
+	 * MADV_FREE relys on it.
+	 */
+	pte = mk_pte(pte_mkdirty(page), vma->vm_page_prot);

pte_mkdirty() usage seems wrong here.

Argh, it reveals I didn't test even build. My shame. But RFC tag might mitigate my shame. :)
I will fix it if I send a formal version. Thanks for the review.

Quoted 35 lines

mm/memory.c +1 -1

 	if ((flags & FAULT_FLAG_WRITE) && reuse_swap_page(page)) {
-		pte = maybe_mkwrite(pte_mkdirty(pte), vma);
+		pte = maybe_mkwrite(pte, vma);
 		flags &= ~FAULT_FLAG_WRITE;
 		ret |= VM_FAULT_WRITE;

mm/memory.c +0 -0

 		exclusive = 1;

mm/rmap.c +1 -1

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

mm/rmap.c +0 -0

 				goto discard;

mm/vmscan.c +1 -2

index 671e47e..7f520c9 100644
@@ -805,8 +805,7 @@ static enum page_references
 page_check_references(struct page *page,
 		return PAGEREF_KEEP;
 	}
 
-	if (PageAnon(page) && !pte_dirty && !PageSwapCache(page) &&
-			!PageDirty(page))
+	if (PageAnon(page) && !pte_dirty && !PageSwapCache(page))
 		*freeable = true;

Quoted 8 lines

Quoted 3 lines
/* Reclaim if clean, defer dirty pages to writeback */
--
1.9.3
Could we remove SetPageDirty(page); in try_to_free_swap() function based on this patch? Because your patch will make sure the pte is always dirty,
We don't need setpagedirty(),
The try_to_unmap() path will re-dirty the page during reclaim path,
Isn't it?

I dont't know what side-effect we will have if we removes SetPageDirty. It might regress on tmpfs which would page without pte.
I don't want to have such risk in this patch.
If you want it, you could suggest it separately if this patch lands.

Signature · 8 lines

Kind regards,
Minchan Kim

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

Wang, Yalin

2015-03-03 06:46 UTC #3 lore raw

Quoted 139 lines

-----Original Message-----
From: Minchan Kim [mailto:[email protected]] On Behalf Of Minchan Kim
Sent: Tuesday, March 03, 2015 12:15 PM
To: Wang, Yalin
Cc: 'Michal Hocko'; 'Andrew Morton'; '[email protected]'; '[email protected]'; 'Rik van Riel'; 'Johannes Weiner'; 'Mel Gorman'; 'Shaohua Li'; Hugh Dickins; Cyrill Gorcunov
Subject: Re: [RFC V3] mm: change mm_advise_free to clear page dirty
Quoted 2 lines · Yalin
Quoted 2 lines
-----Original Message-----
From: Minchan Kim [mailto:[email protected]] On Behalf Of Minchan
Kim
Quoted 42 lines
Quoted 23 lines
Sent: Tuesday, March 03, 2015 11:26 AM
To: Wang, Yalin
Cc: 'Michal Hocko'; 'Andrew Morton'; '[email protected]'; '[email protected]'; 'Rik van Riel'; 'Johannes Weiner'; 'Mel Gorman'; 'Shaohua Li'; Hugh Dickins; Cyrill Gorcunov
Subject: Re: [RFC V3] mm: change mm_advise_free to clear page dirty
Could you separte this patch in this patchset thread? It's tackling differnt problem.
As well, I had a question to previous thread about why shared page has a problem now but you didn't answer and send a new patchset. It makes reviewers/maintainer time waste/confuse. Please, don't hurry to send a code. Before that, resolve reviewers's comments.
Quoted 6 lines · Yalin
This patch add ClearPageDirty() to clear AnonPage dirty flag, if not clear page dirty for this anon page, the page will never be treated as freeable. We also make sure the shared AnonPage is not freeable, we implement it by dirty all copyed AnonPage pte, so that make sure the Anonpage will not become freeable, unless all process which shared this page call madvise_free syscall.
Quoted 6 lines
Please, spend more time to make description clear. I really doubt who understand this description without code inspection. :( Of course, I'm not a person to write description clear like native , either but just I'm sure I spend a more time to write description rather than coding, at least. :)
I see, I will send another mail for file private map pages. Sorry for my English expressions.
I think your solution is ok,
Your patch will make sure the anonpage pte will always be dirty. I add some comments for your patch:
Quoted 7 lines
---
 mm/madvise.c | 1 -
 mm/memory.c  | 9 +++++++--
 mm/rmap.c    | 2 +-
 mm/vmscan.c  | 3 +--
 4 files changed, 9 insertions(+), 6 deletions(-)
Quoted 5 lines
mm/madvise.c static int madvise_free_pte_range(pmd_t *pmd,
Quoted 5 lines
mm/madvise.c +0 -0
index 6d0fcb8..d64200e 100644
@@ -309,7 +309,6 @@ static int madvise_free_pte_range(pmd_t *pmd,
unsigned
Quoted 8 lines
Quoted 8 lines
mm/madvise.c +0 -1
 long addr,
 				continue;
 			}
 
-			ClearPageDirty(page);
 			unlock_page(page);
 		}
Quoted 5 lines
mm/memory.c static int do_swap_page(struct mm_struct *mm,
Quoted 5 lines
mm/memory.c +0 -0
index 8ae52c9..2f45e77 100644
@@ -2460,9 +2460,14 @@ static int do_swap_page(struct mm_struct *mm,
struct
Quoted 12 lines
mm/memory.c static int do_swap_page(struct mm_struct *mm, · line 2469
Quoted 11 lines
mm/memory.c +6 -1
 vm_area_struct *vma,
 
 	inc_mm_counter_fast(mm, MM_ANONPAGES);
 	dec_mm_counter_fast(mm, MM_SWAPENTS);
-	pte = mk_pte(page, vma->vm_page_prot);
+
+	/*
+	 * Every page swapped-out was pte_dirty so we makes pte dirty again.
+	 * MADV_FREE relys on it.
+	 */
+	pte = mk_pte(pte_mkdirty(page), vma->vm_page_prot);
pte_mkdirty() usage seems wrong here.
Argh, it reveals I didn't test even build. My shame. But RFC tag might mitigate my shame. :)
I will fix it if I send a formal version. Thanks for the review.
Quoted 12 lines
mm/rmap.c static int try_to_unmap_one(struct page *page,
Quoted 12 lines
mm/memory.c +1 -1
 	if ((flags & FAULT_FLAG_WRITE) && reuse_swap_page(page)) {
-		pte = maybe_mkwrite(pte_mkdirty(pte), vma);
+		pte = maybe_mkwrite(pte, vma);
 		flags &= ~FAULT_FLAG_WRITE;
 		ret |= VM_FAULT_WRITE;
mm/memory.c +0 -0
 		exclusive = 1;
mm/rmap.c +0 -0
index 47b3ba8..34c1d66 100644
@@ -1268,7 +1268,7 @@ static int try_to_unmap_one(struct page *page,
struct
Quoted 23 lines
Quoted 23 lines
mm/rmap.c +1 -1
 vm_area_struct *vma,
 
 		if (flags & TTU_FREE) {
 			VM_BUG_ON_PAGE(PageSwapCache(page), page);
-			if (!dirty && !PageDirty(page)) {
+			if (!dirty) {
 				/* It's a freeable page by MADV_FREE */
 				dec_mm_counter(mm, MM_ANONPAGES);
mm/rmap.c +0 -0
 				goto discard;
mm/vmscan.c +1 -2
index 671e47e..7f520c9 100644
@@ -805,8 +805,7 @@ static enum page_references
 page_check_references(struct page *page,
 		return PAGEREF_KEEP;
 	}
 
-	if (PageAnon(page) && !pte_dirty && !PageSwapCache(page) &&
-			!PageDirty(page))
+	if (PageAnon(page) && !pte_dirty && !PageSwapCache(page))
 		*freeable = true;
 
Quoted 4 lines
Quoted 3 lines
/* Reclaim if clean, defer dirty pages to writeback */
--
1.9.3
Could we remove SetPageDirty(page); in try_to_free_swap() function based
on this patch?
Quoted 4 lines
Because your patch will make sure the pte is always dirty, We don't need setpagedirty(),
The try_to_unmap() path will re-dirty the page during reclaim path, Isn't it?

Quoted 5 lines

I dont't know what side-effect we will have if we removes SetPageDirty. It might regress on tmpfs which would page without pte.
I don't want to have such risk in this patch.
If you want it, you could suggest it separately if this patch lands.

Ok, Could you send out your change as a normal patch for more related maintainers to review /comment it?

Thanks

Minchan Kim

author

2015-03-03 13:41 UTC #4 lore raw

Quoted 139 lines · Yalin

Quoted 139 lines
-----Original Message-----
From: Minchan Kim [mailto:[email protected]] On Behalf Of Minchan Kim
Sent: Tuesday, March 03, 2015 12:15 PM
To: Wang, Yalin
Cc: 'Michal Hocko'; 'Andrew Morton'; '[email protected]'; '[email protected]'; 'Rik van Riel'; 'Johannes Weiner'; 'Mel Gorman'; 'Shaohua Li'; Hugh Dickins; Cyrill Gorcunov
Subject: Re: [RFC V3] mm: change mm_advise_free to clear page dirty
Quoted 2 lines · Yalin
Quoted 2 lines
-----Original Message-----
From: Minchan Kim [mailto:[email protected]] On Behalf Of Minchan
Kim
Quoted 42 lines
Quoted 23 lines
Sent: Tuesday, March 03, 2015 11:26 AM
To: Wang, Yalin
Cc: 'Michal Hocko'; 'Andrew Morton'; '[email protected]'; '[email protected]'; 'Rik van Riel'; 'Johannes Weiner'; 'Mel Gorman'; 'Shaohua Li'; Hugh Dickins; Cyrill Gorcunov
Subject: Re: [RFC V3] mm: change mm_advise_free to clear page dirty
Could you separte this patch in this patchset thread? It's tackling differnt problem.
As well, I had a question to previous thread about why shared page has a problem now but you didn't answer and send a new patchset. It makes reviewers/maintainer time waste/confuse. Please, don't hurry to send a code. Before that, resolve reviewers's comments.
Quoted 6 lines · Yalin
This patch add ClearPageDirty() to clear AnonPage dirty flag, if not clear page dirty for this anon page, the page will never be treated as freeable. We also make sure the shared AnonPage is not freeable, we implement it by dirty all copyed AnonPage pte, so that make sure the Anonpage will not become freeable, unless all process which shared this page call madvise_free syscall.
Quoted 6 lines
Please, spend more time to make description clear. I really doubt who understand this description without code inspection. :( Of course, I'm not a person to write description clear like native , either but just I'm sure I spend a more time to write description rather than coding, at least. :)
I see, I will send another mail for file private map pages. Sorry for my English expressions.
I think your solution is ok,
Your patch will make sure the anonpage pte will always be dirty. I add some comments for your patch:
Quoted 7 lines
---
 mm/madvise.c | 1 -
 mm/memory.c  | 9 +++++++--
 mm/rmap.c    | 2 +-
 mm/vmscan.c  | 3 +--
 4 files changed, 9 insertions(+), 6 deletions(-)
Quoted 5 lines
mm/madvise.c static int madvise_free_pte_range(pmd_t *pmd,
Quoted 5 lines
mm/madvise.c +0 -0
index 6d0fcb8..d64200e 100644
@@ -309,7 +309,6 @@ static int madvise_free_pte_range(pmd_t *pmd,
unsigned
Quoted 8 lines
Quoted 8 lines
mm/madvise.c +0 -1
 long addr,
 				continue;
 			}
 
-			ClearPageDirty(page);
 			unlock_page(page);
 		}
Quoted 5 lines
mm/memory.c static int do_swap_page(struct mm_struct *mm,
Quoted 5 lines
mm/memory.c +0 -0
index 8ae52c9..2f45e77 100644
@@ -2460,9 +2460,14 @@ static int do_swap_page(struct mm_struct *mm,
struct
Quoted 12 lines
mm/memory.c static int do_swap_page(struct mm_struct *mm, · line 2469
Quoted 11 lines
mm/memory.c +6 -1
 vm_area_struct *vma,
 
 	inc_mm_counter_fast(mm, MM_ANONPAGES);
 	dec_mm_counter_fast(mm, MM_SWAPENTS);
-	pte = mk_pte(page, vma->vm_page_prot);
+
+	/*
+	 * Every page swapped-out was pte_dirty so we makes pte dirty again.
+	 * MADV_FREE relys on it.
+	 */
+	pte = mk_pte(pte_mkdirty(page), vma->vm_page_prot);
pte_mkdirty() usage seems wrong here.
Argh, it reveals I didn't test even build. My shame. But RFC tag might mitigate my shame. :)
I will fix it if I send a formal version. Thanks for the review.
Quoted 12 lines
mm/rmap.c static int try_to_unmap_one(struct page *page,
Quoted 12 lines
mm/memory.c +1 -1
 	if ((flags & FAULT_FLAG_WRITE) && reuse_swap_page(page)) {
-		pte = maybe_mkwrite(pte_mkdirty(pte), vma);
+		pte = maybe_mkwrite(pte, vma);
 		flags &= ~FAULT_FLAG_WRITE;
 		ret |= VM_FAULT_WRITE;
mm/memory.c +0 -0
 		exclusive = 1;
mm/rmap.c +0 -0
index 47b3ba8..34c1d66 100644
@@ -1268,7 +1268,7 @@ static int try_to_unmap_one(struct page *page,
struct
Quoted 23 lines
Quoted 23 lines
mm/rmap.c +1 -1
 vm_area_struct *vma,
 
 		if (flags & TTU_FREE) {
 			VM_BUG_ON_PAGE(PageSwapCache(page), page);
-			if (!dirty && !PageDirty(page)) {
+			if (!dirty) {
 				/* It's a freeable page by MADV_FREE */
 				dec_mm_counter(mm, MM_ANONPAGES);
mm/rmap.c +0 -0
 				goto discard;
mm/vmscan.c +1 -2
index 671e47e..7f520c9 100644
@@ -805,8 +805,7 @@ static enum page_references
 page_check_references(struct page *page,
 		return PAGEREF_KEEP;
 	}
 
-	if (PageAnon(page) && !pte_dirty && !PageSwapCache(page) &&
-			!PageDirty(page))
+	if (PageAnon(page) && !pte_dirty && !PageSwapCache(page))
 		*freeable = true;
 
Quoted 4 lines
Quoted 3 lines
/* Reclaim if clean, defer dirty pages to writeback */
--
1.9.3
Could we remove SetPageDirty(page); in try_to_free_swap() function based
on this patch?
Quoted 4 lines
Because your patch will make sure the pte is always dirty, We don't need setpagedirty(),
The try_to_unmap() path will re-dirty the page during reclaim path, Isn't it?

Quoted 6 lines

Quoted 5 lines
I dont't know what side-effect we will have if we removes SetPageDirty. It might regress on tmpfs which would page without pte.
I don't want to have such risk in this patch.
If you want it, you could suggest it separately if this patch lands.
Ok, Could you send out your change as a normal patch for more related maintainers to review /comment it?

NP but let's wait a few days to see if we have a luck which they grab a time slot to review. :)

Thanks.

Signature · 8 lines

Kind regards,
Minchan Kim

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

Michal Hocko

2015-03-05 15:35 UTC #5 lore raw

In reply to Minchan Kim

On Tue 03-03-15 12:25:51, Minchan Kim wrote:
[...]

Quoted 4 lines

From 30c6d5b35a3dc7e451041183ce5efd6a6c42bf88 Mon Sep 17 00:00:00 2001
From: Minchan Kim <[email protected]>
Date: Tue, 3 Mar 2015 10:06:59 +0900
Subject: [RFC] mm: make every pte dirty on do_swap_page

Hi Minchan, could you resend this patch separately. I am afraid that this one got so convoluted with originally unrelated issues that people might miss it.

Thanks!

Signature · 8 lines

Michal Hocko
SUSE Labs

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

Minchan Kim

author

2015-03-09 00:57 UTC #6 lore raw

Hello Michal,

Quoted 7 lines · Michal Hocko

On Tue 03-03-15 12:25:51, Minchan Kim wrote:
[...]
Quoted 4 lines
From 30c6d5b35a3dc7e451041183ce5efd6a6c42bf88 Mon Sep 17 00:00:00 2001
From: Minchan Kim <[email protected]>
Date: Tue, 3 Mar 2015 10:06:59 +0900
Subject: [RFC] mm: make every pte dirty on do_swap_page

Quoted 5 lines

Hi Minchan, could you resend this patch separately. I am afraid that this one got so convoluted with originally unrelated issues that people might miss it.
Thanks!

No problem. Thanks for the review.
I will resend it this week but I'm afraid everybody will be in LSF/MM so they will be busy with hardwork in there. :)

Quoted 3 lines

Signature · 2 lines
Michal Hocko
SUSE Labs

Signature · 8 lines

Kind regards,
Minchan Kim

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