Re: [PATCH 10/10] mm/hugetlb: not necessary to abuse temporary page to workaround the nasty free_huge_page

From: Mike Kravetz <mike.kravetz@oracle.com>
To: Baoquan He <bhe@redhat.com>,
	Wei Yang <richard.weiyang@linux.alibaba.com>,
	mhocko@kernel.org
Cc: akpm@linux-foundation.org, linux-mm@kvack.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 10/10] mm/hugetlb: not necessary to abuse temporary page to workaround the nasty free_huge_page
Date: Mon, 10 Aug 2020 17:19:06 -0700	[thread overview]
Message-ID: <129cc03e-c6d5-24f8-2f3c-f5a3cc821e76@oracle.com> (raw)
In-Reply-To: <20200810021737.GV14854@MiWiFi-R3L-srv>

On 8/9/20 7:17 PM, Baoquan He wrote:
> On 08/07/20 at 05:12pm, Wei Yang wrote:
>> Let's always increase surplus_huge_pages and so that free_huge_page
>> could decrease it at free time.
>>
>> Signed-off-by: Wei Yang <richard.weiyang@linux.alibaba.com>
>> ---
>>  mm/hugetlb.c | 14 ++++++--------
>>  1 file changed, 6 insertions(+), 8 deletions(-)
>>
>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
>> index 1f2010c9dd8d..a0eb81e0e4c5 100644
>> --- a/mm/hugetlb.c
>> +++ b/mm/hugetlb.c
>> @@ -1913,21 +1913,19 @@ static struct page *alloc_surplus_huge_page(struct hstate *h, gfp_t gfp_mask,
>>  		return NULL;
>>  
>>  	spin_lock(&hugetlb_lock);
>> +
>> +	h->surplus_huge_pages++;
>> +	h->surplus_huge_pages_node[page_to_nid(page)]++;
>> +
>>  	/*
>>  	 * We could have raced with the pool size change.
>>  	 * Double check that and simply deallocate the new page
>> -	 * if we would end up overcommiting the surpluses. Abuse
>> -	 * temporary page to workaround the nasty free_huge_page
>> -	 * codeflow
>> +	 * if we would end up overcommiting the surpluses.
>>  	 */
>> -	if (h->surplus_huge_pages >= h->nr_overcommit_huge_pages) {
>> -		SetPageHugeTemporary(page);
> 
> Hmm, the temporary page way is taken intentionally in
> commit 9980d744a0428 ("mm, hugetlb: get rid of surplus page accounting tricks").
> From code, this is done inside hugetlb_lock holding, and the code flow
> is straightforward, should be safe. Adding Michal to CC.
> 

I remember when the temporary page code was added for page migration.
The use of temporary page here was added at about the same time.  Temporary
page does have one advantage in that it will not CAUSE surplus count to
exceed overcommit.  This patch could cause surplus to exceed overcommit
for a very short period of time.  However, do note that for this to happen
the code needs to race with a pool resize which itself could cause surplus
to exceed overcommit.

IMO both approaches are valid.
- Advantage of temporary page is that it can not cause surplus to exceed
  overcommit.  Disadvantage is as mentioned in the comment 'abuse of temporary
  page'.
- Advantage of this patch is that it uses existing counters.  Disadvantage
  is that it can momentarily cause surplus to exceed overcommit.

Unless someone has a strong opinion, I prefer the changes in this patch.
-- 
Mike Kravetz