On Fri, Nov 6, 2020 at 12:17 PM Zi Yan <z...@nvidia.com> wrote:
>
> On 3 Nov 2020, at 8:03, Yang Shi wrote:
>
> > In the current implementation unmap_and_move() would return -ENOMEM if
> > THP migration is unsupported, then the THP will be split. If split is
> > failed just exit without trying to migrate other pages. It doesn't make
> > too much sense since there may be enough free memory to migrate other
> > pages and there may be a lot base pages on the list.
> >
> > Return -ENOSYS to make consistent with hugetlb. And if THP split is
> > failed just skip and try other pages on the list.
> >
> > Just skip the whole list and exit when free memory is really low.
> >
> > Signed-off-by: Yang Shi <shy828...@gmail.com>
> > ---
> > mm/migrate.c | 62 ++++++++++++++++++++++++++++++++++++++--------------
> > 1 file changed, 46 insertions(+), 16 deletions(-)
> >
> > diff --git a/mm/migrate.c b/mm/migrate.c
> > index 8f6a61c9274b..b3466d8c7f03 100644
> > --- a/mm/migrate.c
> > +++ b/mm/migrate.c
> > @@ -1172,7 +1172,7 @@ static int unmap_and_move(new_page_t get_new_page,
> > struct page *newpage = NULL;
> >
> > if (!thp_migration_supported() && PageTransHuge(page))
> > - return -ENOMEM;
> > + return -ENOSYS;
> >
> > if (page_count(page) == 1) {
> > /* page was freed from under us. So we are done. */
> > @@ -1376,6 +1376,20 @@ static int unmap_and_move_huge_page(new_page_t
> > get_new_page,
> > return rc;
> > }
> >
> > +static inline int try_split_thp(struct page *page, struct page *page2,
> > + struct list_head *from)
> > +{
> > + int rc = 0;
> > +
> > + lock_page(page);
> > + rc = split_huge_page_to_list(page, from);
> > + unlock_page(page);
> > + if (!rc)
> > + list_safe_reset_next(page, page2, lru);
>
> This does not work as expected, right? After macro expansion, we have
> page2 = list_next_entry(page, lru). Since page2 is passed as a pointer, the
> change
> does not return back the caller. You need to use the pointer to page2 here.
>
> > +
> > + return rc;
> > +}
> > +
> > /*
> > * migrate_pages - migrate the pages specified in a list, to the free pages
> > * supplied as the target for the page migration
> > @@ -1445,24 +1459,40 @@ int migrate_pages(struct list_head *from,
> > new_page_t get_new_page,
> > reason, &ret_pages);
> >
> > switch(rc) {
> > + /*
> > + * THP migration might be unsupported or the
> > + * allocation could've failed so we should
> > + * retry on the same page with the THP split
> > + * to base pages.
> > + *
> > + * Head page is retried immediately and tail
> > + * pages are added to the tail of the list so
> > + * we encounter them after the rest of the list
> > + * is processed.
> > + */
> > + case -ENOSYS:
> > + /* THP migration is unsupported */
> > + if (is_thp) {
> > + if (!try_split_thp(page, page2,
> > from)) {
> > + nr_thp_split++;
> > + goto retry;
> > + }
> > +
> > + nr_thp_failed++;
> > + nr_failed += nr_subpages;
> > + break;
> > + }
> > +
> > + /* Hugetlb migration is unsupported */
> > + nr_failed++;
> > + break;
> > case -ENOMEM:
> > /*
> > - * THP migration might be unsupported or the
> > - * allocation could've failed so we should
> > - * retry on the same page with the THP split
> > - * to base pages.
> > - *
> > - * Head page is retried immediately and tail
> > - * pages are added to the tail of the list so
> > - * we encounter them after the rest of the
> > list
> > - * is processed.
> > + * When memory is low, don't bother to try to
> > migrate
> > + * other pages, just exit.
>
> The comment does not match the code below. For THPs, the code tries to split
> the THP
> and migrate the base pages if the split is successful.
BTW, actually I don't think -ENOSYS is a proper return value for this
case since it typically means "the syscall doesn't exist". IMHO, it
should return -EINVAL. And actually the return value doesn't matter
since all callers of migrate_pages() just check if the return value !=
0. And, neither -ENOSYS nor -EINVAL won't be visible by userspace
since migrate_pages() just returns the number of failed pages for this
case.
Anyway the return value is a little bit messy, it may return -ENOMEM,
0 or nr_failed. But it looks the callers just care about if ret != 0,
so it may be better to let it return nr_failed for -ENOMEM case too.
>
> > */
> > if (is_thp) {
> > - lock_page(page);
> > - rc = split_huge_page_to_list(page,
> > from);
> > - unlock_page(page);
> > - if (!rc) {
> > - list_safe_reset_next(page,
> > page2, lru);
> > + if (!try_split_thp(page, page2,
> > from)) {
> > nr_thp_split++;
> > goto retry;
> > }
> > @@ -1490,7 +1520,7 @@ int migrate_pages(struct list_head *from, new_page_t
> > get_new_page,
> > break;
> > default:
> > /*
> > - * Permanent failure (-EBUSY, -ENOSYS, etc.):
> > + * Permanent failure (-EBUSY, etc.):
> > * unlike -EAGAIN case, the failed page is
> > * removed from migration page list and not
> > * retried in the next outer loop.
> > --
> > 2.26.2
>
>
> —
> Best Regards,
> Yan Zi
