Re: [External] Re: [PATCH v5 3/5] mm: hugetlb: fix a race between freeing and dissolving the page

[Muchun Song <songmuchun@bytedance.com>]

On Thu, Jan 14, 2021 at 9:20 PM Michal Hocko <mhocko@suse.com> wrote:
>
> On Thu 14-01-21 18:35:13, Muchun Song wrote:
> > There is a race condition between __free_huge_page()
> > and dissolve_free_huge_page().
> >
> > CPU0:                         CPU1:
> >
> > // page_count(page) == 1
> > put_page(page)
> >   __free_huge_page(page)
> >                               dissolve_free_huge_page(page)
> >                                 spin_lock(&hugetlb_lock)
> >                                 // PageHuge(page) && !page_count(page)
> >                                 update_and_free_page(page)
> >                                 // page is freed to the buddy
> >                                 spin_unlock(&hugetlb_lock)
> >     spin_lock(&hugetlb_lock)
> >     clear_page_huge_active(page)
> >     enqueue_huge_page(page)
> >     // It is wrong, the page is already freed
> >     spin_unlock(&hugetlb_lock)
> >
> > The race windows is between put_page() and dissolve_free_huge_page().
> >
> > We should make sure that the page is already on the free list
> > when it is dissolved.
>
> Please describe user visible effects as suggested in
> http://lkml.kernel.org/r/20210113093134.GU22493@dhcp22.suse.cz

Sorry forgot to update this.

>
> > Fixes: c8721bbbdd36 ("mm: memory-hotplug: enable memory hotplug to handle hugepage")
> > Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> > Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>
> > Cc: stable@vger.kernel.org
> > ---
> >  mm/hugetlb.c | 41 +++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 41 insertions(+)
> [...]
> > +retry:
> >       /* Not to disrupt normal path by vainly holding hugetlb_lock */
> >       if (!PageHuge(page))
> >               return 0;
> > @@ -1770,6 +1789,28 @@ int dissolve_free_huge_page(struct page *page)
> >               int nid = page_to_nid(head);
> >               if (h->free_huge_pages - h->resv_huge_pages == 0)
> >                       goto out;
> > +
> > +             /*
> > +              * We should make sure that the page is already on the free list
> > +              * when it is dissolved.
> > +              */
> > +             if (unlikely(!PageHugeFreed(head))) {
> > +                     spin_unlock(&hugetlb_lock);
> > +
> > +                     /*
> > +                      * Theoretically, we should return -EBUSY when we
> > +                      * encounter this race. In fact, we have a chance
> > +                      * to successfully dissolve the page if we do a
> > +                      * retry. Because the race window is quite small.
> > +                      * If we seize this opportunity, it is an optimization
> > +                      * for increasing the success rate of dissolving page.
> > +                      */
> > +                     while (PageHeadHuge(head) && !PageHugeFreed(head))
> > +                             cond_resched();
>
> Sorry, I should have raised that when replying to the previous version
> already but we have focused more on other things. Is there any special
> reason that you didn't simply
>         if (!PageHugeFreed(head)) {
>                 spin_unlock(&hugetlb_lock);
>                 cond_resched();
>                 goto retry;
>         }
>
> This would be less code and a very slight advantage would be that the
> waiter might get blocked on the spin lock while the concurrent freeing
> is happening. But maybe you wanted to avoid exactly this contention?
> Please put your thinking into the changelog.

I want to avoid the lock contention. I will add this reason
to the changelog. Thanks.

>
> > +
> > +                     goto retry;
> > +             }
> > +
> >               /*
> >                * Move PageHWPoison flag from head page to the raw error page,
> >                * which makes any subpages rather than the error page reusable.
> > --
> > 2.11.0
>
> --
> Michal Hocko
> SUSE Labs