Re: [RFC PATCH v3 03/16] mm: Provide mm_struct and address to huge_ptep_get()

[Christophe Leroy]

Le 27/05/2024 à 13:19, Oscar Salvador a écrit :
> On Sun, May 26, 2024 at 11:22:23AM +0200, Christophe Leroy wrote:
>> On powerpc 8xx huge_ptep_get() will need to know whether the given
>> ptep is a PTE entry or a PMD entry. This cannot be known with the
>> PMD entry itself because there is no easy way to know it from the
>> content of the entry.
>>
>> So huge_ptep_get() will need to know either the size of the page
>> or get the pmd.
>>
>> In order to be consistent with huge_ptep_get_and_clear(), give
>> mm and address to huge_ptep_get().
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@xxxxxxxxxx>
>> ---
>> v2: Add missing changes in arch implementations
>> v3: Fixed a comment in ARM and missing changes in S390
>> ---
>>   arch/arm/include/asm/hugetlb-3level.h |  4 +--
>>   arch/arm64/include/asm/hugetlb.h      |  2 +-
>>   arch/arm64/mm/hugetlbpage.c           |  2 +-
>>   arch/riscv/include/asm/hugetlb.h      |  2 +-
>>   arch/riscv/mm/hugetlbpage.c           |  2 +-
>>   arch/s390/include/asm/hugetlb.h       |  4 +--
>>   arch/s390/mm/hugetlbpage.c            |  4 +--
> 
> I was wondering whether we could do something similar for what we did in
> patch#1, so we do not touch architectures code.

We could be is that worth the churn ?

With patch 1 there was only one callsite.

Here we have many callsites, and we also have huge_ptep_get_and_clear() 
which already takes three arguments. So for me it make more sense to 
adapt huge_ptep_get() here.

Today several of the huge-related functions already have parameters that 
are used only by a few architectures and everytime one architecture 
needs a new parameter it is added for all of them, and there are 
exemples in the past of new functions added to get new parameters for 
only a few architectures that ended up with a mess and a need to 
re-factor at the end.

See for instance the story around arch_make_huge_pte() and pte_mkhuge(), 
both do the same but arch_make_huge_pte() was added to take additional 
parameters by commit d9ed9faac283 ("mm: add new arch_make_huge_pte() 
method for tile support") then they were merged by commit 16785bd77431 
("mm: merge pte_mkhuge() call into arch_make_huge_pte()")

So I'm open to any suggestion but we need to try not make it a bigger 
mess at the end.

By the way, I think most if not all huge related helpers should all take 
the same parameters even if not all of them are used, then it would make 
things easier. And maybe the cleanest would be to give the page size to 
all those functions instead of having them guess it.

So let's have your ideas here on the most straight forward way to handle 
that.

> 
>    
>> diff --git a/mm/gup.c b/mm/gup.c
>> index 1611e73b1121..86b5105b82a1 100644
>> --- a/mm/gup.c
>> +++ b/mm/gup.c
>> @@ -2812,7 +2812,7 @@ static int gup_hugepte(pte_t *ptep, unsigned long sz, unsigned long addr,
>>   	if (pte_end < end)
>>   		end = pte_end;
>>   
>> -	pte = huge_ptep_get(ptep);
>> +	pte = huge_ptep_get(NULL, addr, ptep);
> 
> I know that after this series all this code is gone, but I was not sure
> about the behaviour between this patch and the last one.
> 
> It made me nervous, until I realized that this code is only used
> on CONFIG_ARCH_HAS_HUGEPD, which should not be the case anymore for 8xx after
> patch#8, and since 8xx is the only one that will use the mm parameter from
> huge_ptep_get, we are all good.
> 

By the way, after commit 01d89b93e176 ("mm/gup: fix hugepd handling in 
hugetlb rework") we now have the vma in gup_hugepte() so we now pass 
vma->vm_mm

Thanks for the review
Christophe