Re: [PATCH v3 2/2] mm: Make alloc_contig_range handle in-use hugetlb pages

[Michal Hocko]

On Fri 26-02-21 11:24:29, Oscar Salvador wrote:
> On Fri, Feb 26, 2021 at 09:46:57AM +0100, Michal Hocko wrote:
> > On Mon 22-02-21 14:51:37, Oscar Salvador wrote:
> > [...]
> > > @@ -2394,9 +2397,19 @@ bool isolate_or_dissolve_huge_page(struct page *page)
> > >  	 */
> > >  	if (hstate_is_gigantic(h))
> > >  		return ret;
> > > -
> > > -	if (!page_count(head) && alloc_and_dissolve_huge_page(h, head))
> > > +retry:
> > > +	if (page_count(head) && isolate_huge_page(head, list)) {
> > >  		ret = true;
> > > +	} else if (!page_count(head)) {
> > 
> > This is rather head spinning. Do we need to test page_count in the else
> > branch? Do you want to optimize for a case where the page cannot be
> > isolated because of page_huge_active?
> 
> Well, I wanted to explictly call out both cases.
> We either 1) have an in-use page and we try to issolate it or 2) we have a free
> page (count == 0).
> 
> If the page could not be dissolved due to page_huge_active, this would either
> mean that page is about to be freed, or that someone has already issolated the
> page.
> Being the former case, one could say that falling-through alloc_and_dissolve is
> ok.
> 
> But no, I did not really want to optimize anything here, just wanted to be explicit
> about what we are checking and why.

Well, I will leave it to others. I do not feel strongly about this but
to me it makes the code harder to think about because the situation is
unstable and any of those condition can change as they are evaluated. So
an explicit checks makes the code harder in the end. I would simply got
with 
	if (isolate_huge_page(head, list) || !alloc_and_dissolve_huge_page())
		ret = true;

if either of the conditional needs a retry then it should be done
internally. Like alloc_and_dissolve_huge_page already does to stabilize
the PageFreed flag. An early bail out on non-free hugetlb page would
also better be done inside alloc_and_dissolve_huge_page.

-- 
Michal Hocko
SUSE Labs