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 <[email protected]>
> ---
> 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.
> 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.
--
Oscar Salvador
SUSE Labs
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 <[email protected]>
>> ---
>> 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
On Mon, May 27, 2024 at 03:51:41PM +0000, Christophe Leroy wrote:
> We could be is that worth the churn ?
Probably not.
> With patch 1 there was only one callsite.
Yes, you are right here.
> 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.
It is probably not worth pursuing this then.
As you said, there are many callers and we would have to create some kind of hook
for only those interested places, which I guess would end up looking just too ugly
in order to save little code in arch code.
So please disregard my comment here, and stick with what we have.
> 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
I did not notice, thanks.
--
Oscar Salvador
SUSE Labs