Re: [PATCH v3 08/14] mm/sparse-vmemmap: populate compound pagemaps

[Dan Williams <dan.j.williams@intel.com>]

On Wed, Jul 28, 2021 at 8:36 AM Joao Martins <joao.m.martins@oracle.com> wrote:
[..]
> +/*
> + * For compound pages bigger than section size (e.g. x86 1G compound
> + * pages with 2M subsection size) fill the rest of sections as tail
> + * pages.
> + *
> + * Note that memremap_pages() resets @nr_range value and will increment
> + * it after each range successful onlining. Thus the value or @nr_range
> + * at section memmap populate corresponds to the in-progress range
> + * being onlined here.
> + */
> +static bool compound_section_index(unsigned long start_pfn,

Oh, I was thinking this would return the actual Nth index number for
the section within the compound page. A bool is ok too, but then the
function name would be something like:

reuse_compound_section()

...right?


[..]
> [...] And here's compound_section_tail_huge_page() (for the last patch in the series):
>
>
> @@ -690,6 +727,33 @@ static struct page * __meminit compound_section_tail_page(unsigned
> long addr)
>         return pte_page(*ptep);
>  }
>
> +static struct page * __meminit compound_section_tail_huge_page(unsigned long addr,
> +                               unsigned long offset, struct dev_pagemap *pgmap)
> +{
> +       unsigned long geometry_size = pgmap_geometry(pgmap) << PAGE_SHIFT;
> +       pmd_t *pmdp;
> +
> +       addr -= PAGE_SIZE;
> +
> +       /*
> +        * Assuming sections are populated sequentially, the previous section's
> +        * page data can be reused.
> +        */
> +       pmdp = pmd_off_k(addr);
> +       if (!pmdp)
> +               return ERR_PTR(-ENOMEM);
> +
> +       /*
> +        * Reuse the tail pages vmemmap pmd page
> +        * See layout diagram in Documentation/vm/vmemmap_dedup.rst
> +        */
> +       if (offset % geometry_size > PFN_PHYS(PAGES_PER_SECTION))
> +               return pmd_page(*pmdp);
> +
> +       /* No reusable PMD fallback to PTE tail page*/
> +       return NULL;
> +}
> +
>  static int __meminit vmemmap_populate_compound_pages(unsigned long start_pfn,
>                                                      unsigned long start,
>                                                      unsigned long end, int node,
> @@ -697,14 +761,22 @@ static int __meminit vmemmap_populate_compound_pages(unsigned long
> start_pfn,
>  {
>         unsigned long offset, size, addr;
>
> -       if (compound_section_index(start_pfn, pgmap)) {
> -               struct page *page;
> +       if (compound_section_index(start_pfn, pgmap, &offset)) {
> +               struct page *page, *hpage;
> +
> +               hpage = compound_section_tail_huge_page(addr, offset);
> +               if (IS_ERR(hpage))
> +                       return -ENOMEM;
> +               else if (hpage)

No need for "else" after return... other than that these helpers and
this arrangement looks good to me.