Re: [PATCH v3 4/8] mm/rmap: Split migration into its own function

[Alistair Popple <apopple@nvidia.com>]

On Wednesday, 3 March 2021 9:08:15 AM AEDT Zi Yan wrote:
> On 26 Feb 2021, at 2:18, Alistair Popple wrote:

> > diff --git a/include/linux/rmap.h b/include/linux/rmap.h
> > index 7f1ee411bd7b..77fa17de51d7 100644
> > --- a/include/linux/rmap.h
> > +++ b/include/linux/rmap.h
> > @@ -86,8 +86,6 @@ struct anon_vma_chain {
> >  };
> >
> >  enum ttu_flags {
> > -	TTU_MIGRATION		= 0x1,	/* migration mode */
> > -
> >  	TTU_SPLIT_HUGE_PMD	= 0x4,	/* split huge PMD if any */
> 
> It implies freeze in try_to_migrate() and no freeze in try_to_unmap(). I 
think
> we need some comments here, above try_to_migrate(), and above try_to_unmap()
> to clarify the implication.

Sure. This confused me for a bit and I was initially tempted to leave 
TTU_SPLIT_FREEZE as a separate mode flag but looking at what freeze actually 
does it made sense to remove it because try_to_migrate() is for installing 
migration entries (which is what freeze does) and try_to_unmap() just unmaps. 
So I'll add some comments to that effect.
 
> >  	TTU_IGNORE_MLOCK	= 0x8,	/* ignore mlock */
> >  	TTU_IGNORE_HWPOISON	= 0x20,	/* corrupted page is recoverable */
> > @@ -96,7 +94,6 @@ enum ttu_flags {
> >  					 * do a final flush if necessary */
> >  	TTU_RMAP_LOCKED		= 0x80,	/* do not grab rmap lock:
> >  					 * caller holds it */
> > -	TTU_SPLIT_FREEZE	= 0x100,		/* freeze pte under splitting thp */
> >  };
> >
> >  #ifdef CONFIG_MMU
> > @@ -193,6 +190,7 @@ static inline void page_dup_rmap(struct page *page, 
bool compound)
> >  int page_referenced(struct page *, int is_locked,
> >  			struct mem_cgroup *memcg, unsigned long *vm_flags);
> >
> > +bool try_to_migrate(struct page *page, enum ttu_flags flags);
> >  bool try_to_unmap(struct page *, enum ttu_flags flags);
> >
> >  /* Avoid racy checks */
> > diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> > index d00b93dc2d9e..357052a4567b 100644
> > --- a/mm/huge_memory.c
> > +++ b/mm/huge_memory.c
> > @@ -2351,16 +2351,16 @@ void vma_adjust_trans_huge(struct vm_area_struct 
*vma,
> >
> >  static void unmap_page(struct page *page)
> >  {
> > -	enum ttu_flags ttu_flags = TTU_IGNORE_MLOCK |
> > -		TTU_RMAP_LOCKED | TTU_SPLIT_HUGE_PMD;
> > +	enum ttu_flags ttu_flags = TTU_RMAP_LOCKED | TTU_SPLIT_HUGE_PMD;
> >  	bool unmap_success;
> >
> >  	VM_BUG_ON_PAGE(!PageHead(page), page);
> >
> >  	if (PageAnon(page))
> > -		ttu_flags |= TTU_SPLIT_FREEZE;
> > -
> > -	unmap_success = try_to_unmap(page, ttu_flags);
> > +		unmap_success = try_to_migrate(page, ttu_flags);
> > +	else
> > +		unmap_success = try_to_unmap(page, ttu_flags |
> > +						TTU_IGNORE_MLOCK);
> 
> I think we need a comment here about why anonymous pages need 
try_to_migrate()
> and others need try_to_unmap().

Historically this comes from baa355fd3314 ("thp: file pages support for 
split_huge_page()") which says:

"We don't setup migration entries. Just unmap pages. It helps handling cases 
when i_size is in the middle of the page: no need handle unmap pages beyond 
i_size manually."

But I'll add a comment here, thanks.

 - Alistair

> Thanks.
> 
> —
> Best Regards,
> Yan Zi