Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 20 Jan 2023 19:47:43 +0000
From:   Matthew Wilcox <willy@infradead.org>
To:     "Yin, Fengwei" <fengwei.yin@intel.com>
Cc:     "Vishal Moola (Oracle)" <vishal.moola@gmail.com>,
        linux-mm@kvack.org, akpm@linux-foundation.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH mm-unstable 5/5] mm/mempolicy: Convert migrate_page_add()
 to migrate_folio_add()
Message-ID: <Y8rv3/GfW8XDDXj7@casper.infradead.org>
References: <20230118232219.27038-1-vishal.moola@gmail.com>
 <20230118232219.27038-6-vishal.moola@gmail.com>
 <4dd1a4f4-4da6-8079-a8de-bea7d8c18681@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <4dd1a4f4-4da6-8079-a8de-bea7d8c18681@intel.com>
Precedence: bulk

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.