On Wed, Jul 28, 2021 at 8:56 AM Joao Martins <[email protected]> wrote: > > On 7/28/21 8:28 AM, Dan Williams wrote: > > On Wed, Jul 14, 2021 at 12:36 PM Joao Martins <[email protected]> > > 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_5fpienm9p9cqlzzt...@mail.gmail.com/ > > 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.
