Re: [PATCH v4] mm/userfaultfd: fix hugetlb fault mutex hash calculation

[David Hildenbrand (Arm)]

On 3/26/26 00:46, jane.chu@xxxxxxxxxx wrote:
> Hi, David,
> 
> On 3/25/2026 1:49 AM, David Hildenbrand (Arm) wrote:
> [..]

[...]

>>
>> But it raises the question:
>>
>> (1) should be convert all that to just operate on the ordinary index,
>> such that we don't even need hugetlb_linear_page_index()? That would be
>> an addon patch.
>>
> 
> Do you mean to convert all callers of hugetlb_linear_page_index() and
> vma_hugepcache_offset() to use index and huge_page_order(h) ?
> May I add, to improve readability, rename the huge-page-granularity
> 'idx' to huge_idx or hidx ?

What I meant is that we change all hugetlb code to use the ordinary idx.
It's a bigger rework.

For example, we'd be getting rid of filemap_lock_hugetlb_folio() completely and
simply use filemap_lock_folio. As one example:

@@ -657,10 +657,9 @@ static void hugetlbfs_zero_partial_page(struct hstate *h,
                                        loff_t start,
                                        loff_t end)
 {
-       pgoff_t idx = start >> huge_page_shift(h);
        struct folio *folio;
 
-       folio = filemap_lock_hugetlb_folio(h, mapping, idx);
+       folio = filemap_lock_folio(mapping, start >> PAGE_SHIFT);
        if (IS_ERR(folio))
                return;
 
Other parts are more tricky, as we have to make sure that we get
an idx that points at the start of the folio.

Likely such a conversion could be done incrementally. But it's a bit of work.

We'd be getting rid of some more hugetlb special casing.


An alternative is passing in an address into hugetlb_linear_page_index(),
just letting it do the calculation itself (it can get the hstate from the mapping).

> 
>> (2) Alternatively, could we replace all users of vma_hugecache_offset()
>> by the much cleaner hugetlb_linear_page_index() ?
>>
> 
> The difference between the two helpers is hstate_vma() in the latter
> that is about 5 pointer de-references, not sure of any performance
> implication though. 

hstate_vma() is really just hstate_file(vma->vm_file)->
hstate_inode(file_inode(f))->HUGETLBFS_SB(i->i_sb)->hstate;

So some pointer chasing.

hard to believe that this would matter in any of this code :)

> At minimum, we could have
>   hugetlb_linear_page_index(vma, addr)
>   -> __hugetlb_linear_page_index(h, vma, addr)
> basically renaming vma_hugecache_offset().
I would only do that if it's really required for performance.

-- 
Cheers,

David