Re: [PATCH 5/6] mm/mremap: Use pmd/pud_poplulate to update page table entries

From: "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Nick Piggin <npiggin@gmail.com>, Linux-MM <linux-mm@kvack.org>,
	Kalesh Singh <kaleshsingh@google.com>,
	Joel Fernandes <joel@joelfernandes.org>,
	"Kirill A . Shutemov" <kirill@shutemov.name>,
	Andrew Morton <akpm@linux-foundation.org>,
	linuxppc-dev <linuxppc-dev@lists.ozlabs.org>
Subject: Re: [PATCH 5/6] mm/mremap: Use pmd/pud_poplulate to update page table entries
Date: Sun, 13 Jun 2021 14:36:13 +0530	[thread overview]
Message-ID: <87wnqy9lru.fsf@linux.ibm.com> (raw)
In-Reply-To: <CAHk-=wi+J+iodze9FtjM3Zi4j4OeS+qqbKxME9QN4roxPEXH9Q@mail.gmail.com>

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Thu, Jun 10, 2021 at 1:36 AM Aneesh Kumar K.V
> <aneesh.kumar@linux.ibm.com> wrote:
>>
>> @@ -306,8 +306,7 @@ static bool move_normal_pud(struct vm_area_struct *vma, unsigned long old_addr,
>>
>>         VM_BUG_ON(!pud_none(*new_pud));
>>
>> -       /* Set the new pud */
>> -       set_pud_at(mm, new_addr, new_pud, pud);
>> +       pud_populate(mm, new_pud, (pmd_t *)pud_page_vaddr(pud));
>>         flush_tlb_range(vma, old_addr, old_addr + PUD_SIZE);
>>         if (new_ptl != old_ptl)
>>                 spin_unlock(new_ptl);
>
> That "(pmd_t *)pud_page_vaddr(pud)" looks a bit odd and doesn't match
> the pattern.
>
> Should we perhaps have a wrapper for it? Maybe called "pud_pgtable()",
> the same way we have pmd_pgtable()?

IIUC the reason why we do have pmd_pgtable() is that pgtable_t type
is arch dependent. On some architecture it is pte_t * and on the other
struct page *. The reason being highmem and level 4 page table can
be located in highmem. 

The above is not true with the level 3 table and hence we didn't add an
extra type to point to level 3 table.

We could add pud_pgtable(), but then it will essentially be a typecast
as I did above?

Even if we want to do that, that should be done as a separate patch than
this series?

>
> And that wrapper would be good to have a comment or two about what it
> actually is. The same way that pmd_pgtable() should but doesn't ;(
>
>             Linus