Re: [PATCH v1 2/8] powerpc: hugetlb: Convert set_huge_pte_at() to take vma

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

On 22/09/2023 10:14, Christophe Leroy wrote:
> 
> 
> Le 22/09/2023 à 10:41, Ryan Roberts a écrit :
>> On 22/09/2023 09:10, Christophe Leroy wrote:
>>>
>>>
>>>> I'm happy to take your proposed approach if that's your preference. Another
>>>> option is to use a dummy VMA, as I have done in the core code, for the one call
>>>> site that calls set_huge_pte_at() with init_mm:
>>>>
>>>> struct vm_area_struct vma = TLB_FLUSH_VMA(&init_mm, 0);
>>>>
>>>> This is an existing macro that creates a dummy vma with vma->vm_mm filled in.
>>>> Then I pass &vma to the function.
>>>
>>> I don't like that, I prefer the solution I proposed. We already have a
>>> couple places where powerpc do things based on whether vma is NULL or not.
>>>
>>>>
>>>> Or yet another option would be to keep the mm param as is in set_huge_pte_at(),
>>>> and add a size param to the function. But then all call sites have the burden of
>>>> figuring out the size of the huge pte (although I think most know already).
>>>
>>> Indeed.
>>>
>>> arch_make_huge_pte() used to take a vma until commit 79c1c594f49a
>>> ("mm/hugetlb: change parameters of arch_make_huge_pte()").
>>>
>>> Should we try and have the same approach ? Or is it irrelevant ?
>>
>> See [1]; I'm going to rework to pass mm + size parameter since the current
>> approach will break riscv.
> 
> Can you pass a shift parameter instead of a size, like 
> arch_make_huge_pte() ? As far as I remember it is easier to handle a 
> shift than a size.

Most of the call sites already have the size, not the shift. And arm64 needs the
size, so it would have do (1UL << shift). So on that basis, I prefer to pass
size. huge_pte_clear() already passes long unsigned sz, so I'd rather follow
that pattern.

> 
> Christophe