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

[Muchun Song <songmuchun@bytedance.com>]

On Thu, Feb 24, 2022 at 7:47 PM Joao Martins <joao.m.martins@oracle.com> wrote:
>
> 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 :(

Agree. So I suggest adding @altmap to vmemmap_populate_range() like
you have done as follows.

>
> > 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.

LGTM.

>
> > 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.

Make sense.