Re: [PATCH v4 4/8] hugetlb: create remove_hugetlb_page() to separate functionality

[Mike Kravetz <mike.kravetz@oracle.com>]

On 4/6/21 6:41 AM, Oscar Salvador wrote:
> On Mon, Apr 05, 2021 at 04:00:39PM -0700, Mike Kravetz wrote:
>> +static void remove_hugetlb_page(struct hstate *h, struct page *page,
>> +							bool adjust_surplus)
>> +{
>> +	int nid = page_to_nid(page);
>> +
>> +	if (hstate_is_gigantic(h) && !gigantic_page_runtime_supported())
>> +		return;
>> +
>> +	list_del(&page->lru);
>> +
>> +	if (HPageFreed(page)) {
>> +		h->free_huge_pages--;
>> +		h->free_huge_pages_node[nid]--;
>> +		ClearHPageFreed(page);
>> +	}
>> +	if (adjust_surplus) {
>> +		h->surplus_huge_pages--;
>> +		h->surplus_huge_pages_node[nid]--;
>> +	}
>> +
>> +	VM_BUG_ON_PAGE(hugetlb_cgroup_from_page(page), page);
>> +	VM_BUG_ON_PAGE(hugetlb_cgroup_from_page_rsvd(page), page);
> 
> These checks feel a bit odd here.
> I would move them above, before we start messing with the counters?
> 

This routine is comprised of code that was previously in update_and_free_page
and __free_huge_page.   In those routines, the VM_BUG_ON_PAGE came after the
counter adjustments.  That is the only reason they are positioned as they are.

I agree that it makes more sense to add them to the beginning of the routine.

>> +
>> +	ClearHPageTemporary(page);
> 
> Why clearing it unconditionally? I guess we do not really care, but
> someone might wonder why when reading the core.
> So I would either do as we used to do and only clear it in case of
> HPageTemporary(), or drop a comment.
> 

Technically, the HPage* flags are meaningless after calling this
routine.  So, there really is no need to modify them at all.  The
flag clearing code is left over from the routines in which they
previously existed.

Any clearing of HPage* flags in this routine is unnecessary and should
be removed to avoid any questions.
-- 
Mike Kravetz