Re: [PATCH v1 07/11] mm/sparse-vmemmap: populate compound pagemaps

[Joao Martins <joao.m.martins@oracle.com>]

On 5/13/21 7:45 PM, Joao Martins wrote:
> On 5/10/21 8:19 PM, Dan Williams wrote:
>> On Thu, May 6, 2021 at 4:02 AM Joao Martins <joao.m.martins@oracle.com> wrote:
>> [..]
>>>>> +static pte_t * __meminit vmemmap_lookup_address(unsigned long addr)
>>>>
>>>> I think this can be replaced with a call to follow_pte(&init_mm...).
>>>
>>> Ah, of course! That would shorten things up too.
>>
>> Now that I look closely, I notice that function disclaims being
>> suitable as a general purpose pte lookup utility. 
>> If it works for you,
>> great, but if not I think it's past time to create such a helper. I
>> know Ira is looking for one, and I contributed to the proliferation
>> when I added dev_pagemap_mapping_shift().
>>
> There's also x86 equivalents, like lookup_address() and lookup_address_in_pgd().
> These two don't differ that much from vmemmap_lookup_address().
> 
> I can move this to an internal place e.g. mm/internal.h
> 
> The patch after this one, changes vmemmap_lookup_address() to stop at the PMD (to reuse
> that across the next sections).
> 
Thinking again on this particular comment, but more on the actual need for the lookup
function. It is very specific to 1G geometry case being spread over multiple sections.

Given section population *needs* to succeed for the followup sections to continue to be
populated, perhaps we simplify this a whole lot e.g. below comparing this patch before and
after.

diff --git a/mm/sparse-vmemmap.c b/mm/sparse-vmemmap.c
index 3d671e3e804d..c06796fcc77d 100644
--- a/mm/sparse-vmemmap.c
+++ b/mm/sparse-vmemmap.c
@@ -554,37 +554,6 @@ static inline int __meminit vmemmap_populate_page(unsigned long addr,
int node,
        return vmemmap_populate_address(addr, node, NULL, NULL, page);
 }

-static pte_t * __meminit vmemmap_lookup_address(unsigned long addr)
-{
-       pgd_t *pgd;
-       p4d_t *p4d;
-       pud_t *pud;
-       pmd_t *pmd;
-       pte_t *pte;
-
-       pgd = pgd_offset_k(addr);
-       if (pgd_none(*pgd))
-               return NULL;
-
-       p4d = p4d_offset(pgd, addr);
-       if (p4d_none(*p4d))
-               return NULL;
-
-       pud = pud_offset(p4d, addr);
-       if (pud_none(*pud))
-               return NULL;
-
-       pmd = pmd_offset(pud, addr);
-       if (pmd_none(*pmd))
-               return NULL;
-
-       pte = pte_offset_kernel(pmd, addr);
-       if (pte_none(*pte))
-               return NULL;
-
-       return pte;
-}
-
 static int __meminit vmemmap_populate_compound_pages(unsigned long start_pfn,
                                                     unsigned long start,
                                                     unsigned long end, int node,
@@ -605,8 +574,10 @@ static int __meminit vmemmap_populate_compound_pages(unsigned long
start_pfn,
        offset = PFN_PHYS(start_pfn) - pgmap->ranges[pgmap->nr_range].start;
        if (!IS_ALIGNED(offset, pgmap_geometry(pgmap)) &&
            pgmap_geometry(pgmap) > SUBSECTION_SIZE) {
-               pte_t *ptep = vmemmap_lookup_address(start - PAGE_SIZE);
+               pte_t *ptep;

+               addr = start - PAGE_SIZE;
+               ptep = pte_offset_kernel(pmd_off_k(addr), addr);
                if (!ptep)
                        return -ENOMEM;

The 'if (!ptep)' cannot happen in pratice AFAIU so could remove that as well.

Thoughts?