Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20201103130334.13468-1-shy828301@gmail.com> <20201103130334.13468-6-shy828301@gmail.com>
 <A842D483-D9F2-4347-9A4F-1939769BC831@nvidia.com>
In-Reply-To: <A842D483-D9F2-4347-9A4F-1939769BC831@nvidia.com>
From:   Yang Shi <shy828301@gmail.com>
Date:   Fri, 6 Nov 2020 14:02:00 -0800
Message-ID: <CAHbLzkrXuvT5pMrJQDPwJtXWKK5LC53UoXRkx2-xkWfwX488wg@mail.gmail.com>
Subject: Re: [PATCH 5/5] mm: migrate: return -ENOSYS if THP migration is unsupported
To:     Zi Yan <ziy@nvidia.com>
Cc:     Michal Hocko <mhocko@suse.com>, Song Liu <songliubraving@fb.com>,
        Mel Gorman <mgorman@suse.de>, Jan Kara <jack@suse.cz>,
        Andrew Morton <akpm@linux-foundation.org>,
        Linux MM <linux-mm@kvack.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, Nov 6, 2020 at 12:17 PM Zi Yan <ziy@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 mak=
e
> > 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 <shy828301@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 =3D NULL;
> >
> >       if (!thp_migration_supported() && PageTransHuge(page))
> > -             return -ENOMEM;
> > +             return -ENOSYS;
> >
> >       if (page_count(page) =3D=3D 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 g=
et_new_page,
> >       return rc;
> >  }
> >
> > +static inline int try_split_thp(struct page *page, struct page *page2,
> > +                             struct list_head *from)
> > +{
> > +     int rc =3D 0;
> > +
> > +     lock_page(page);
> > +     rc =3D 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 =3D 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 her=
e.
>
> > +
> > +     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_p=
age_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, f=
rom)) {
> > +                                             nr_thp_split++;
> > +                                             goto retry;
> > +                                     }
> > +
> > +                                     nr_thp_failed++;
> > +                                     nr_failed +=3D 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 shoul=
d
> > -                              * retry on the same page with the THP sp=
lit
> > -                              * to base pages.
> > -                              *
> > -                              * Head page is retried immediately and t=
ail
> > -                              * pages are added to the tail of the lis=
t so
> > -                              * we encounter them after the rest of th=
e list
> > -                              * is processed.
> > +                              * When memory is low, don't bother to tr=
y to migrate
> > +                              * other pages, just exit.
>
> The comment does not match the code below. For THPs, the code tries to sp=
lit 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 !=3D
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 !=3D 0,
so it may be better to let it return nr_failed for -ENOMEM case too.

>
> >                                */
> >                               if (is_thp) {
> > -                                     lock_page(page);
> > -                                     rc =3D split_huge_page_to_list(pa=
ge, from);
> > -                                     unlock_page(page);
> > -                                     if (!rc) {
> > -                                             list_safe_reset_next(page=
, page2, lru);
> > +                                     if (!try_split_thp(page, page2, f=
rom)) {
> >                                               nr_thp_split++;
> >                                               goto retry;
> >                                       }
> > @@ -1490,7 +1520,7 @@ int migrate_pages(struct list_head *from, new_pag=
e_t get_new_page,
> >                               break;
> >                       default:
> >                               /*
> > -                              * Permanent failure (-EBUSY, -ENOSYS, et=
c.):
> > +                              * Permanent failure (-EBUSY, etc.):
> >                                * unlike -EAGAIN case, the failed page i=
s
> >                                * removed from migration page list and n=
ot
> >                                * retried in the next outer loop.
> > --
> > 2.26.2
>
>
> =E2=80=94
> Best Regards,
> Yan Zi