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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 7/28/21 5:08 PM, Dan Williams wrote:
> 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.
> 
OK, got it.



[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux