Re: [PATCH] mm: Merge pte_mkhuge() call into arch_make_huge_pte()

[Anshuman Khandual <anshuman.khandual@arm.com>]

On 2/2/22 11:50 AM, Christophe Leroy wrote:
> 
> Le 02/02/2022 à 06:38, Anshuman Khandual a écrit :
>> Each call into pte_mkhuge() is invariably followed by arch_make_huge_pte().
>> Instead arch_make_huge_pte() can accommodate pte_mkhuge() at the beginning.
>> This updates generic fallback stub for arch_make_huge_pte() and available
>> platforms definitions. This makes huge pte creation much cleaner and easier
>> to follow.
> I think it is a good cleanup. I always wonder why commit d9ed9faac283 
> ("mm: add new arch_make_huge_pte() method for tile support") didn't move 
> the pte_mkhuge() into arch_make_huge_pte().

+1

> 
> When I implemented arch_make_huge_pte() for powerpc 8xx, in one case 
> arch_make_huge_pte() have to undo the things done by pte_mkhuge(), see below
> 
> As a second step we could probably try to get rid of pte_mkhuge() 
> completely, at least in the core.

Sure.

> 
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Will Deacon <will@kernel.org>
>> Cc: Michael Ellerman <mpe@ellerman.id.au>
>> Cc: Paul Mackerras <paulus@samba.org>
>> Cc: "David S. Miller" <davem@davemloft.net>
>> Cc: Mike Kravetz <mike.kravetz@oracle.com>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: linuxppc-dev@lists.ozlabs.org
>> Cc: sparclinux@vger.kernel.org
>> Cc: linux-mm@kvack.org
>> Cc: linux-kernel@vger.kernel.org
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> 
>> ---
>>   arch/arm64/mm/hugetlbpage.c                      | 1 +
>>   arch/powerpc/include/asm/nohash/32/hugetlb-8xx.h | 1 +
>>   arch/sparc/mm/hugetlbpage.c                      | 1 +
>>   include/linux/hugetlb.h                          | 2 +-
>>   mm/hugetlb.c                                     | 3 +--
>>   mm/vmalloc.c                                     | 1 -
>>   6 files changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm64/mm/hugetlbpage.c b/arch/arm64/mm/hugetlbpage.c
>> index ffb9c229610a..228226c5fa80 100644
>> --- a/arch/arm64/mm/hugetlbpage.c
>> +++ b/arch/arm64/mm/hugetlbpage.c
>> @@ -347,6 +347,7 @@ pte_t arch_make_huge_pte(pte_t entry, unsigned int shift, vm_flags_t flags)
>>   {
>>   	size_t pagesize = 1UL << shift;
>>   
>> +	entry = pte_mkhuge(entry);
>>   	if (pagesize == CONT_PTE_SIZE) {
>>   		entry = pte_mkcont(entry);
>>   	} else if (pagesize == CONT_PMD_SIZE) {
>> diff --git a/arch/powerpc/include/asm/nohash/32/hugetlb-8xx.h b/arch/powerpc/include/asm/nohash/32/hugetlb-8xx.h
>> index 64b6c608eca4..e41e095158c7 100644
>> --- a/arch/powerpc/include/asm/nohash/32/hugetlb-8xx.h
>> +++ b/arch/powerpc/include/asm/nohash/32/hugetlb-8xx.h
>> @@ -70,6 +70,7 @@ static inline pte_t arch_make_huge_pte(pte_t entry, unsigned int shift, vm_flags
>>   {
>>   	size_t size = 1UL << shift;
>>   
>> +	entry = pte_mkhuge(entry);
> Could drop that and replace the below by:
> 
> 	if (size == SZ_16K)
> 		return __pte(pte_val(entry) | _PAGE_SPS);
> 	else
> 		return __pte(pte_val(entry) | _PAGE_SPS | _PAGE_HUGE);
> 	
> 

Sure, will change as stated above.