On Fri 26-02-21 11:24:29, Oscar Salvador wrote:

On Fri, Feb 26, 2021 at 09:46:57AM +0100, Michal Hocko wrote:

quoted

On Mon 22-02-21 14:51:37, Oscar Salvador wrote:
[...]

quoted

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