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

[Peter Xu <peterx@redhat.com>]

On Tue, Jun 03, 2025 at 10:57:21AM -0400, Peter Xu wrote:
> On Tue, Jun 03, 2025 at 03:50:54PM +0200, Oscar Salvador wrote:
> > On Mon, Jun 02, 2025 at 05:30:19PM -0400, Peter Xu wrote:
> > > 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.
> > 
> > Ok, I have to confess I was not looking things from this perspective,
> > but when doing so, yes, you are right, we should strive to find
> > replacements wherever we can for not using hugetlb-specific code.
> > 
> > I do not know about this case though, not sure what other options do we
> > have when trying to shut concurrent faults while doing other operation.
> > But it is something we should definitely look at.
> > 
> > Wrt. to the lock.
> > There were two locks, old_folio (taken in hugetlb_fault) and
> > pagecache_folio one.
> 
> There're actually three places this patch touched, the 3rd one is
> hugetlb_no_page(), in which case I also think we should lock it, not only
> because file folios normally does it (see do_fault(), for example), but
> also that's exactly what James mentioned I believe on possible race of
> !uptodate hugetlb folio being injected by UFFDIO_CONTINUE, along the lines:
> 
> 		folio = alloc_hugetlb_folio(vma, vmf->address, false);
>                 ...
> 		folio_zero_user(folio, vmf->real_address);
> 		__folio_mark_uptodate(folio);

I think I was wrong here at least partly..  So what this patch changed is
only the lookup of the no_page path, hence what I said here doesn't apply.
This patch also mentioned in the commit message on why James's concern was
ok - the fault mutex was held.  Yes I agree. Actually even without fault
mutex, the folio is only injected into page cache after mark uptodate.. so
it looks fine even without the mutex.

Though it's still true there're three paths to be discussed, which should
include no_page, and it's still needed to be discussed when any of us like
to remove folio lock even in the lookup path.

For example, I'm not sure whether it's always thread safe to do
folio_test_hwpoison() when without it, even with the fault mutex.

Sorry for the confusion, Oscar.

-- 
Peter Xu