Hi Mike,
Mike Kravetz <mike.kravetz@xxxxxxxxxx> writes:
> On 04/14/23 21:12, Matthew Wilcox wrote:
>> On Sat, Apr 15, 2023 at 01:18:32AM +0530, Tarun Sahu wrote:
>> > folio_set_order(folio, 0); which is an abuse of folio_set_order as 0-order
>> > folio does not have any tail page to set order.
>>
>> I think you're missing the point of how folio_set_order() is used.
>> When splitting a large folio, we need to zero out the folio_nr_pages
>> in the tail, so it does have a tail page, and that tail page needs to
>> be zeroed. We even assert that there is a tail page:
>>
>> if (WARN_ON_ONCE(!folio_test_large(folio)))
>> return;
>>
>> Or maybe you need to explain yourself better.
>>
>> > folio->_folio_nr_pages is
>> > set to 0 for order 0 in folio_set_order. It is required because
>> > _folio_nr_pages overlapped with page->mapping and leaving it non zero
>> > caused "bad page" error while freeing gigantic hugepages. This was fixed in
>> > Commit ba9c1201beaa ("mm/hugetlb: clear compound_nr before freeing gigantic
>> > pages"). Also commit a01f43901cfb ("hugetlb: be sure to free demoted CMA
>> > pages to CMA") now explicitly clear page->mapping and hence we won't see
>> > the bad page error even if _folio_nr_pages remains unset. Also the order 0
>> > folios are not supposed to call folio_set_order, So now we can get rid of
>> > folio_set_order(folio, 0) from hugetlb code path to clear the confusion.
>>
>> ... this is all very confusing.
>>
>> > The patch also moves _folio_set_head and folio_set_order calls in
>> > __prep_compound_gigantic_folio() such that we avoid clearing them in the
>> > error path.
>>
>> But don't we need those bits set while we operate on the folio to set it
>> up? It makes me nervous if we don't have those bits set because we can
>> end up with speculative references that point to a head page while that
>> page is not marked as a head page. It may not be a problem, but I want
>> to see some air-tight analysis of that.
>
> I am fairly certain we are 'safe'. Here is code before setting up the
> pointer to the head page.
>
> * In the case of demote, the ref count will be zero.
> */
> if (!demote) {
> if (!page_ref_freeze(p, 1)) {
> pr_warn("HugeTLB page can not be used due to unexpected inflated ref count\n");
> goto out_error;
> }
> } else {
> VM_BUG_ON_PAGE(page_count(p), p);
> }
> if (i != 0)
> set_compound_head(p, &folio->page);
>
> So, before setting the pointer to head page ref count will be zero.
>
> I 'think' it would actually be better to move the calls to _folio_set_head and
> folio_set_order in __prep_compound_gigantic_folio() as suggested here. Why?
> In the current code, the ref count on the 'head page' is still 1 (or more)
> while those calls are made. So, someone could take a speculative ref on the
> page BEFORE the tail pages are set up.
>
Thanks, for confirming the correctness of moving these calls. Also I
didn't look at it this way while moving them. Thanks for the comment.
I will update the commit msg and send the v2.
~Tarun
> TBH, I do not have much of an opinion about potential confusion surrounding
> folio_set_compound_order(folio, 0). IIUC, hugetlb gigantic page setup is the
> only place outside the page allocation code that sets up compound pages/large
> folios. So, it is going to be a bit 'special'. As mentioned, when this was
> originally discussed I suggested folio_clear_order(). I would be happy with
> either.
> --
> Mike Kravetz
