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

[David Hildenbrand <david@redhat.com>]

On 03.06.25 15:50, 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.
> The thing was not about worry as how much perf we leave on the table
> because of these locks, as I am pretty sure is next to 0, but my drive
> was to understand what are protection and why, because as the discussion
> showed, none of us really had a good idea about it and it turns out that this
> goes back more than ~20 years ago.
> 
> Another topic for the lock (old_folio, so the one we copy from),
> when we compare it to generic code, we do not take the lock there.
> Looking at do_wp_page(), we do __get__ a reference on the folio we copy
> from, but not the lock, so AFAIU, the lock seems only to please
> folio_move_anon_rmap() from hugetlb_wp.
> 
> Taking a look at do_wp_page()->wp_can_reuse_anon_folio() which also
> calls folio_move_anon_rmap() in case we can re-use the folio, it only
> takes the lock before the call to folio_move_anon_rmap(), and then
> unlocks it.

No.

It takes the lock around "folio_ref_count(folio) != 1" as well.

IOW, if the ref_count is 1, the mapcount must be <= 1, and as the page 
*is* mapped, we know the mapcount is >= 1.

So if the ref_count == mapcount == 1 and the folio is locked, we cannot 
have concurrent unmapping/splitting/migration of the folio that could 
affect the mapcount/refcount.

-- 
Cheers,

David / dhildenb