On Thu, Jul 15, 2021 at 1:39 AM Mike Kravetz [off-list ref] wrote:

On 7/14/21 3:57 AM, Muchun Song wrote:

quoted

On Sat, Jul 10, 2021 at 8:25 AM Mike Kravetz [off-list ref] wrote:

quoted

+       /*
+        * 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 <redacted>

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