Re: [PATCH v1 01/16] mm: hugetlb: Add huge page size param to huge_ptep_get_and_clear()

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

Thanks for the review!

On 06/02/2025 05:03, Anshuman Khandual wrote:
> 
> 
> On 2/5/25 20:39, Ryan Roberts wrote:
>> 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>
>> ---
>>  arch/arm64/include/asm/hugetlb.h     |  4 ++--
>>  arch/arm64/mm/hugetlbpage.c          |  8 +++++---
>>  arch/loongarch/include/asm/hugetlb.h |  6 ++++--
>>  arch/mips/include/asm/hugetlb.h      |  6 ++++--
>>  arch/parisc/include/asm/hugetlb.h    |  2 +-
>>  arch/parisc/mm/hugetlbpage.c         |  2 +-
>>  arch/powerpc/include/asm/hugetlb.h   |  6 ++++--
>>  arch/riscv/include/asm/hugetlb.h     |  3 ++-
>>  arch/riscv/mm/hugetlbpage.c          |  2 +-
>>  arch/s390/include/asm/hugetlb.h      | 12 ++++++++----
>>  arch/s390/mm/hugetlbpage.c           | 10 ++++++++--
>>  arch/sparc/include/asm/hugetlb.h     |  2 +-
>>  arch/sparc/mm/hugetlbpage.c          |  2 +-
>>  include/asm-generic/hugetlb.h        |  2 +-
>>  include/linux/hugetlb.h              |  4 +++-
>>  mm/hugetlb.c                         |  4 ++--
>>  16 files changed, 48 insertions(+), 27 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/hugetlb.h b/arch/arm64/include/asm/hugetlb.h
>> index c6dff3e69539..03db9cb21ace 100644
>> --- a/arch/arm64/include/asm/hugetlb.h
>> +++ b/arch/arm64/include/asm/hugetlb.h
>> @@ -42,8 +42,8 @@ extern int huge_ptep_set_access_flags(struct vm_area_struct *vma,
>>  				      unsigned long addr, pte_t *ptep,
>>  				      pte_t pte, int dirty);
>>  #define __HAVE_ARCH_HUGE_PTEP_GET_AND_CLEAR
>> -extern pte_t huge_ptep_get_and_clear(struct mm_struct *mm,
>> -				     unsigned long addr, pte_t *ptep);
>> +extern pte_t huge_ptep_get_and_clear(struct mm_struct *mm, unsigned long addr,
>> +				     pte_t *ptep, unsigned long sz);
> 
> If VMA could be passed instead of MM, the size of the huge page can
> be derived via huge_page_size(hstate_vma(vma)) and another argument
> here need not be added. Also MM can be derived from VMA if required.

I considered this approach; infact that's what I first implemented when fixing
an equivalent bug on set_huge_pte_at() in the past. But that was problematic
because there are some cases where the helper is used for kernel mappings (see
vmalloc) and there is no VMA to pass in that case. See [1].

To fix this bug, usage of this helper for kernel mappings is not an issue (yet)
so I guess technically it could be fixed by passing VMA. But later in this
series I start using huge_ptep_get_and_clear() for the vmap (see patch 11) so it
would break at that point.

Another approach I considered was to allocate a spare swap-pte bit (we have a
few) to indicate PTE_CONT for non-present PTEs. Then no API change is required
at all. But given set_huge_pte_at() and huge_pte_clear() already pass "sz", it
seemed best just to keep things simple and follow that pattern.

[1] https://lore.kernel.org/all/20230922115804.2043771-1-ryan.roberts@arm.com/

Thanks,
Ryan