Re: [PATCH v2 2/5] mm,hugetlb: Sort out folio locking in the faulting path

[Oscar Salvador <osalvador@suse.de>]

On Wed, Jun 25, 2025 at 09:47:00AM +0200, Oscar Salvador wrote:
> On Mon, Jun 23, 2025 at 04:11:38PM +0200, David Hildenbrand wrote:
> > > @@ -6801,13 +6810,24 @@ vm_fault_t hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma,
> > >   		/* Fallthrough to CoW */
> > >   	}
> > > -	/* hugetlb_wp() requires page locks of pte_page(vmf.orig_pte) */
> > > -	folio = page_folio(pte_page(vmf.orig_pte));
> > > -	folio_lock(folio);
> > > -	folio_get(folio);
> > > -
> > >   	if (flags & (FAULT_FLAG_WRITE|FAULT_FLAG_UNSHARE)) {
> > >   		if (!huge_pte_write(vmf.orig_pte)) {
> > > +			/*
> > > +			 * Anonymous folios need to be lock since hugetlb_wp()
> > > +			 * checks whether we can re-use the folio exclusively
> > > +			 * for us in case we are the only user of it.
> > > +			 */
> > 
> > Should we move that comment to hugetlb_wp() instead? And if we are already
> > doing this PTL unlock dance now, why not do it in hugetlb_wp() instead so we
> > can simplify this code?
> 
> Yes, probably we can move it further up.
> Let me see how it would look.

So, I've been thinking about this, and I'm not so sure.
By default, the state of the folio in hugetlb_no_page and hugetlb_fault is
different.

hugetlb_no_page() has the folio locked already, and hugetlb_fault() hasn't, which
means that if we want to move this further up, 1) hugetlb_no_page() would have to
unlock the folio to then lock it in hugetlb_wp() in case it's anonymous or
2) pass a parameter to hugetlb_wp() to let it know whether the folio is already locked.

Don't really like any of them. Case 1) seems suboptimal as right now (with this patch)
we only unlock the folio in !anon case in hugetlb_no_page(). If we want to move the 'dance'
from hugetlb_fault() to hugetlb_wp(), we'd have to unlock and then lock it again.

-- 
Oscar Salvador
SUSE Labs