Re: [PATCH v2 04/10] mm/hugetlb: Move swap entry handling into vma lock when faulted

[John Hubbard <jhubbard@nvidia.com>]

On 12/7/22 14:43, Peter Xu wrote:
> Note that here migration_entry_wait_huge() will release it.
> 
> Sorry it's definitely not as straightforward, but this is also something I
> didn't come up with a better solution, because we need the vma lock to
> protect the spinlock, which is used in deep code path of the migration
> code.
> 
> That's also why I added a rich comment above, and there's "The vma lock
> will be released there" which is just for that.
> 

Yes, OK,

Reviewed-by: John Hubbard <jhubbard@nvidia.com>

...and here is some fancy documentation polishing (incremental on top of this
specific patch) if you would like to fold it in, optional but it makes me
happier:


diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 49f73677a418..e3bbd4869f68 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -5809,6 +5809,10 @@ u32 hugetlb_fault_mutex_hash(struct address_space *mapping, pgoff_t idx)
  }
  #endif
  
+/*
+ * There are a few special cases in which this function returns while still
+ * holding locks. Those are noted inline.
+ */
  vm_fault_t hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma,
  			unsigned long address, unsigned int flags)
  {
@@ -5851,8 +5855,8 @@ vm_fault_t hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma,
  	/* PTE markers should be handled the same way as none pte */
  	if (huge_pte_none_mostly(entry))
  		/*
-		 * hugetlb_no_page will drop vma lock and hugetlb fault
-		 * mutex internally, which make us return immediately.
+		 * hugetlb_no_page() will release both the vma lock and the
+		 * hugetlb fault mutex, so just return directly from that:
  		 */
  		return hugetlb_no_page(mm, vma, mapping, idx, address, ptep,
  				      entry, flags);
@@ -5869,10 +5873,11 @@ vm_fault_t hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma,
  	if (!pte_present(entry)) {
  		if (unlikely(is_hugetlb_entry_migration(entry))) {
  			/*
-			 * Release fault lock first because the vma lock is
-			 * needed to guard the huge_pte_lockptr() later in
-			 * migration_entry_wait_huge().  The vma lock will
-			 * be released there.
+			 * Release the hugetlb fault lock now, but retain the
+			 * vma lock, because it is needed to guard the
+			 * huge_pte_lockptr() later in
+			 * migration_entry_wait_huge(). The vma lock will be
+			 * released there.
  			 */
  			mutex_unlock(&hugetlb_fault_mutex_table[hash]);
  			migration_entry_wait_huge(vma, ptep);
diff --git a/mm/migrate.c b/mm/migrate.c
index d14f1f3ab073..a31df628b938 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -333,16 +333,18 @@ void migration_entry_wait(struct mm_struct *mm, pmd_t *pmd,
  }
  
  #ifdef CONFIG_HUGETLB_PAGE
+
+/*
+ * The vma read lock must be held upon entry. Holding that lock prevents either
+ * the pte or the ptl from being freed.
+ *
+ * This function will release the vma lock before returning.
+ */
  void __migration_entry_wait_huge(struct vm_area_struct *vma,
  				 pte_t *ptep, spinlock_t *ptl)
  {
  	pte_t pte;
  
-	/*
-	 * The vma read lock must be taken, which will be released before
-	 * the function returns.  It makes sure the pgtable page (along
-	 * with its spin lock) not be freed in parallel.
-	 */
  	hugetlb_vma_assert_locked(vma);
  
  	spin_lock(ptl);


thanks,
-- 
John Hubbard
NVIDIA