Re: [PATCH 2/5] mm,hugetlb: Document the reason to lock the folio in the faulting path

[Oscar Salvador <osalvador@suse.de>]

On Tue, Jun 17, 2025 at 02:10:09PM +0200, Oscar Salvador wrote:
> On Tue, Jun 17, 2025 at 02:08:16PM +0200, David Hildenbrand wrote:
> > folio lock is a sleeping lock, PTL is a spinlock. :)
> 
> Lol yes, overlooked that totally.
> And I also saw the comment from mm/rmap.c about lockin order.

So, we could do something like this:

        if (folio_test_anon(old_folio)) {
                spin_unlock(vmf->ptl);
                folio_lock(old_folio);
                spin_lock(vmf->ptl);
                vmf->pte = hugetlb_walk(vma, vmf->address, huge_page_size(h));
                if (unlikely(!vmf->pte ||
                    !pte_same(huge_ptep_get(mm, vmf->address, vmf->pte), pte))) {
                        spin_unlock(vmf->ptl);
                        folio_unlock(old_folio);
                        goto out_take_lock;
                }

                if (folio_mapcount(old_folio == 1)) {
                        if (!PageAnonExclusive(&old_folio->page)) {
                                folio_move_anon_rmap(old_folio, vma);
                                SetPageAnonExclusive(&old_folio->page);
                        }
                        if (likely(!unshare))
                                set_huge_ptep_maybe_writable(vma, vmf->address,
                                                             vmf->pte);

                        spin_unlock(vmf->ptl);
                        folio_unlock(old_folio);
                        goto out_take_lock;
                }
                VM_BUG_ON_PAGE(folio_test_anon(old_folio) &&
                               PageAnonExclusive(&old_folio->page), &old_folio->page);
                spin_unlock(vmf->ptl);
                folio_unlock(old_folio);
                spin_lock(vmf->ptl);
                vmf->pte = hugetlb_walk(vma, vmf->address, huge_page_size(h));
                if (unlikely(!vmf->pte ||
                   !pte_same(huge_ptep_get(mm, vmf->address, vmf->pte), pte)))
                        return 0;
        }

Hopefully we can do some refactor here, because I quite dislike the
unlock-lock-retake-unlock-blah cycle.



-- 
Oscar Salvador
SUSE Labs