Baolin Wang <baolin.wang@xxxxxxxxxxxxxxxxx> writes: > 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. Thanks. With that change feel free to add: Reviewed-by: Alistair Popple <apopple@xxxxxxxxxx> >>>> 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/
