Re: [PATCH 2/2] mm: support MIGRATE_DISCARD

[Mel Gorman]

On Fri, Sep 07, 2012 at 11:24:34AM +0900, Minchan Kim wrote:
> > +unsigned long reclaim_clean_pages_from_list(struct list_head *page_list)
> > +{
> > +	struct scan_control sc = {
> > +		.gfp_mask = GFP_KERNEL,
> > +		.priority = DEF_PRIORITY,
> > +	};
> > +	unsigned long ret;
> > +	struct page *page, *next;
> > +	LIST_HEAD(clean_pages);
> > +
> > +	list_for_each_entry_safe(page, next, page_list, lru) {
> > +		if (page_is_file_cache(page) && !PageDirty(page))
> > +			list_move(&page->lru, &clean_pages);
> > +	}
> > +
> > +	ret = shrink_page_list(&clean_pages, NULL, &sc, NULL, NULL);
> > +	list_splice(&clean_pages, page_list);
> > +	return ret;
> > +}
> > +
> 
> It's different with my point.
> My intention is to free mapped clean pages as well as not-mapped's one.
> 

Then just set may_unmap == 1?

struct scan_control sc = {
	.gfp_mask = GFP_KERNEL,
	.may_unmap = 1,
	.priority = DEF_PRIORITY
};

> How about this?
> 
> From 0f6986e943e55929b4d7b0220a1c24a6bae1a24d Mon Sep 17 00:00:00 2001
> From: Minchan Kim <minchan@xxxxxxxxxx>
> Date: Fri, 7 Sep 2012 11:20:48 +0900
> Subject: [PATCH] mm: cma: Discard clean pages during contiguous allocation
>  instead of migration
> 
> This patch introudes MIGRATE_DISCARD mode in migration.

This line is now redundant.

> It drops *clean cache pages* instead of migration so that
> migration latency could be reduced by avoiding (memcpy + page remapping).
> It's useful for CMA because latency of migration is very important rather
> than eviction of background processes's workingset. In addition, it needs

processes working set

> less free pages for migration targets so it could avoid memory reclaiming
> to get free pages, which is another factor increase latency.
> 
> Cc: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
> Cc: Michal Nazarewicz <mina86@xxxxxxxxxx>
> Cc: Rik van Riel <riel@xxxxxxxxxx>
> Signed-off-by: Mel Gorman <mgorman@xxxxxxx>
> Signed-off-by: Minchan Kim <minchan@xxxxxxxxxx>
> ---
>  mm/internal.h   |    2 ++
>  mm/page_alloc.c |    2 ++
>  mm/vmscan.c     |   42 ++++++++++++++++++++++++++++++++++++------
>  3 files changed, 40 insertions(+), 6 deletions(-)
> 
> diff --git a/mm/internal.h b/mm/internal.h
> index 3314f79..be09a7e 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -355,3 +355,5 @@ extern unsigned long vm_mmap_pgoff(struct file *, unsigned long,
>          unsigned long, unsigned long);
>  
>  extern void set_pageblock_order(void);
> +unsigned long reclaim_clean_pages_from_list(struct zone *zone,
> +				struct list_head *page_list);
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index ba3100a..bf35e59 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -5668,6 +5668,8 @@ static int __alloc_contig_migrate_range(unsigned long start, unsigned long end)
>  			break;
>  		}
>  
> +		reclaim_clean_pages_from_list(&cc.migratepages, cc.zone);
> +

The signature is zone, list_head but you pass in list_head, zone. I
expect that generates fun compile warnings and crashes at runtime.

The reason that I did not pass in zone to reclaim_clean_pages_from_list()
in my version is because I did not want to force the list to all be from the
same zone. That will just happen to be true but shrink_page_list() really
does not care about the zone from the perspective of this new function. It's
used for a debugging check (disable it if !zone) and setting ZONE_CONGESTED
which will never be important as far as reclaim_clean_pages_from_list()
is concerned. It will never call pageout().

It's up to you if you really want to pass zone in but I would not bother
if I was you :)

>  		ret = migrate_pages(&cc.migratepages,
>  				    __alloc_contig_migrate_alloc,
>  				    0, false, MIGRATE_SYNC);
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 8d01243..525355e 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -674,8 +674,10 @@ static enum page_references page_check_references(struct page *page,
>  static unsigned long shrink_page_list(struct list_head *page_list,
>  				      struct zone *zone,
>  				      struct scan_control *sc,
> +				      enum ttu_flags ttu_flags,
>  				      unsigned long *ret_nr_dirty,
> -				      unsigned long *ret_nr_writeback)
> +				      unsigned long *ret_nr_writeback,
> +				      bool force_reclaim)
>  {
>  	LIST_HEAD(ret_pages);
>  	LIST_HEAD(free_pages);
> @@ -689,10 +691,10 @@ static unsigned long shrink_page_list(struct list_head *page_list,
>  
>  	mem_cgroup_uncharge_start();
>  	while (!list_empty(page_list)) {
> -		enum page_references references;
>  		struct address_space *mapping;
>  		struct page *page;
>  		int may_enter_fs;
> +		enum page_references references = PAGEREF_RECLAIM;
>  
>  		cond_resched();
>  
> @@ -758,7 +760,9 @@ static unsigned long shrink_page_list(struct list_head *page_list,
>  			wait_on_page_writeback(page);
>  		}
>  
> -		references = page_check_references(page, sc);
> +		if (!force_reclaim)
> +			references = page_check_references(page, sc);
> +
>  		switch (references) {
>  		case PAGEREF_ACTIVATE:
>  			goto activate_locked;

Good point.

> @@ -788,7 +792,7 @@ static unsigned long shrink_page_list(struct list_head *page_list,
>  		 * processes. Try to unmap it here.
>  		 */
>  		if (page_mapped(page) && mapping) {
> -			switch (try_to_unmap(page, TTU_UNMAP)) {
> +			switch (try_to_unmap(page, ttu_flags)) {
>  			case SWAP_FAIL:
>  				goto activate_locked;
>  			case SWAP_AGAIN:

Another good point.

> @@ -960,6 +964,32 @@ keep:
>  	return nr_reclaimed;
>  }
>  
> +unsigned long reclaim_clean_pages_from_list(struct zone *zone,
> +					struct list_head *page_list)
> +{
> +	struct scan_control sc = {
> +		.gfp_mask = GFP_KERNEL,
> +		.priority = DEF_PRIORITY,
> +		.may_unmap = 1,
> +	};

Yep.

> +	unsigned long ret, dummy1, dummy2;
> +	struct page *page, *next;
> +	LIST_HEAD(clean_pages);
> +
> +	list_for_each_entry_safe(page, next, page_list, lru) {
> +		if (page_is_file_cache(page) && !PageDirty(page)) {
> +			ClearPageActive(page);

Yep.

> +			list_move(&page->lru, &clean_pages);
> +		}
> +	}
> +
> +	ret = shrink_page_list(&clean_pages, zone, &sc,
> +				TTU_UNMAP|TTU_IGNORE_ACCESS,
> +				&dummy1, &dummy2, true);
> +	list_splice(&clean_pages, page_list);
> +	return ret;
> +}
> +
>  /*
>   * Attempt to remove the specified page from its LRU.  Only take this page
>   * if it is of the appropriate PageActive status.  Pages which are being
> @@ -1278,8 +1308,8 @@ shrink_inactive_list(unsigned long nr_to_scan, struct lruvec *lruvec,
>  	if (nr_taken == 0)
>  		return 0;
>  
> -	nr_reclaimed = shrink_page_list(&page_list, zone, sc,
> -						&nr_dirty, &nr_writeback);
> +	nr_reclaimed = shrink_page_list(&page_list, zone, sc, TTU_UNMAP,
> +					&nr_dirty, &nr_writeback, false);
>  
>  	spin_lock_irq(&zone->lru_lock);
>  

So other than the mix up of order parameters I think this should work.

-- 
Mel Gorman
SUSE Labs
--
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/