Re: [PATCH v4 08/14] mm/gup: grab head page refcount once for group of subpages

[Joao Martins <joao.m.martins@oracle.com>]

On 8/27/21 5:25 PM, Jason Gunthorpe wrote:
> On Fri, Aug 27, 2021 at 03:58:13PM +0100, Joao Martins wrote:
> 
>>  #if defined(CONFIG_ARCH_HAS_PTE_DEVMAP) && defined(CONFIG_TRANSPARENT_HUGEPAGE)
>>  static int __gup_device_huge(unsigned long pfn, unsigned long addr,
>>  			     unsigned long end, unsigned int flags,
>>  			     struct page **pages, int *nr)
>>  {
>> -	int nr_start = *nr;
>> +	int refs, nr_start = *nr;
>>  	struct dev_pagemap *pgmap = NULL;
>>  	int ret = 1;
>>  
>>  	do {
>> -		struct page *page = pfn_to_page(pfn);
>> +		struct page *head, *page = pfn_to_page(pfn);
>> +		unsigned long next = addr + PAGE_SIZE;
>>  
>>  		pgmap = get_dev_pagemap(pfn, pgmap);
>>  		if (unlikely(!pgmap)) {
>> @@ -2252,16 +2265,25 @@ static int __gup_device_huge(unsigned long pfn, unsigned long addr,
>>  			ret = 0;
>>  			break;
>>  		}
>> -		SetPageReferenced(page);
>> -		pages[*nr] = page;
>> -		if (unlikely(!try_grab_page(page, flags))) {
>> -			undo_dev_pagemap(nr, nr_start, flags, pages);
>> +
>> +		head = compound_head(page);
>> +		/* @end is assumed to be limited at most one compound page */
>> +		if (PageHead(head))
>> +			next = end;
>> +		refs = record_subpages(page, addr, next, pages + *nr);
>> +
>> +		SetPageReferenced(head);
>> +		if (unlikely(!try_grab_compound_head(head, refs, flags))) {
>> +			if (PageHead(head))
>> +				ClearPageReferenced(head);
>> +			else
>> +				undo_dev_pagemap(nr, nr_start, flags, pages);
>>  			ret = 0;
>>  			break;
> 
> Why is this special cased for devmap?
> 
> Shouldn't everything processing pud/pmds/etc use the same basic loop
> that is similar in idea to the 'for_each_compound_head' scheme in
> unpin_user_pages_dirty_lock()?
> 
> Doesn't that work for all the special page type cases here?

We are iterating over PFNs to create an array of base pages (regardless of page table
type), rather than iterating over an array of pages to work on. Given that all these gup
functions already give you the boundary (end of pmd or end of pud, etc) then we just need
to grab the ref to pgmap and head page and save the tails. But sadly we need to handle the
base page case which is why there's this outer loop exists sadly. If it was just head
pages we wouldn't need the outer loop (and hence no for_each_compound_head, like the
hugetlb variant).

But maybe I am being dense and you just mean to replace the outer loop with
for_each_compound_range(). I am a little stuck on the part that I anyways need to record
back the tail pages when iterating over the (single) head page. So I feel that it wouldn't
bring that much improvement, unless I missed your point.

	Joao