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

[yalin wang]

> On Oct 27, 2015, at 10:09, Hugh Dickins <hughd@xxxxxxxxxx> 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@xxxxxxxxxx>
>> Cc: Mel Gorman <mgorman@xxxxxxx>
>> Signed-off-by: Minchan Kim <minchan@xxxxxxxxxx>
> 
> Acked-by: Hugh Dickins <hughd@xxxxxxxxxx>
> 
> 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@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/