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

[Mike Kravetz]

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.

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.

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