Re: [RFC PATCH v4 13/16] powerpc/e500: Use contiguous PMD instead of hugepd

[Christophe Leroy]

Le 29/05/2024 à 10:49, Oscar Salvador a écrit :
> [Vous ne recevez pas souvent de courriers de osalvador@xxxxxxxx. D?couvrez pourquoi ceci est important ? https://aka.ms/LearnAboutSenderIdentification ]
> 
> On Mon, May 27, 2024 at 03:30:11PM +0200, Christophe Leroy wrote:
>> e500 supports many page sizes among which the following size are
>> implemented in the kernel at the time being: 4M, 16M, 64M, 256M, 1G.
>>
>> On e500, TLB miss for hugepages is exclusively handled by SW even
>> on e6500 which has HW assistance for 4k pages, so there are no
>> constraints like on the 8xx.
>>
>> On e500/32, all are at PGD/PMD level and can be handled as
>> cont-PMD.
>>
>> On e500/64, smaller ones are on PMD while bigger ones are on PUD.
>> Again, they can easily be handled as cont-PMD and cont-PUD instead
>> of hugepd.
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@xxxxxxxxxx>
> 
> ...
> 
>> diff --git a/arch/powerpc/include/asm/nohash/pgtable.h b/arch/powerpc/include/asm/nohash/pgtable.h
>> index 90d6a0943b35..f7421d1a1693 100644
>> --- a/arch/powerpc/include/asm/nohash/pgtable.h
>> +++ b/arch/powerpc/include/asm/nohash/pgtable.h
>> @@ -52,11 +52,36 @@ static inline pte_basic_t pte_update(struct mm_struct *mm, unsigned long addr, p
>>   {
>>        pte_basic_t old = pte_val(*p);
>>        pte_basic_t new = (old & ~(pte_basic_t)clr) | set;
>> +     unsigned long sz;
>> +     unsigned long pdsize;
>> +     int i;
>>
>>        if (new == old)
>>                return old;
>>
>> -     *p = __pte(new);
>> +#ifdef CONFIG_PPC_E500
>> +     if (huge)
>> +             sz = 1UL << (((old & _PAGE_HSIZE_MSK) >> _PAGE_HSIZE_SHIFT) + 20);
>> +     else
> 
> I think this will not compile when CONFIG_PPC_85xx && !CONFIG_PTE_64BIT.

Yes, I got a feedback on this from the robots.

> 
> You have declared _PAGE_HSIZE_MSK and _PAGE_HSIZE_SHIFT in
> arch/powerpc/include/asm/nohash/hugetlb-e500.h.
> 
> But hugetlb-e500.h is only included if CONFIG_PPC_85xx && CONFIG_PTE_64BIT
> (see arch/powerpc/include/asm/nohash/32/pgtable.h).
> 
> 
> 
>> +#endif
>> +             sz = PAGE_SIZE;
>> +
>> +     if (!huge || sz < PMD_SIZE)
>> +             pdsize = PAGE_SIZE;
>> +     else if (sz < PUD_SIZE)
>> +             pdsize = PMD_SIZE;
>> +     else if (sz < P4D_SIZE)
>> +             pdsize = PUD_SIZE;
>> +     else if (sz < PGDIR_SIZE)
>> +             pdsize = P4D_SIZE;
>> +     else
>> +             pdsize = PGDIR_SIZE;
>> +
>> +     for (i = 0; i < sz / pdsize; i++, p++) {
>> +             *p = __pte(new);
>> +             if (new)
>> +                     new += (unsigned long long)(pdsize / PAGE_SIZE) << PTE_RPN_SHIFT;
> 
> I guess 'new' can be 0 if pte_update() is called on behave of clearing the pte?

It is exactly that, and without that verification I had pmd_bad() 
returning bad pmds after freeing page tables.

> 
>> +static inline unsigned long pmd_leaf_size(pmd_t pmd)
>> +{
>> +     return 1UL << (((pmd_val(pmd) & _PAGE_HSIZE_MSK) >> _PAGE_HSIZE_SHIFT) + 20);
> 
> Can we have the '20' somewhere defined with a comment on top explaining
> what is so it is not a magic number?
> Otherwise people might come look at this and wonder why 20.

Yes I now have :

+#define _PAGE_HSIZE_MSK (_PAGE_U0 | _PAGE_U1 | _PAGE_U2 | _PAGE_U3)
+#define _PAGE_HSIZE_SHIFT              14
+#define _PAGE_HSIZE_SHIFT_OFFSET       20

and have added a helper to avoid doing the calculation at several places:

+static inline unsigned long pte_huge_size(pte_t pte)
+{
+       pte_basic_t val = pte_val(pte);
+
+       return 1UL << (((val & _PAGE_HSIZE_MSK) >> _PAGE_HSIZE_SHIFT) + 
_PAGE_HSIZE_SHIFT_OFFSET);
+}


> 
>> --- a/arch/powerpc/mm/pgtable.c
>> +++ b/arch/powerpc/mm/pgtable.c
>> @@ -331,6 +331,37 @@ void set_huge_pte_at(struct mm_struct *mm, unsigned long addr, pte_t *ptep,
>>                __set_huge_pte_at(pmdp, ptep, pte_val(pte));
>>        }
>>   }
>> +#elif defined(CONFIG_PPC_E500)
>> +void set_huge_pte_at(struct mm_struct *mm, unsigned long addr, pte_t *ptep,
>> +                  pte_t pte, unsigned long sz)
>> +{
>> +     unsigned long pdsize;
>> +     int i;
>> +
>> +     pte = set_pte_filter(pte, addr);
>> +
>> +     /*
>> +      * Make sure hardware valid bit is not set. We don't do
>> +      * tlb flush for this update.
>> +      */
>> +     VM_WARN_ON(pte_hw_valid(*ptep) && !pte_protnone(*ptep));
>> +
>> +     if (sz < PMD_SIZE)
>> +             pdsize = PAGE_SIZE;
>> +     else if (sz < PUD_SIZE)
>> +             pdsize = PMD_SIZE;
>> +     else if (sz < P4D_SIZE)
>> +             pdsize = PUD_SIZE;
>> +     else if (sz < PGDIR_SIZE)
>> +             pdsize = P4D_SIZE;
>> +     else
>> +             pdsize = PGDIR_SIZE;
>> +
>> +     for (i = 0; i < sz / pdsize; i++, ptep++, addr += pdsize) {
>> +             __set_pte_at(mm, addr, ptep, pte, 0);
>> +             pte = __pte(pte_val(pte) + ((unsigned long long)pdsize / PAGE_SIZE << PFN_PTE_SHIFT));
> 
> You can use pte_advance_pfn() here? Just give have
> 
>   nr = (unsigned long long)pdsize / PAGE_SIZE << PFN_PTE_SHIFT)
>   pte_advance_pfn(pte, nr)

That's what I did before but it didn't work. The problem is that 
pte_advance_pfn() takes a long not a long long:

static inline pte_t pte_advance_pfn(pte_t pte, unsigned long nr)
{
	return __pte(pte_val(pte) + (nr << PFN_PTE_SHIFT));
}

And when I called it with nr = PMD_SIZE / PAGE_SIZE = 2M / 4k = 512, as 
we have PFN_PTE_SHIFT = 24, I got 512 << 24 = 0

> 
> Which 'sz's can we have here? You mentioned that e500 support:
> 
> 4M, 16M, 64M, 256M, 1G.
> 
> which of these ones can be huge?

All are huge.

> 
> 
> --
> Oscar Salvador
> SUSE Labs