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