On 05/15/23 18:16, Matthew Wilcox wrote:
> On Mon, May 15, 2023 at 10:38:09PM +0530, Tarun Sahu wrote:
> > @@ -1951,9 +1950,6 @@ static bool __prep_compound_gigantic_folio(struct folio *folio,
> > struct page *p;
> >
> > __folio_clear_reserved(folio);
> > - __folio_set_head(folio);
> > - /* we rely on prep_new_hugetlb_folio to set the destructor */
> > - folio_set_order(folio, order);
> > for (i = 0; i < nr_pages; i++) {
> > p = folio_page(folio, i);
> >
> > @@ -1999,6 +1995,9 @@ static bool __prep_compound_gigantic_folio(struct folio *folio,
> > if (i != 0)
> > set_compound_head(p, &folio->page);
> > }
> > + __folio_set_head(folio);
> > + /* we rely on prep_new_hugetlb_folio to set the destructor */
> > + folio_set_order(folio, order);
>
> This makes me nervous, as I said before. This means that
> compound_head(tail) can temporarily point to a page which is not marked
> as a head page. That's different from prep_compound_page(). You need to
> come up with some good argumentation for why this is safe, and no amount
> of testing you do can replace it -- any race in this area will be subtle.
I added comments supporting this approach in the first version of the patch.
My argument was that this is actually safer than the existing code. That is
because we freeze the page (ref count 0) before setting compound_head(tail).
So, nobody should be taking any speculative refs on those tail pages.
In the existing code, we set the compound page order in the head before
freezing the head or any tail pages. Therefore, speculative refs can be
taken on any of the pages while in this state.
If we want prep_compound_gigantic_folio to work like prep_compound_page
we would need to take two passes through the pages. In the first pass,
freeze all the pages and in the second set up the compound page.
--
Mike Kravetz
