Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Tarun Sahu <tsahu@linux.ibm.com>
To:     Matthew Wilcox <willy@infradead.org>
Cc:     linux-mm@kvack.org, akpm@linux-foundation.org,
        muchun.song@linux.dev, mike.kravetz@oracle.com,
        aneesh.kumar@linux.ibm.com, sidhartha.kumar@oracle.com,
        gerald.schaefer@linux.ibm.com, linux-kernel@vger.kernel.org,
        jaypatel@linux.ibm.com
Subject: Re: [PATCH v2] mm/folio: Avoid special handling for order value 0
 in folio_set_order
In-Reply-To: <ZGJo4UhdyakGFwP7@casper.infradead.org>
References: <20230515170809.284680-1-tsahu@linux.ibm.com>
 <ZGJo4UhdyakGFwP7@casper.infradead.org>
Date:   Tue, 16 May 2023 18:39:11 +0530
Message-ID: <87fs7wqv7s.fsf@linux.ibm.com>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Hi Mathew,

Matthew Wilcox <willy@infradead.org> writes:

> 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.

IIUC, I am certain that it is safe to move these calls and agree with what
Mike said. Here is my reasoning:

When we get pages from CMA allocator for gigantic folio, page refcount
for each pages is 1.
page_cache_get_speculative (now folio_try_get_rcu) can take reference to
any of these pages before prep_compound_gigantic_folio explicitly freeze
refcount of these pages. With this race condition there are 2 possible situation.

...
		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);
	}
...

1. In the current code, before freezing refcount of nth (hence, n+th)
tail page, folio_try_get_rcu might try to take nth tail page reference,
so refcount will be increased of the nth tail page not the head page
(as compound head is not yet set for nth tail page). and once this
happens, nth iteration of loop will cause error and
prep_compound_gigantic_folio will fail.

So, setting the PG_head at the starting of for-loop or at the end won't
have any difference to this flow.

2. If reference for the head page is taken by folio_try_get_rcu before
freezing it, prep_compound_gigantic_page will fail, but before PG_head
and folio_order of head page is cleared in error path, the caller of
folio_try_get_rcu path will find that this page is head page and might
try to operate on its tail pages while these tail pages are invalid.

Hence, It will be safer if we call __folio_set_head and folio_set_order
after freezing the tail page refcount.

~Tarun