Re: [External] [PATCH 3/3] hugetlb: before freeing hugetlb page set dtor to appropriate value

[Muchun Song]

On Thu, Jul 15, 2021 at 1:39 AM Mike Kravetz <mike.kravetz@xxxxxxxxxx> wrote:
>
> On 7/14/21 3:57 AM, Muchun Song wrote:
> > On Sat, Jul 10, 2021 at 8:25 AM Mike Kravetz <mike.kravetz@xxxxxxxxxx> wrote:
> >> +       /*
> >> +        * Very subtle
> >> +        *
> >> +        * For non-gigantic pages set the destructor to the normal compound
> >> +        * page dtor.  This is needed in case someone takes an additional
> >> +        * temporary ref to the page, and freeing is delayed until they drop
> >> +        * their reference.
> >> +        *
> >> +        * For gigantic pages set the destructor to the null dtor.  This
> >> +        * destructor will never be called.  Before freeing the gigantic
> >> +        * page destroy_compound_gigantic_page will turn the compound page
> >> +        * into a simple group of pages.  After this the destructor does not
> >> +        * apply.
> >> +        *
> >> +        * This handles the case where more than one ref is held when and
> >> +        * after update_and_free_page is called.
> >> +        */
> >>         set_page_refcounted(page);
> >> -       set_compound_page_dtor(page, NULL_COMPOUND_DTOR);
> >> +       if (hstate_is_gigantic(h))
> >> +               set_compound_page_dtor(page, NULL_COMPOUND_DTOR);
> >> +       else
> >> +               set_compound_page_dtor(page, COMPOUND_PAGE_DTOR);
> >
> > Hi Mike,
> >
> > The race is really subtle. But we also should remove the WARN from
> > free_contig_range, right? Because the refcount of the head page of
> > the gigantic page can be greater than one, but free_contig_range has
> > the following warning.
> >
> > WARN(count != 0, "%lu pages are still in use!\n", count);
> >
>
> I did hit that warning in my testing and thought about removing it.
> However, I decided to keep it because non-hugetlb code also makes use of
> alloc_contig_range/free_contig_range and it might be useful in those
> cases.
>
> My 'guess' is that the warning was added not because of temporary ref
> count increases but rather to point out any code that forgot to drop a
> reference.

Got it. At least this patch looks good to me. So

Reviewed-by: Muchun Song <songmuchun@xxxxxxxxxxxxx>

>
> BTW - It is not just the 'head' page which could trigger this warning, but
> any 'tail' page as well.  That is because we do not call free_contig_range
> with a compound page, but rather a group of pages all with ref count of
> at least one.

Right.

>
> I'm happy to remove the warning if people do not think it is generally
> useful.

For me, I suggest removing it. If someone has any ideas, please
let us know.

> --
> Mike Kravetz