Re: [PATCH v2 1/2] mm: migrate: Fix return value if all subpages of THPs are migrated successfully

[Baolin Wang]

On 10/24/2022 3:24 PM, Alistair Popple wrote:
> Baolin Wang <baolin.wang@xxxxxxxxxxxxxxxxx> writes:
>
> > On 10/24/2022 10:36 AM, Alistair Popple wrote:
> > > Baolin Wang <baolin.wang@xxxxxxxxxxxxxxxxx> writes:
> > >
> > > > When THP migration, if THPs are split and all subpages are migrated successfully
> > > > , the migrate_pages() will still return the number of THP that were not migrated.
> > > > That will confuse the callers of migrate_pages(), for example, which will make
> > > > the longterm pinning failed though all pages are migrated successfully.
> > > >
> > > > Thus we should return 0 to indicate all pages are migrated in this case.
> > > >
> > > > Signed-off-by: Baolin Wang <baolin.wang@xxxxxxxxxxxxxxxxx>
> > > > ---
> > > > Changes from v1:
> > > > - Fix the return value of migrate_pages() instead of fixing the
> > > >     callers' validation.
> > > > ---
> > > >    mm/migrate.c | 7 +++++++
> > > >    1 file changed, 7 insertions(+)
> > > >
> > > > diff --git a/mm/migrate.c b/mm/migrate.c
> > > > index 8e5eb6e..1da0dbc 100644
> > > > --- a/mm/migrate.c
> > > > +++ b/mm/migrate.c
> > > > @@ -1582,6 +1582,13 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
> > > >    	 */
> > > >    	list_splice(&ret_pages, from);
> > > >
> > > > +	/*
> > > > +	 * Return 0 in case all subpages of fail-to-migrate THPs are
> > > > +	 * migrated successfully.
> > > > +	 */
> > > > +	if (nr_thp_split && list_empty(from))
> > > > +		rc = 0;
> > > Why do you need to check nr_thp_split? Wouldn't list_empty(from) == True
> >
> > Only in the case of THP split, we can meet this abnormal case. So if no THP
> > split, just return the original 'rc' instead of validating the list, since the
> > 'nr_thp_split' validation is cheaper than the list_empty() validation IMHO.
>
> Is it really that much cheaper? We're already retrying migrations
> multiple times, etc. so surely the difference here would be marginal at
> best, and IMHO the code would be much clearer if we always set rc = 0
> when list_empty(from) = True.

Yeah, the difference is marginal and I have no strong preference. OK, 
will drop the 'nr_thp_split' in next version. Thanks.

> > > imply success? And if it doesn't imply success wouldn't it be possible
> > > to end up with nr_thp_split && list_empty(from) whilst still having
> > > pages that failed to migrate?
> > > The list management and return code logic from unmap_and_move() has
> > > gotten pretty difficult to follow and could do with some rework IMHO.
> >
> > Yes, Huang Ying has sent a RFC patchset[1] doing some code refactor, which seems
> > a good start.
>
> Thanks for pointing that out, I looked at it a while back but missed the
> clean ups. I was kind of waiting for the non-RFC version before taking
> another closer look.
>
> > [1] https://lore.kernel.org/all/20220921060616.73086-1-ying.huang@xxxxxxxxx/