On Wed, Jul 28, 2021 at 8:56 AM Joao Martins <joao.m.martins@xxxxxxxxxx> wrote:
>
> On 7/28/21 8:28 AM, Dan Williams wrote:
> > On Wed, Jul 14, 2021 at 12:36 PM Joao Martins <joao.m.martins@xxxxxxxxxx> wrote:
> >>
> >> + /*
> >> + * With compound page geometry and when struct pages are stored in ram
> >> + * (!altmap) most tail pages are reused. Consequently, the amount of
> >> + * unique struct pages to initialize is a lot smaller that the total
> >> + * amount of struct pages being mapped.
> >> + * See vmemmap_populate_compound_pages().
> >> + */
> >> + if (!altmap)
> >> + nr_pages = min_t(unsigned long, nr_pages,
> >
> > What's the scenario where nr_pages is < 128? Shouldn't alignment
> > already be guaranteed?
> >
> Oh yeah, that's right.
>
> >> + 2 * (PAGE_SIZE/sizeof(struct page)));
> >
> >
> >> +
> >> __SetPageHead(page);
> >>
> >> for (i = 1; i < nr_pages; i++) {
> >> @@ -6657,7 +6669,7 @@ void __ref memmap_init_zone_device(struct zone *zone,
> >> continue;
> >>
> >> memmap_init_compound(page, pfn, zone_idx, nid, pgmap,
> >> - pfns_per_compound);
> >> + altmap, pfns_per_compound);
> >
> > This feels odd, memmap_init_compound() doesn't really care about
> > altmap, what do you think about explicitly calculating the parameters
> > that memmap_init_compound() needs and passing them in?
> >
> > Not a strong requirement to change, but take another look at let me know.
> >
>
> Yeah, memmap_init_compound() indeed doesn't care about @altmap itself -- but a previous
> comment was to abstract this away in memmap_init_compound() given the mix of complexity in
> memmap_init_zone_device() PAGE_SIZE geometry case and the compound case:
>
> https://lore.kernel.org/linux-mm/CAPcyv4gtSqfmuAaX9cs63OvLkf-h4B_5fPiEnM9p9cqLZztXpg@xxxxxxxxxxxxxx/
>
> Before this was called @ntails above and I hide that calculation in memmap_init_compound().
>
> But I can move this back to the caller:
>
> memmap_init_compound(page, pfn, zone_idx, nid, pgmap,
> (!altmap ? 2 * (PAGE_SIZE/sizeof(struct page))) : pfns_per_compound);
>
> Or with another helper like:
>
> #define compound_nr_pages(__altmap, __nr_pages) \
> (!__altmap ? 2 * (PAGE_SIZE/sizeof(struct page))) : __nr_pages);
>
> memmap_init_compound(page, pfn, zone_idx, nid, pgmap,
> compound_nr_pages(altmap, pfns_per_compound));
I like the helper, but I'd go further to make it a function with a
comment that it is a paired / mild layering violation with explicit
knowledge of how the sparse_vmemmap() internals handle compound pages
in the presence of an altmap. I.e. if someone later goes to add altmap
support, leave them a breadcrumb that they need to update both
locations.
