lkml.org 
[lkml]   [2021]   [Jan]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [External] Re: [PATCH v5 3/5] mm: hugetlb: fix a race between freeing and dissolving the page
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

\
 
 \ /
  Last update: 2021-01-14 14:52    [W:0.309 / U:0.020 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site