Re: [PATCH v3 04/12] mm/khugepaged: add struct collapse_result

[Peter Xu <peterx@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1293 bytes --]

On Tue, Apr 26, 2022 at 07:44:04AM -0700, Zach O'Keefe wrote:
> +/* Gather information from one khugepaged_scan_[pmd|file]() request */
> +struct collapse_result {
> +	enum scan_result result;
> +
> +	/* Was mmap_lock dropped during request? */
> +	bool dropped_mmap_lock;
> +};

IMHO this new dropped_mmap_lock makes things a bit more complicated..

To me, the old code actually is easy to read on when the lock is dropped:

  - For file, we always drop it
  - For anon, when khugepaged_scan_pmd() returns 1

That's fairly clear to me.

But I understand what you need, probably the "result" hidden in the old
world but you'd need that for MADV_COLLAPSE?

I also noticed major chunks of this patch is converting "result = XXX" into
"cr->result = XXX".  IMHO that's really not necessary for touching up all
the places around.

I'm also wondering whether we could simply pass over the result into your
newly introduced collapse_control*.

In summary, this is what's in my mind..

  - Add collapse_control.result
  - Make sure both khugepaged_scan_file() and khugepaged_scan_pmd() to
    simply remember to update cc->result before it returns

I've attached a pesudo code patch (note!  it's pesudo not even compile but
to show what I meant..), so far I don't see a problem with it.

-- 
Peter Xu

[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 2507 bytes --]

diff --git a/mm/khugepaged.c b/mm/khugepaged.c
index d7c5bb9fd1fb..bd341115d359 100644
--- a/mm/khugepaged.c
+++ b/mm/khugepaged.c
@@ -1040,7 +1040,7 @@ static bool __collapse_huge_page_swapin(struct mm_struct *mm,
 	return true;
 }
 
-static void collapse_huge_page(struct mm_struct *mm,
+static int collapse_huge_page(struct mm_struct *mm,
 				   unsigned long address,
 				   struct page **hpage,
 				   int node, int referenced, int unmapped)
@@ -1208,7 +1208,7 @@ static void collapse_huge_page(struct mm_struct *mm,
 	if (!IS_ERR_OR_NULL(*hpage))
 		mem_cgroup_uncharge(page_folio(*hpage));
 	trace_mm_collapse_huge_page(mm, isolated, result);
-	return;
+	return result;
 }
 
 static int khugepaged_scan_pmd(struct mm_struct *mm,
@@ -1361,13 +1361,19 @@ static int khugepaged_scan_pmd(struct mm_struct *mm,
 	pte_unmap_unlock(pte, ptl);
 	if (ret) {
 		node = khugepaged_find_target_node();
-		/* collapse_huge_page will return with the mmap_lock released */
-		collapse_huge_page(mm, address, hpage, node,
-				referenced, unmapped);
+		/*
+		 * collapse_huge_page will return with the mmap_lock
+		 * released.  It'll also further update cc->result if
+		 * anything wrong happens.
+		 */
+		result = collapse_huge_page(mm, address, hpage, node,
+					    referenced, unmapped, cc);
 	}
 out:
 	trace_mm_khugepaged_scan_pmd(mm, page, writable, referenced,
 				     none_or_zero, result, unmapped);
+	/* Deliver the result to the caller */
+	cc->result = result;
 	return ret;
 }
 
@@ -1646,7 +1652,7 @@ static void retract_page_tables(struct address_space *mapping, pgoff_t pgoff)
  *    + restore gaps in the page cache;
  *    + unlock and free huge page;
  */
-static void collapse_file(struct mm_struct *mm,
+static int collapse_file(struct mm_struct *mm,
 		struct file *file, pgoff_t start,
 		struct page **hpage, int node)
 {
@@ -2009,6 +2015,7 @@ static void collapse_file(struct mm_struct *mm,
 	if (!IS_ERR_OR_NULL(*hpage))
 		mem_cgroup_uncharge(page_folio(*hpage));
 	/* TODO: tracepoints */
+	return result;
 }
 
 static void khugepaged_scan_file(struct mm_struct *mm,
@@ -2086,11 +2093,12 @@ static void khugepaged_scan_file(struct mm_struct *mm,
 			count_vm_event(THP_SCAN_EXCEED_NONE_PTE);
 		} else {
 			node = khugepaged_find_target_node();
-			collapse_file(mm, file, start, hpage, node);
+			result = collapse_file(mm, file, start, hpage, node);
 		}
 	}
 
 	/* TODO: tracepoints */
+	cc->result = result;
 }
 #else
 static void khugepaged_scan_file(struct mm_struct *mm,