Re: [PATCH 2/2] mm/damon: Add a new scheme to support demotion on tiered memory system

[Baolin Wang]

On 12/22/2021 4:43 PM, SeongJae Park wrote:
> On Tue, 21 Dec 2021 22:18:06 +0800 Baolin Wang <baolin.wang@xxxxxxxxxxxxxxxxx> wrote:
>
> > Hi SeongJae,
> >
> > On 12/21/2021 9:24 PM, SeongJae Park Wrote:
> > > Hi Baolin,
> > >
> > >
> > > Thank you for this great patch!  I left some comments below.
> > >
> > > On Tue, 21 Dec 2021 17:18:04 +0800 Baolin Wang <baolin.wang@xxxxxxxxxxxxxxxxx> wrote:
> > >
> > > > On tiered memory system, the reclaim path in shrink_page_list() already
> > > > support demoting pages to slow memory node instead of discarding the
> > > > pages. However, at that time the fast memory node memory wartermark is
> > > > already tense, which will increase the memory allocation latency during
> > > > page demotion.
> > > >
> > > > We can rely on the DAMON in user space to help to monitor the cold
> > > > memory on fast memory node, and demote the cold pages to slow memory
> > > > node proactively to keep the fast memory node in a healthy state.
> > > > Thus this patch introduces a new scheme named DAMOS_DEMOTE to support
> > > > this feature.
> > > >
> > > > Signed-off-by: Baolin Wang <baolin.wang@xxxxxxxxxxxxxxxxx>
> > > > ---

[snip]

> > > > +		list_add(&page->lru, &pagelist);
> > > > +		target_nid = next_demotion_node(page_to_nid(page));
> > > > +		nr_pages = thp_nr_pages(page);
> > > > +
> > > > +		ret = migrate_pages(&pagelist, alloc_demote_page, NULL,
> > > > +				    target_nid, MIGRATE_SYNC, MR_DEMOTION,
> > > > +				    &nr_succeeded);
> > > > +		if (ret) {
> > > > +			if (!list_empty(&pagelist)) {
> > > > +				list_del(&page->lru);
> > > > +				mod_node_page_state(page_pgdat(page),
> > > > +						    NR_ISOLATED_ANON + page_is_file_lru(page),
> > > > +						    -nr_pages);
> > > > +				putback_lru_page(page);
> > > > +			}
> > > > +		} else {
> > > > +			__count_vm_events(PGDEMOTE_DIRECT, nr_succeeded);
> > > > +			demoted_pages += nr_succeeded;
> > > > +		}
> > >
> > > Why should we do above work for each page on our own instead of exporting and
> > > calling 'demote_page_list()'?
> >
> > Cuase demote_page_list() is used for demote cold pages which are from
> > one same fast memory node, they can have one same target demotion node.
> >
> > But for the regions for the process, we can get some cold pages from
> > different fast memory nodes (suppose we have mulptiple dram node on the
> > system), so its target demotion node may be different. Thus we should
> > migrate each cold pages with getting the correct target demotion node.
>
> Thank you for the kind explanation.  But, I still want to reuse the code rather
> than copying if possible.  How about below dumb ideas off the top of my head?
>
> 1. Call demote_page_list() for each page from here

Sounds reasonable.

> 2. Call demote_page_list() for each page from damos_migrate_pmd_entry()

We are under mmap lock in damos_migrate_pmd_entry(), it is not suitable 
to do page migration.

> 3. Make damos_migrate_pages_walk_ops to configure multiple demote_pages lists,
>     each per fast memory node, and calling demote_page_list() here for each
>     per-fast-memory-node demote_pages list.

Which will bring more complexity I think, and we should avoid doing 
migration under mmap lock.

> 4. Make demote_page_list() handles this case and use it.  e.g., NULL pgdat
>     parameter means the pages are not from same node.

Thanks for your suggestion, actually after more thinking, I think we can 
reuse the demote_page_list() and it can be easy to change. Somethink 
like below on top of my current patch, how do you think? Thanks.

--- a/mm/damon/vaddr.c
+++ b/mm/damon/vaddr.c
@@ -796,7 +796,7 @@ static unsigned long damos_demote(struct 
damon_target *target,
        struct mm_struct *mm;
        LIST_HEAD(demote_pages);
        LIST_HEAD(pagelist);
-       int target_nid, nr_pages, ret = 0;
+       int nr_pages;
        unsigned int nr_succeeded, demoted_pages = 0;
        struct page *page, *next;

@@ -818,14 +818,11 @@ static unsigned long damos_demote(struct 
damon_target *target,
                return 0;

        list_for_each_entry_safe(page, next, &demote_pages, lru) {
-               list_add(&page->lru, &pagelist);
-               target_nid = next_demotion_node(page_to_nid(page));
                nr_pages = thp_nr_pages(page);
+               list_add(&page->lru, &pagelist);

-               ret = migrate_pages(&pagelist, alloc_demote_page, NULL,
-                                   target_nid, MIGRATE_SYNC, MR_DEMOTION,
-                                   &nr_succeeded);
-               if (ret) {
+               nr_succeeded = demote_page_list(pagelist, page_pgdat(page));
+               if (!nr_succeeded) {
                        if (!list_empty(&pagelist)) {
                                list_del(&page->lru);
                                mod_node_page_state(page_pgdat(page),
@@ -834,11 +831,9 @@ static unsigned long damos_demote(struct 
damon_target *target,
                                putback_lru_page(page);
                        }
                } else {
-                       __count_vm_events(PGDEMOTE_DIRECT, nr_succeeded);
                        demoted_pages += nr_succeeded;
                }

-               INIT_LIST_HEAD(&pagelist);
                cond_resched();
        }