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

On 7/28/21 7:03 PM, Dan Williams wrote:

On Wed, Jul 28, 2021 at 8:36 AM Joao Martins [off-list ref] wrote:
[..]

quoted

+/*
+ * 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?

Yes.

[..]

quoted

[...] 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);
+

Maybe I can drop the geometry_size and just do here:

if (PHYS_PFN(offset) % pgmap_geometry(pgmap) > PAGES_PER_SECTION)

and thus drop the geometry_size variable.

quoted

+       /* 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.

OK.