Re: [External] Re: [PATCH 4/6] mm: hugetlb: add return -EAGAIN for dissolve_free_huge_page

[Muchun Song]

On Tue, Jan 5, 2021 at 2:38 PM HORIGUCHI NAOYA(堀口　直也)
<naoya.horiguchi@xxxxxxx> wrote:
>
> On Mon, Jan 04, 2021 at 02:58:41PM +0800, Muchun Song wrote:
> > When dissolve_free_huge_page() races with __free_huge_page(), we can
> > do a retry. Because the race window is small.
> >
> > Signed-off-by: Muchun Song <songmuchun@xxxxxxxxxxxxx>
> > ---
> >  mm/hugetlb.c | 16 +++++++++++-----
> >  1 file changed, 11 insertions(+), 5 deletions(-)
> >
> > diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> > index 72608008f8b4..db00ae375d2a 100644
> > --- a/mm/hugetlb.c
> > +++ b/mm/hugetlb.c
> > @@ -1763,10 +1763,11 @@ static int free_pool_huge_page(struct hstate *h, nodemask_t *nodes_allowed,
> >   * nothing for in-use hugepages and non-hugepages.
> >   * This function returns values like below:
> >   *
> > - *  -EBUSY: failed to dissolved free hugepages or the hugepage is in-use
> > - *          (allocated or reserved.)
> > - *       0: successfully dissolved free hugepages or the page is not a
> > - *          hugepage (considered as already dissolved)
> > + *  -EAGAIN: race with __free_huge_page() and can do a retry
> > + *  -EBUSY:  failed to dissolved free hugepages or the hugepage is in-use
> > + *           (allocated or reserved.)
> > + *       0:  successfully dissolved free hugepages or the page is not a
> > + *           hugepage (considered as already dissolved)
> >   */
> >  int dissolve_free_huge_page(struct page *page)
> >  {
> > @@ -1815,8 +1816,10 @@ int dissolve_free_huge_page(struct page *page)
> >                * We should make sure that the page is already on the free list
> >                * when it is dissolved.
> >                */
> > -             if (unlikely(!PageHugeFreed(head)))
> > +             if (unlikely(!PageHugeFreed(head))) {
> > +                     rc = -EAGAIN;
> >                       goto out;
> > +             }
>
> If dissolve_free_huge_page() races with __free_huge_page() and we detect
> it with this check, the hugepage is expected to be freed or dissolved by
> __free_huge_page(), so is it enough just to return with rc = 0 without retrying?

The dissolve_free_huge_page() aims to free the page to the buddy
allocator not the hugepage pool. So it is not enough just to return 0,
right? Or did you mean that we set the page temporary here and
let the __free_huge_page do the freeing later for us? Thanks.

>
> Thanks,
> Naoya Horiguchi
>
> >
> >               /*
> >                * Move PageHWPoison flag from head page to the raw error page,
> > @@ -1857,7 +1860,10 @@ int dissolve_free_huge_pages(unsigned long start_pfn, unsigned long end_pfn)
> >
> >       for (pfn = start_pfn; pfn < end_pfn; pfn += 1 << minimum_order) {
> >               page = pfn_to_page(pfn);
> > +retry:
> >               rc = dissolve_free_huge_page(page);
> > +             if (rc == -EAGAIN)
> > +                     goto retry;
> >               if (rc)
> >                       break;
> >       }
> > --
> > 2.11.0
> >