Re: [PATCH v6 4/5] mm/sparse-vmemmap: improve memory savings for compound devmaps

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

On 2/24/22 05:54, Muchun Song wrote:
> On Thu, Feb 24, 2022 at 3:48 AM Joao Martins <joao.m.martins@oracle.com> wrote:
>> diff --git a/include/linux/mm.h b/include/linux/mm.h
>> index 5f549cf6a4e8..b0798b9c6a6a 100644
>> --- a/include/linux/mm.h
>> +++ b/include/linux/mm.h
>> @@ -3118,7 +3118,7 @@ p4d_t *vmemmap_p4d_populate(pgd_t *pgd, unsigned long addr, int node);
>>  pud_t *vmemmap_pud_populate(p4d_t *p4d, unsigned long addr, int node);
>>  pmd_t *vmemmap_pmd_populate(pud_t *pud, unsigned long addr, int node);
>>  pte_t *vmemmap_pte_populate(pmd_t *pmd, unsigned long addr, int node,
>> -                           struct vmem_altmap *altmap);
>> +                           struct vmem_altmap *altmap, struct page *block);
> 
> Have forgotten to update @block to @reuse here.
> 

Fixed.

> [...]
>> +
>> +static int __meminit vmemmap_populate_range(unsigned long start,
>> +                                           unsigned long end,
>> +                                           int node, struct page *page)
> 
> All of the users are passing a valid parameter of @page. This function
> will populate the vmemmap with the @page 

Yeap.

> and without memory
> allocations. So the @node parameter seems to be unnecessary.
> 
I am a little bit afraid of making this logic more fragile by removing node.
When we populate the the tail vmemmap pages, we *may need* to populate a new PMD page
. And we need the @node for those or anything preceeding that (even though it's highly
unlikely). It's just the PTE reuse that doesn't need node :(

> If you want to make this function more generic like
> vmemmap_populate_address() to handle memory allocations
> (the case of @page == NULL). I think vmemmap_populate_range()
> should add another parameter of `struct vmem_altmap *altmap`.

Oh, that's a nice cleanup/suggestion. I've moved vmemmap_populate_range() to be
used by vmemmap_populate_basepages(), and delete the duplication. I'll
adjust the second patch for this cleanup, to avoid moving the same code
over again between the two patches. I'll keep your Rb in the second patch, this is
the diff to this version:

diff --git a/mm/sparse-vmemmap.c b/mm/sparse-vmemmap.c
index 44cb77523003..1b30a82f285e 100644
--- a/mm/sparse-vmemmap.c
+++ b/mm/sparse-vmemmap.c
@@ -637,8 +637,9 @@ static pte_t * __meminit vmemmap_populate_address(unsigned long addr,
int node,
        return pte;
 }

-int __meminit vmemmap_populate_basepages(unsigned long start, unsigned long end,
-                                        int node, struct vmem_altmap *altmap)
+static int __meminit vmemmap_populate_range(unsigned long start,
+                                           unsigned long end, int node,
+                                           struct vmem_altmap *altmap)
 {
        unsigned long addr = start;
        pte_t *pte;
@@ -652,6 +653,12 @@ int __meminit vmemmap_populate_basepages(unsigned long start,
unsigned long end,
        return 0;
 }

+int __meminit vmemmap_populate_basepages(unsigned long start, unsigned long end,
+                                        int node, struct vmem_altmap *altmap)
+{
+       return vmemmap_populate_range(start, end, node, altmap);
+}
+
 struct page * __meminit __populate_section_memmap(unsigned long pfn,
                unsigned long nr_pages, int nid, struct vmem_altmap *altmap,
                struct dev_pagemap *pgmap)

Meanwhile I'll adjust the other callers of vmemmap_populate_range() in this patch.

> Otherwise, is it better to remove @node and rename @page to @reuse?

I've kept the @node for now, due to the concern explained earlier, but
renamed vmemmap_populate_range() to have its new argument be named @reuse.
Let me know if you disagree otherwise.

Thanks again for the comments, appreciate it!