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

From: Minchan Kim
Date: Thu Sep 06 2012 - 22:22:49 EST

On Thu, Sep 06, 2012 at 10:03:25AM +0100, Mel Gorman wrote:
> On Thu, Sep 06, 2012 at 09:29:35AM +0100, Mel Gorman wrote:
> > On Thu, Sep 06, 2012 at 02:31:12PM +0900, Minchan Kim wrote:
> > > Hi Mel,
> > >
> > > On Wed, Sep 05, 2012 at 11:56:11AM +0100, Mel Gorman wrote:
> > > > On Wed, Sep 05, 2012 at 05:11:13PM +0900, Minchan Kim wrote:
> > > > > This patch introudes MIGRATE_DISCARD mode in migration.
> > > > > 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
> > > > > less free pages for migration targets so it could avoid memory reclaiming
> > > > > to get free pages, which is another factor increase latency.
> > > > >
> > > >
> > > > Bah, this was released while I was reviewing the older version. I did
> > > > not read this one as closely but I see the enum problems have gone away
> > > > at least. I'd still prefer if CMA had an additional helper to discard
> > > > some pages with shrink_page_list() and migrate the remaining pages with
> > > > migrate_pages(). That would remove the need to add a MIGRATE_DISCARD
> > > > migrate mode at all.
> > >
> > > I am not convinced with your point. What's the benefit on separating
> > > reclaim and migration? For just removing MIGRATE_DISCARD mode?
> >
> > Maintainability. There are reclaim functions and there are migration
> > functions. Your patch takes migrate_pages() and makes it partially a
> > reclaim function mixing up the responsibilities of migrate.c and vmscan.c.
> >
> > > I don't think it's not bad because my implementation is very simple(maybe
> > > it's much simpler than separating reclaim and migration) and
> > > could be used by others like memory-hotplug in future.
> >
> > They could also have used the helper function from CMA that takes a list
> > of pages, reclaims some and migrates other.
> >
>
> I also do not accept that your approach is inherently simpler than what I
> proposed to you. This is not tested at all but it should be functionally
> similar to both your patches except that it keeps the responsibility for
> reclaim in vmscan.c
>
> Your diffstats are
>
> 8 files changed, 39 insertions(+), 36 deletions(-)
> 3 files changed, 46 insertions(+), 4 deletions(-)
>
> Mine is
>
> 3 files changed, 32 insertions(+), 5 deletions(-)
>
> Fewer files changed and fewer lines inserted.
>
> ---8<---
> mm: cma: Discard clean pages during contiguous allocation instead of migration
>
> This patch drops clean cache pages instead of migration during
> alloc_contig_range() to minimise allocation latency by reducing the amount
> of migration is necessary. It's useful for CMA because latency of migration
> is more important than evicting the background processes working set.
>
> Prototype-not-signed-off-but-feel-free-to-pick-up-and-test
> ---
> mm/internal.h | 1 +
> mm/page_alloc.c | 2 ++
> mm/vmscan.c | 34 +++++++++++++++++++++++++++++-----
> 3 files changed, 32 insertions(+), 5 deletions(-)
>
> diff --git a/mm/internal.h b/mm/internal.h
> index b8c91b3..6d4bdf9 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -356,3 +356,4 @@ 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 list_head *page_list);
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index c66fb87..977bdb2 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -5670,6 +5670,8 @@ static int __alloc_contig_migrate_range(unsigned long start, unsigned long end)
> break;
> }
>
> + reclaim_clean_pages_from_list(&cc.migratepages);
> +
> 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..ccf7bc2 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -703,7 +703,7 @@ static unsigned long shrink_page_list(struct list_head *page_list,
> goto keep;
>
> VM_BUG_ON(PageActive(page));
> - VM_BUG_ON(page_zone(page) != zone);
> + VM_BUG_ON(zone && page_zone(page) != zone);
>
> sc->nr_scanned++;
>
> @@ -817,7 +817,9 @@ static unsigned long shrink_page_list(struct list_head *page_list,
> * except we already have the page isolated
> * and know it's dirty
> */
> - inc_zone_page_state(page, NR_VMSCAN_IMMEDIATE);
> + if (zone)
> + inc_zone_page_state(page,
> + NR_VMSCAN_IMMEDIATE);
> SetPageReclaim(page);
>
> goto keep_locked;
> @@ -947,7 +949,7 @@ keep:
> * back off and wait for congestion to clear because further reclaim
> * will encounter the same problem
> */
> - if (nr_dirty && nr_dirty == nr_congested && global_reclaim(sc))
> + if (zone && nr_dirty && nr_dirty == nr_congested && global_reclaim(sc))
> zone_set_flag(zone, ZONE_CONGESTED);
>
> free_hot_cold_page_list(&free_pages, 1);
> @@ -955,11 +957,33 @@ keep:
> list_splice(&ret_pages, page_list);
> count_vm_events(PGACTIVATE, pgactivate);
> mem_cgroup_uncharge_end();
> - *ret_nr_dirty += nr_dirty;
> - *ret_nr_writeback += nr_writeback;
> + if (ret_nr_dirty)
> + *ret_nr_dirty += nr_dirty;
> + if (ret_nr_writeback)
> + *ret_nr_writeback += nr_writeback;
> return nr_reclaimed;
> }
>
> +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.

How about this?