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/
