Re: [PATCH mm-unstable 5/5] mm/mempolicy: Convert migrate_page_add() to migrate_folio_add()

[Matthew Wilcox]

On Thu, Jan 19, 2023 at 09:24:16AM +0800, Yin, Fengwei wrote:
> On 1/19/2023 7:22 AM, Vishal Moola (Oracle) wrote:
> > @@ -1022,27 +1022,23 @@ static long do_get_mempolicy(int *policy, nodemask_t *nmask,
> >  }
> >  
> >  #ifdef CONFIG_MIGRATION
> > -/*
> > - * page migration, thp tail pages can be passed.
> > - */
> > -static int migrate_page_add(struct page *page, struct list_head *pagelist,
> > +static int migrate_folio_add(struct folio *folio, struct list_head *foliolist,
> >  				unsigned long flags)
> >  {
> > -	struct page *head = compound_head(page);
> >  	/*
> > -	 * Avoid migrating a page that is shared with others.
> > +	 * Avoid migrating a folio that is shared with others.
> >  	 */
> > -	if ((flags & MPOL_MF_MOVE_ALL) || page_mapcount(head) == 1) {
> > -		if (!isolate_lru_page(head)) {
> > -			list_add_tail(&head->lru, pagelist);
> > -			mod_node_page_state(page_pgdat(head),
> > -				NR_ISOLATED_ANON + page_is_file_lru(head),
> > -				thp_nr_pages(head));
> > +	if ((flags & MPOL_MF_MOVE_ALL) || folio_mapcount(folio) == 1) {
> One question to the page_mapcount -> folio_mapcount here.
> 
> For a large folio with 0 entire mapcount, if the first sub-page and any
> other sub-page are mapped, page_mapcount(head) == 1 is true while
> folio_mapcount(folio) == 1 is not.

We had a good discussion about this in today's THP Cabal meeting [1].  I
didn't quite check everything that I said was true, so let me summarise
& correct it now ...

 - This is a heuristic.  We're trying to see whether this folio is
   mapped by multiple processes (because if it is, it's probably not
   worth migrating).  If the heuristic is wrong, it probably doesn't
   matter _too_ much?
 - A proper heuristic for this would be
		folio_total_mapcount(folio) == folio_nr_pages(folio)
   but this would be expensive to calculate as it requires examining
   512 cachelines for a 2MB page.
 - For a large folio which is smaller than PMD size, we're guaranteed
   that folio_mapcount() is 0 today.
 - In the meeting I said that page_mapcount() of the head of a THP
   page was zero; that's not true; I had forgotten that we added in
   entire_mapcount to the individual page mapcount.

so I now think this should be:

	page_mapcount(folio_page(folio, 0))

with an explanation that checking every page is too heavy-weight.
Maybe it should be its own function:

static inline int folio_estimated_mapcount(folio)
{
	return page_mapcount(folio_page(folio, 0));
}

with a nice comment explaining what's going on.

[1] https://www.youtube.com/watch?v=A3PoGQQQD3Q is the recording of
today's meeting.