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));

Reply via email to