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

[Peter Xu <peterx@redhat.com>]

On Wed, Dec 07, 2022 at 03:05:42PM -0800, John Hubbard wrote:
> 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.
> + */

This is not true, I think?  It always releases all the locks.

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

I'm probably not gonna touch this part because it's not part of the patch..
For the rest, I can do.

I'll also apply the comment changes elsewhere if I don't speak up - in most
cases they all look good to me.

Thanks,

>  		 */
>  		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
> 

-- 
Peter Xu