Re: [PATCH v1 09/11] mm/page_alloc: reuse tail struct pages for compound pagemaps

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

Just like patch8 lets rename this "optimize memmap_init_zone_device()
for compound page maps"


On Thu, Mar 25, 2021 at 4:10 PM Joao Martins <joao.m.martins@oracle.com> wrote:
>
> When a pgmap @align is set,

Rewrite this to talk about geometry rather than align per previous
patch comments.

> all pages are mapped at a given huge page
> alignment and thus uses compound pages to describe them as opposed to a

s/thus uses/use/

> struct per 4K.
>
> With @align > PAGE_SIZE and when struct pages are stored in ram
> (!altmap) most tail pages are reused. Consequently, the amount of unique
> struct pages is a lot smaller that the total amount of struct pages
> being mapped.
>
> When struct pages are initialize in memmap_init_zone_device, make
> sure that only unique struct pages are initialized i.e. the first 2
> 4K pages per @align which means 128 struct pages, instead of 32768 for
> 2M @align or 262144 for a 1G @align.

This is the important detail to put at the beginning and then follow
with the detailed description, i.e. something like:

"Currently memmap_init_zone_device() ends up initializing 32768 pages
when it only needs to initialize 128 given tail page reuse. That
number is worse with 1GB compound page geometries, 262144 instead of
128. Update memmap_init_zone_device() to skip redundant
initialization, details below:"

> Keep old behaviour with altmap given that it doesn't support reusal
> of tail vmemmap areas.

I think I like how you described this earlier as the (!altmap) path. So:

"The !altmap path is left alone since it does not support compound
page map geometries."

>
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> ---
>  mm/page_alloc.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 3a77f9e43f3a..948dfad6754b 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -6277,6 +6277,8 @@ static void __ref __init_zone_device_page(struct page *page, unsigned long pfn,
>         }
>  }
>
> +#define MEMMAP_NR_PAGES        (2 * (PAGE_SIZE/sizeof(struct page)))

This seems too generic a name for something that is specific to the
compound page init case... more below.

> +
>  void __ref memmap_init_zone_device(struct zone *zone,
>                                    unsigned long start_pfn,
>                                    unsigned long nr_pages,
> @@ -6287,6 +6289,7 @@ void __ref memmap_init_zone_device(struct zone *zone,
>         struct vmem_altmap *altmap = pgmap_altmap(pgmap);
>         unsigned int pfn_align = pgmap_pfn_align(pgmap);
>         unsigned int order_align = order_base_2(pfn_align);
> +       unsigned long ntails = min_t(unsigned long, pfn_align, MEMMAP_NR_PAGES);

I'd rather delay this to a specific "if (compound)" branch below
because compound init is so different from the base page case.

>         unsigned long zone_idx = zone_idx(zone);
>         unsigned long start = jiffies;
>         int nid = pgdat->node_id;
> @@ -6302,6 +6305,7 @@ void __ref memmap_init_zone_device(struct zone *zone,
>         if (altmap) {
>                 start_pfn = altmap->base_pfn + vmem_altmap_offset(altmap);
>                 nr_pages = end_pfn - start_pfn;
> +               ntails = pfn_align;

Why would the altmap case need to init ntails, won't the inner loop of
compound page setup be skipped?

>         }

Given all of the above I'm wondering if there should be a new
"compound specific" flavor of this routine rather than trying to
cleverly inter mingle the old path with the new. This is easier
because you have already done the work in previous patches to break
this into helpers. So just have memmap_init_zone_device() do it the
"old" way and memmap_init_compound() handle all the tail page init +
optimizations.