Re: [PATCH v2 2/2] mm/hugetlb: refactor subpage recording

[Mike Kravetz]

On 2/11/21 12:47 PM, Zi Yan wrote:
> On 28 Jan 2021, at 16:53, Mike Kravetz wrote:
> 
>> On 1/28/21 10:26 AM, Joao Martins wrote:
>>> For a given hugepage backing a VA, there's a rather ineficient
>>> loop which is solely responsible for storing subpages in GUP
>>> @pages/@vmas array. For each subpage we check whether it's within
>>> range or size of @pages and keep increment @pfn_offset and a couple
>>> other variables per subpage iteration.
>>>
>>> Simplify this logic and minimize the cost of each iteration to just
>>> store the output page/vma. Instead of incrementing number of @refs
>>> iteratively, we do it through pre-calculation of @refs and only
>>> with a tight loop for storing pinned subpages/vmas.
>>>
>>> Additionally, retain existing behaviour with using mem_map_offset()
>>> when recording the subpages for configurations that don't have a
>>> contiguous mem_map.
>>>
>>> pinning consequently improves bringing us close to
>>> {pin,get}_user_pages_fast:
>>>
>>>   - 16G with 1G huge page size
>>>   gup_test -f /mnt/huge/file -m 16384 -r 30 -L -S -n 512 -w
>>>
>>> PIN_LONGTERM_BENCHMARK: ~12.8k us -> ~5.8k us
>>> PIN_FAST_BENCHMARK: ~3.7k us
>>>
>>> Signed-off-by: Joao Martins <joao.m.martins@xxxxxxxxxx>
>>> ---
>>>  mm/hugetlb.c | 49 ++++++++++++++++++++++++++++---------------------
>>>  1 file changed, 28 insertions(+), 21 deletions(-)
>>
>> Thanks for updating this.
>>
>> Reviewed-by: Mike Kravetz <mike.kravetz@xxxxxxxxxx>
>>
>> I think there still is an open general question about whether we can always
>> assume page structs are contiguous for really big pages.  That is outside
> 
> I do not think page structs need to be contiguous, but PFNs within a big page
> need to be contiguous, at least based on existing code like mem_map_offset() we have.

Thanks for looking Zi,
Yes, PFNs need to be contiguous.  Also, as you say page structs do not need
to be contiguous.  The issue is that there is code that assumes page structs
are contiguous for gigantic pages.  hugetlb code does not make this assumption
and does a pfn_to_page() when looping through page structs for gigantic pages.

I do not believe this to be a huge issue.  In most cases CONFIG_VIRTUAL_MEM_MAP
is defined and struct pages can be accessed contiguously.  I 'think' we could
run into problems with CONFIG_SPARSEMEM and without CONFIG_VIRTUAL_MEM_MAP
and doing hotplug operations.  However, I still need to look into more.
-- 
Mike Kravetz

> The assumption seems valid according to the existing big page allocation methods,
> which use alloc_contig_pages() at the end of the day. alloc_contig_pages()
> calls pfn_range_valid_contig() to make sure all PFNs are contiguous.
> On the other hand, the buddy allocator only merges contiguous PFNs, so there
> will be no problem even if someone configures the buddy allocator to allocate
> gigantic pages.
> 
> Unless someone comes up with some fancy way of making page allocations from
> contiguous page structs in SPARSEMEM_VMEMMAP case, where non-contiguous
> PFNs with contiguous page structs are possible, or out of any adjacent
> pages in !SPARSEMEM_VMEMMAP case, where non-contiguous page structs
> and non-contiguous PFNs are possible, we should be good.
> 
> 
> —
> Best Regards,
> Yan Zi
>