Re: [PATCH v2 1/4] mm: hugetlb: Add huge page size param to huge_ptep_get_and_clear()

[Ryan Roberts <ryan.roberts@arm.com>]

On 25/02/2025 14:25, Alexander Gordeev wrote:
> On Mon, Feb 17, 2025 at 02:04:14PM +0000, Ryan Roberts wrote:
> 
> Hi Ryan,
> 
>> In order to fix a bug, arm64 needs to be told the size of the huge page
>> for which the huge_pte is being set in huge_ptep_get_and_clear().
>> Provide for this by adding an `unsigned long sz` parameter to the
>> function. This follows the same pattern as huge_pte_clear() and
>> set_huge_pte_at().
>>
>> This commit makes the required interface modifications to the core mm as
>> well as all arches that implement this function (arm64, loongarch, mips,
>> parisc, powerpc, riscv, s390, sparc). The actual arm64 bug will be fixed
>> in a separate commit.
>>
>> Cc: stable@vger.kernel.org
>> Fixes: 66b3923a1a0f ("arm64: hugetlb: add support for PTE contiguous bit")
>> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
>> ---
> ...
>> diff --git a/arch/s390/include/asm/hugetlb.h b/arch/s390/include/asm/hugetlb.h
>> index 7c52acaf9f82..420c74306779 100644
>> --- a/arch/s390/include/asm/hugetlb.h
>> +++ b/arch/s390/include/asm/hugetlb.h
>> @@ -26,7 +26,11 @@ void __set_huge_pte_at(struct mm_struct *mm, unsigned long addr,
>>  pte_t huge_ptep_get(struct mm_struct *mm, unsigned long addr, pte_t *ptep);
>>  
>>  #define __HAVE_ARCH_HUGE_PTEP_GET_AND_CLEAR
>> -pte_t huge_ptep_get_and_clear(struct mm_struct *mm, unsigned long addr, pte_t *ptep);
>> +pte_t huge_ptep_get_and_clear(struct mm_struct *mm,
>> +			      unsigned long addr, pte_t *ptep,
>> +			      unsigned long sz);
> 
> Please, format parameters similarily to set_huge_pte_at() few lines above.

Appologies. I've fixed this for the next version.

> 
>> +pte_t __huge_ptep_get_and_clear(struct mm_struct *mm,
>> +			      unsigned long addr, pte_t *ptep);
> 
> The formatting is broken, but please see below.

Formatting fixed here too.

> 
>>  static inline void arch_clear_hugetlb_flags(struct folio *folio)
>>  {
>> @@ -48,7 +52,7 @@ static inline void huge_pte_clear(struct mm_struct *mm, unsigned long addr,
>>  static inline pte_t huge_ptep_clear_flush(struct vm_area_struct *vma,
>>  					  unsigned long address, pte_t *ptep)
>>  {
>> -	return huge_ptep_get_and_clear(vma->vm_mm, address, ptep);
>> +	return __huge_ptep_get_and_clear(vma->vm_mm, address, ptep);
>>  }
>>  
>>  #define  __HAVE_ARCH_HUGE_PTEP_SET_ACCESS_FLAGS
>> @@ -59,7 +63,7 @@ static inline int huge_ptep_set_access_flags(struct vm_area_struct *vma,
>>  	int changed = !pte_same(huge_ptep_get(vma->vm_mm, addr, ptep), pte);
>>  
>>  	if (changed) {
>> -		huge_ptep_get_and_clear(vma->vm_mm, addr, ptep);
>> +		__huge_ptep_get_and_clear(vma->vm_mm, addr, ptep);
>>  		__set_huge_pte_at(vma->vm_mm, addr, ptep, pte);
>>  	}
>>  	return changed;
>> @@ -69,7 +73,7 @@ static inline int huge_ptep_set_access_flags(struct vm_area_struct *vma,
>>  static inline void huge_ptep_set_wrprotect(struct mm_struct *mm,
>>  					   unsigned long addr, pte_t *ptep)
>>  {
>> -	pte_t pte = huge_ptep_get_and_clear(mm, addr, ptep);
>> +	pte_t pte = __huge_ptep_get_and_clear(mm, addr, ptep);
>>  
>>  	__set_huge_pte_at(mm, addr, ptep, pte_wrprotect(pte));
>>  }
>> diff --git a/arch/s390/mm/hugetlbpage.c b/arch/s390/mm/hugetlbpage.c
>> index d9ce199953de..52ee8e854195 100644
>> --- a/arch/s390/mm/hugetlbpage.c
>> +++ b/arch/s390/mm/hugetlbpage.c
>> @@ -188,8 +188,8 @@ pte_t huge_ptep_get(struct mm_struct *mm, unsigned long addr, pte_t *ptep)
>>  	return __rste_to_pte(pte_val(*ptep));
>>  }
>>  
>> -pte_t huge_ptep_get_and_clear(struct mm_struct *mm,
>> -			      unsigned long addr, pte_t *ptep)
>> +pte_t __huge_ptep_get_and_clear(struct mm_struct *mm,
>> +				unsigned long addr, pte_t *ptep)
>>  {
>>  	pte_t pte = huge_ptep_get(mm, addr, ptep);
>>  	pmd_t *pmdp = (pmd_t *) ptep;
>> @@ -202,6 +202,12 @@ pte_t huge_ptep_get_and_clear(struct mm_struct *mm,
>>  	return pte;
>>  }
>>  
>> +pte_t huge_ptep_get_and_clear(struct mm_struct *mm,
>> +			      unsigned long addr, pte_t *ptep, unsigned long sz)
>> +{
>> +	return __huge_ptep_get_and_clear(mm, addr, ptep);
>> +}
> 
> Is there a reason why this is not a header inline, as other callers of
> __huge_ptep_get_and_clear()?

I was trying to make the change as uninvasive as possible, so didn't want to
change the linkage in case I accidentally broke something. Happy to make this an
inline in the header though, if you prefer?

Thanks,
Ryan

> 
>>  pte_t *huge_pte_alloc(struct mm_struct *mm, struct vm_area_struct *vma,
>>  			unsigned long addr, unsigned long sz)
>>  {
> ...
> 
> Thanks!