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

[jane.chu@oracle.com]

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@kernel.org>
> 
> 
> 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