Re: [PATCH v5 02/18] mm: Define __pte_leaf_size() to also take a PMD entry

[LEROY Christophe <christophe.leroy2@cs-soprasteria.com>]

Le 11/06/2024 à 11:34, Oscar Salvador a écrit :
> [Vous ne recevez pas souvent de courriers de osalvador@suse.de. D?couvrez pourquoi ceci est important ? https://aka.ms/LearnAboutSenderIdentification ]
> 
> On Mon, Jun 10, 2024 at 07:54:47AM +0200, Christophe Leroy wrote:
>> On powerpc 8xx, when a page is 8M size, the information is in the PMD
>> entry. So allow architectures to provide __pte_leaf_size() instead of
>> pte_leaf_size() and provide the PMD entry to that function.
>>
>> When __pte_leaf_size() is not defined, define it as a pte_leaf_size()
>> so that architectures not interested in the PMD arguments are not
>> impacted.
>>
>> Only define a default pte_leaf_size() when __pte_leaf_size() is not
>> defined to make sure nobody adds new calls to pte_leaf_size() in the
>> core.
> 
> Hi Christophe,
> 
> Now I am going to give you a hard time, so sorry in advance.
> I should have raised this before, but I was not fully aware of it.
> 
> There is an ongoing effort of unifying pagewalkers [1], so hugetlb does not have
> to be special-cased anymore, and the operations we do for THP on page-table basis
> work for hugetlb as well.
> 
> The most special bit about this is huge_ptep_get.
> huge_ptep_get() gets special handled on arm/arm64/riscv and s390.
> 
> arm64 and riscv is about cont-pmd/pte and propagate the dirty/young bits bits, so that
> is fine as walkers can already understand that.
> s390 is a funny one because it converts pud/pmd to pte and viceversa, because hugetlb
> *works* with ptes, so before returning the pte it has to transfer all
> bits from PUD/PMD level into a something that PTE level can understand.
> As you can imagine, this can be gone as we already have all the
> information in PUD/PMD and that is all pagewalkers need.
> 
> But we are left with the one you will introduce in patch#8.
> 
> 8MB pages get mapped as cont-pte, but all the information is stored in
> the PMD entries (size, dirtiness, present etc).

I'm not sure I understand what you mean.

In my case, the PMD entry is almost standard, the only thing it contains 
is a bit telling that the pointed PTEs are to be mapped 8M.

> huge_ptep_get() will return the PMD for 8MB, and so all operations hugetlb
> code performs with what huge_ptep_get returns will be performed on those PMDs.

Indeed no, my huge_ptep_get() doesn't return the PMD but the PTE.

> 
> Which brings me to this point:
> 
> I do not think __pte_leaf_size is needed. AFAICS, it should be enough to define
> pmd_leaf on 8xx, and return 8MB if it is a 8MB hugepage.

If I declare it as a PMD leaf, then many places will expect the PTE 
entry to be the PMD entry, which is not the case here. As far as I 
understand, in order that the walker walks down to the page table, we 
need it flaged as non-leaf by PMD-leaf.

> 
>     #define pmd_leaf pmd_leaf
>     static inline bool pmd_leaf(pmd_t pmd)
>     {
>            return pmd_val(pmd) & _PMD_PAGE_8M);
>     }
> 
>     and then pmd_leaf_size to return _PMD_PAGE_8M.
> 
> This will help because on the ongoing effort of unifying hugetlb and
> getting rid of huge_ptep_get() [1], pagewalkers will stumble upon the
> 8mb-PMD as they do for regular PMDs.

But AFAIU, it won't work that simple, because *pmd is definitely not a 
PTE but still a pointer to a page table which contains the PTE.

> 
> Which means that they would be caught in the following code:
> 
>          ptl = pmd_huge_lock(pmd, vma);
>          if (ptl) {
>                  - 8MB hugepages will be handled here
>                  smaps_pmd_entry(pmd, addr, walk);
>                  spin_unlock(ptl);
>          }
>          /* pte stuff */
>          ...
> 
> where pmd_huge_lock is:
> 
>   static inline spinlock_t *pmd_huge_lock(pmd_t *pmd, struct vm_area_struct *vma)
>   {
>          spinlock_t *ptl = pmd_lock(vma->vm_mm, pmd);
> 
>          if (pmd_leaf(*pmd) || pmd_devmap(*pmd))
>                  return ptl;
>          spin_unlock(ptl);
>          return NULL;
>   }
> 
> So, since pmd_leaf() will return true for 8MB hugepages, we are fine,
> because anyway we want to perform pagetable operations on *that* PMD and
> not the ptes that are cont-mapped, which is different for e.g: 512K
> hugepages, where we perform it on pte level.

We still want to do the operation on the cont-PTE, in fact in both 4M 
page tables so that we cover the 8M. There is no operation to do on the 
PMD entry itself except that telling it is a 8M page table underneath.

> 
> So I would suggest that instead of this patch, we have one implementing pmd_leaf
> and pmd_leaf_size for 8Mb hugepages on power8xx, as that takes us closer to our goal of
> unifying hugetlb.

But then, how will it work to go down the PTE road ?

Christophe