Re: [PATCH v7 4/7] mm,hugetlb: Split prep_new_huge_page functionality

[Mike Kravetz]

On 4/13/21 9:59 PM, Oscar Salvador wrote:
> On Tue, Apr 13, 2021 at 02:33:41PM -0700, Mike Kravetz wrote:
>>> -static void prep_new_huge_page(struct hstate *h, struct page *page, int nid)
>>> +/*
>>> + * Must be called with the hugetlb lock held
>>> + */
>>> +static void __prep_account_new_huge_page(struct hstate *h, int nid)
>>> +{
>>> +	h->nr_huge_pages++;
>>> +	h->nr_huge_pages_node[nid]++;
>>
>> I would prefer if we also move setting the destructor to this routine.
>> 	set_compound_page_dtor(page, HUGETLB_PAGE_DTOR);
> 
> Uhm, but that is the routine that does the accounting, it feels wrong
> here, plus...
> 
>>
>> That way, PageHuge() will be false until it 'really' is a huge page.
>> If not, we could potentially go into that retry loop in
>> dissolve_free_huge_page or alloc_and_dissolve_huge_page in patch 5.
> 
> ...I do not follow here, could you please elaborate some more?
> Unless I am missing something, behaviour should not be any different with this
> patch.
> 

I was thinking of the time between the call to __prep_new_huge_page and
__prep_account_new_huge_page.  In that time, PageHuge() will be true but
the page is not yet fully being managed as a hugetlb page.  My thought
was that isolation, migration, offline or any code that does pfn
scanning might the page as PageHuge() (after taking lock) and start to
process it.

Now I see that in patch 5 you call both __prep_new_huge_page and
__prep_account_new_huge_page with the lock held.  So, no issue.  Sorry.

I 'think' there may still be a potential race with the prep_new_huge_page
routine, but that existed before any of your changes.  It may also be
that 'synchronization' at the pageblock level which prevents some of
these pfn scanning operations to operate on the same pageblocks may
prevent this from ever happening.

Mostly thinking out loud.  Upon further thought, I have no objections to
this change.  Sorry for the noise.

Reviewed-by: Mike Kravetz <mike.kravetz@xxxxxxxxxx>
-- 
Mike Kravetz