Re: [RFC PATCH v4 4/4] filemap: batched update mm counter,rmap when map file folio

["Yin, Fengwei" <fengwei.yin@intel.com>]

On 2/6/2023 10:34 PM, Matthew Wilcox wrote:
> On Mon, Feb 06, 2023 at 10:06:39PM +0800, Yin Fengwei wrote:
>> @@ -3354,11 +3354,12 @@ static vm_fault_t filemap_map_folio_range(struct vm_fault *vmf,
>>  	struct file *file = vma->vm_file;
>>  	struct page *page = folio_page(folio, start);
>>  	unsigned int mmap_miss = READ_ONCE(file->f_ra.mmap_miss);
>> -	unsigned int ref_count = 0, count = 0;
>> +	unsigned int mapped = 0;
>> +	pte_t *pte = vmf->pte;
>>  
>>  	do {
>>  		if (PageHWPoison(page))
>> -			continue;
>> +			goto map;
>>  
>>  		if (mmap_miss > 0)
>>  			mmap_miss--;
>> @@ -3368,20 +3369,34 @@ static vm_fault_t filemap_map_folio_range(struct vm_fault *vmf,
>>  		 * handled in the specific fault path, and it'll prohibit the
>>  		 * fault-around logic.
>>  		 */
>> -		if (!pte_none(*vmf->pte))
>> -			continue;
>> +		if (!pte_none(pte[mapped]))
>> +			goto map;
> 
> I see what you're trying to do here, but do_set_pte_range() uses the
> pte from vmf->pte.  Perhaps best to save it at the beginning of the
> function and restore it at the end.  ie:
> 
> 	pte_t *old_ptep = vmf->pte;
> 
> 	} while (vmf->pte++);
> 
> 	vmf->pte = old_ptep;
Yes. This also works.

> 
> The only other thing that bugs me about this patch is the use of
> 'mapped' as the variable name.  It's in the past tense, not the future
> tense, so it gives me the wrong impression about what it's counting.
> While we could use 'tomap', that's kind of clunky.  'count' or even just
> 'i' would be better.
OK. Will use count in next version.


Regards
Yin, Fengwei