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

[David Hildenbrand <david@redhat.com>]

On 17.06.25 14:50, Oscar Salvador wrote:
> 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.

Yes. As an alternative, keep locking it in the caller and only unlock in 
the !anon case?

-- 
Cheers,

David / dhildenb