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

[jane . chu]

Hi, David,

On 3/25/2026 1:49 AM, David Hildenbrand (Arm) wrote:
[..]
> > --- a/include/linux/hugetlb.h~mm-userfaultfd-fix-hugetlb-fault-mutex-hash-calculation
> > +++ a/include/linux/hugetlb.h
> > @@ -796,6 +796,23 @@ static inline unsigned huge_page_shift(s
> >   	return h->order + PAGE_SHIFT;
> >   }
> >   
> > +/**
> > + * hugetlb_linear_page_index() - linear_page_index() but in hugetlb
> > + *				 page size granularity.
> > + * @vma: the hugetlb VMA
> > + * @address: the virtual address within the VMA
> > + *
> > + * Return: the page offset within the mapping in huge page units.
> > + */
> > +static inline pgoff_t hugetlb_linear_page_index(struct vm_area_struct *vma,
> > +		unsigned long address)
> > +{
> > +	struct hstate *h = hstate_vma(vma);
> > +
> > +	return ((address - vma->vm_start) >> huge_page_shift(h)) +
> > +		(vma->vm_pgoff >> huge_page_order(h));
> > +}
> > +
> >   static inline bool order_is_gigantic(unsigned int order)
> >   {
> >   	return order > MAX_PAGE_ORDER;
> > --- a/mm/userfaultfd.c~mm-userfaultfd-fix-hugetlb-fault-mutex-hash-calculation
> > +++ a/mm/userfaultfd.c
> > @@ -573,7 +573,7 @@ retry:
> >   		 * in the case of shared pmds.  fault mutex prevents
> >   		 * races with other faulting threads.
> >   		 */
> > -		idx = linear_page_index(dst_vma, dst_addr);
> > +		idx = hugetlb_linear_page_index(dst_vma, dst_addr);
> >   		mapping = dst_vma->vm_file->f_mapping;
> >   		hash = hugetlb_fault_mutex_hash(mapping, idx);
> >   		mutex_lock(&hugetlb_fault_mutex_table[hash]);
> > _
>
> Let's take a look at other hugetlb_fault_mutex_hash() users:
>
> * remove_inode_hugepages: uses folio->index >> huge_page_order(h)
>   -> hugetlb granularity
> * hugetlbfs_fallocate(): start/index is in hugetlb granularity
>   -> hugetlb granularity
> * memfd_alloc_folio(): idx >>= huge_page_order(h);
>   -> hugetlb granularity
> * hugetlb_wp(): uses vma_hugecache_offset()
>   -> hugetlb granularity
> * hugetlb_handle_userfault(): uses vmf->pgoff, which hugetlb_fault()
>    sets to vma_hugecache_offset()
>   -> hugetlb granularity
> * hugetlb_no_page(): similarly uses vmf->pgoff
>   -> hugetlb granularity
> * hugetlb_fault(): similarly uses vmf->pgoff
>   -> hugetlb granularity
>
> So this change here looks good to me
>
> Reviewed-by: David Hildenbrand (Arm) <david@xxxxxxxxxx>
>
>
> 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 ?

> (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.  At minimum, we could have
  hugetlb_linear_page_index(vma, addr)
  -> __hugetlb_linear_page_index(h, vma, addr)
basically renaming vma_hugecache_offset().

> In general, I think we should look into having idx/vmf->pgoff being
> consistent with the remainder of MM, converting all code in hugetlb to
> do that.
>
> Any takers?

I'd be happy to, just to make sure I understand the proposal clearly.

thanks!
-jane