Re: [RFC PATCH 1/3] mm, hugetlb: Clean up locking in hugetlb_fault and hugetlb_wp

Peter Xu
2025-06-02T21:30:26Z

On Mon, Jun 02, 2025 at 10:47:04PM +0200, Oscar Salvador wrote:
> On Mon, Jun 02, 2025 at 11:14:26AM -0400, Peter Xu wrote:
> > It makes me feel nervous when we move more thing over to fault mutex - I
> > had a feeling it's abused.
> >
> > IIRC the fault mutex was inintially introduced only to solve one problem on
> > concurrent allocations. I confess I didn't follow or yet dig into all
> > history, though. From that POV, mfill_atomic_hugetlb() is indeed
> > reasonable to still take it because it's user-guided fault injections. I'm
> > not yet sure about other places, e.g., for truncations.
>
> Hi Peter,
>
> taking the mutex for truncation/punching hole was added back in:
>
> commit b5cec28d36f5ee6b4e6f68a0a40aa1e4045d6d99
> Author: Mike Kravetz <mike.kravetz@oracle.com>
> Date: Tue Sep 8 15:01:41 2015 -0700
>
> hugetlbfs: truncate_hugepages() takes a range of pages
>
> when the fallocate functionality and the punching-mode were introduced.
> E.g:
>
> commit c672c7f29f2fdb73e1f72911bf499675c81fcdbb
> Author: Mike Kravetz <mike.kravetz@oracle.com>
> Date: Tue Sep 8 15:01:35 2015 -0700
>
> mm/hugetlb: expose hugetlb fault mutex for use by fallocate
>
> hugetlb page faults are currently synchronized by the table of mutexes
> (htlb_fault_mutex_table). fallocate code will need to synchronize with
> the page fault code when it allocates or deletes pages. Expose
> interfaces so that fallocate operations can be synchronized with page
> faults. Minor name changes to be more consistent with other global
> hugetlb symbols.
>
> Sadly, I cannot really see why it was added.
> It was introduced from RFC v2 -> RFC v3:
>
> RFC v3:
> Folded in patch for alloc_huge_page/hugetlb_reserve_pages race
> in current code
> fallocate allocation and hole punch is synchronized with page
> faults via existing mutex table
> hole punch uses existing hugetlb_vmtruncate_list instead of more
> generic unmap_mapping_range for unmapping
> Error handling for the case when region_del() fauils
>
> But RFC v2 shows no discussions in that matter whatsoever, so go figure
> [1].
> I read that some tests were written, so I am not sure whether any of
> those tests caught a race.
>
> Looking at remove_inode_single_folio, there is a case though:
>
> /*
> * If folio is mapped, it was faulted in after being
> * unmapped in caller. Unmap (again) while holding
> * the fault mutex. The mutex will prevent faults
> * until we finish removing the folio.
> */
> if (unlikely(folio_mapped(folio)))
> hugetlb_unmap_file_folio(h, mapping, folio, index);
>
> So, while the folio_lock that is taken further down guards us against
> removing the page from the pagecache, I guess that we need to take the
> mutex to guard us against parallel faults for the page.
>
> Looking back at earlier code, we did not handle that race.
>
> It was reworked in
>
> commit 4aae8d1c051ea00b456da6811bc36d1f69de5445
> Author: Mike Kravetz <mike.kravetz@oracle.com>
> Date: Fri Jan 15 16:57:40 2016 -0800
>
> mm/hugetlbfs: unmap pages if page fault raced with hole punch
>
> and the comment looked like:
>
> /*
> * If page is mapped, it was faulted in after being
> * unmapped in caller. Unmap (again) now after taking
> * the fault mutex. The mutex will prevent faults
> * until we finish removing the page.
> *
> * This race can only happen in the hole punch case.
> * Getting here in a truncate operation is a bug.
> */
>
> So, it was placed to close the race.
> Now, I do not know whether we could close that race somewhat different,
> but it seems risky, and having the mutex here seems like a good
> trade-off.

Right, and thanks for the git digging as usual. I would agree hugetlb is
more challenge than many other modules on git archaeology. :)

Even if I mentioned the invalidate_lock, I don't think I thought deeper
than that. I just wished whenever possible we still move hugetlb code
closer to generic code, so if that's the goal we may still want to one day
have a closer look at whether hugetlb can also use invalidate_lock. Maybe
it isn't worthwhile at last: invalidate_lock is currently a rwsem, which
normally at least allows concurrent fault, but that's currently what isn't
allowed in hugetlb anyway..

If we start to remove finer grained locks that work will be even harder,
and removing folio lock in this case in fault path also brings hugetlbfs
even further from other file systems. That might be slightly against what
we used to wish to do, which is to make it closer to others. Meanwhile I'm
also not yet sure the benefit of not taking folio lock all across, e.g. I
don't expect perf would change at all even if lock is avoided. We may want
to think about that too when doing so.

>
>
> > Meanwhile, IIUC this patch can at least be split into more than one, in
> > which case the 1st patch should only change the behavior of
> > pagecache_folio, rather than the rest - if we really want to move more
> > things over to fault mutex, we can do that in the 2nd+ patches.
>
> Fair enough. This RFC was mainly for kicking off a discussion and decide
> the direction we want to take.
>
> > I'd suggest we stick with fixing pagecache_folio issue first, which can be
> > much easier and looks like a lock ordering issue only (while we can still
> > resolve it with removing on lock, but only on pagecache_folio not rest yet).
>
> Yes, I think that that should be enough to determine if we already mapped and cowed the
> folio privately.
>
> > > +
> > > /*
> > > - * hugetlb_wp() should be called with page lock of the original hugepage held.
> > > * Called with hugetlb_fault_mutex_table held and pte_page locked so we
> > > * cannot race with other handlers or page migration.
> > > * Keep the pte_same checks anyway to make transition from the mutex easier.
> > > */
> > > -static vm_fault_t hugetlb_wp(struct folio *pagecache_folio,
> > > - struct vm_fault *vmf)
> > > +static vm_fault_t hugetlb_wp(struct vm_fault *vmf, enum cow_context context)
> > > {
> > > struct vm_area_struct *vma = vmf->vma;
> > > struct mm_struct *mm = vma->vm_mm;
> > > const bool unshare = vmf->flags & FAULT_FLAG_UNSHARE;
> > > pte_t pte = huge_ptep_get(mm, vmf->address, vmf->pte);
> > > struct hstate *h = hstate_vma(vma);
> > > - struct folio *old_folio;
> > > - struct folio *new_folio;
> > > bool cow_from_owner = 0;
> > > vm_fault_t ret = 0;
> > > struct mmu_notifier_range range;
> > > + struct folio *old_folio, *new_folio, *pagecache_folio;
> > > + struct address_space *mapping = vma->vm_file->f_mapping;
> > >
> > > /*
> > > * Never handle CoW for uffd-wp protected pages. It should be only
> > > @@ -6198,7 +6201,11 @@ static vm_fault_t hugetlb_wp(struct folio *pagecache_folio,
> > > * we run out of free hugetlb folios: we would have to kill processes
> > > * in scenarios that used to work. As a side effect, there can still
> > > * be leaks between processes, for example, with FOLL_GET users.
> > > + *
> > > + * We need to take the lock here because the folio might be undergoing a
> > > + * migration. e.g: see folio_try_share_anon_rmap_pte.
> > > */
> >
> > I agree we'd better keep the old_folio locked as of now to be further
> > discussed, but I'm not 100% sure about the comment - doesn't migration path
> > still need the pgtable lock to modify mapcounts? I think we hold pgtable
> > lock here.
>
> Uhm, yes, looking closer I think you are right.
> In hugetlb_fault, prior to call in hugetlb_wp we take the ptl spinlock,
> and the migration path also takes it (via page_vma_mapped_walk), so we
> should not be seeing any spurious data from hugetlb_remove_rmap, as we
> are serialized.
>
> > > + /*
> > > + * We can be called from two different contexts: hugetlb_no_page or
> > > + * hugetlb_fault.
> > > + * When called from the latter, we try to find the original folio in
> > > + * the pagecache and compare it against the current folio we have mapped
> > > + * in the pagetables. If it differs, it means that this process already
> > > + * CoWed and mapped the folio privately, so we know that a reservation
> > > + * was already consumed.
> > > + * This will be latter used to determine whether we need to unmap the folio
> > > + * from other processes in case we fail to allocate a new folio.
> > > + */
> > > + if (context == HUGETLB_FAULT_CONTEXT) {
> > > + pagecache_folio = filemap_get_hugetlb_folio(h, mapping, vmf->pgoff);
> > > + if (IS_ERR(pagecache_folio))
> > > + pagecache_folio = NULL;
> > > + else
> > > + folio_put(pagecache_folio);
> >
> > So here we released the refcount but then we're referencing the pointer
> > below.. I don't know whether this is wise, looks like it's prone to
> > races.. If we want, we can compare the folios before releasing the
> > refcount. Said that,...
>
> We are holding the mutex so the folio cannot really go anywhere, but
> could folio_put after the check as well.
>
> > > - if (is_vma_resv_set(vma, HPAGE_RESV_OWNER) &&
> > > - old_folio != pagecache_folio)
> > > + if (context == HUGETLB_FAULT_CONTEXT &&
> > > + is_vma_resv_set(vma, HPAGE_RESV_OWNER) &&
> > > + old_folio != pagecache_folio)
> >
> > .. here I am actually thinking whether we can use folio_test_anon() and
> > completely avoid looking up the page cache. Here, the ultimate goal is
> > trying to know whether this is a CoW on top of a private page. Then IIUC
> > folio_test_anon(old_folio) is enough.
>
> I yet have to fully check, but this sounds reasonable to me.
>
> > > new_folio = false;
> > > - folio = filemap_lock_hugetlb_folio(h, mapping, vmf->pgoff);
> > > + folio = filemap_get_hugetlb_folio(h, mapping, vmf->pgoff);
> >
> > So this is part of the changes that I mentioned previously, that we should
> > avoid doing in one patch; actually I really think we should think twice on
> > using fault mutex explicitly for more things. Actually I tend to go the
> > other way round: I used to think whether we can avoid some fault mutex in
> > some paths. E.g. for UFFDIO_COPY it still makes sense to take fault mutex
> > because it faces similar challenge of concurrent allocations. However I'm
> > not sure about truncate/hole-punch lines. IIUC most file folios use
> > invalidate_lock for that, and I'd think going the other way to use the same
> > lock in hugetlb code, then keep fault mutex as minimum used as possible,
> > then the semantics of the lock and what it protects are much clearer.
>
> I hear you, but as explained above, I am not entirely sure we can get
> rid of the mutex in the truncation/punch-hole path, and if that is the
> case, having both the lock and the mutex is definitely an overkill.
>
> I can split up the changes, I have no problem with that.

Thanks! I hope that'll also help whatever patch to land sooner, after it
can be verified to fix the issue.

--
Peter Xu