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

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

On 2/1/2023 11:50 PM, Matthew Wilcox wrote:
> On Wed, Feb 01, 2023 at 04:17:37PM +0800, Yin Fengwei wrote:
>> Use do_set_pte_range() in filemap_map_folio_range(). Which
>> batched updates mm counter, rmap.
>>
>> With a self cooked will-it-scale.page_fault3 like app (change
>> file write fault to read fault) got 15% performance gain.
> 
> I'd suggest that you create a will-it-scale.page_fault4.  Anton
> is quite open to adding new variations of tests.
OK. I will do this.

> 
>> Perf data collected before/after the change:
>>   18.73%--page_add_file_rmap
>>           |
>>            --11.60%--__mod_lruvec_page_state
>>                      |
>>                      |--7.40%--__mod_memcg_lruvec_state
>>                      |          |
>>                      |           --5.58%--cgroup_rstat_updated
>>                      |
>>                       --2.53%--__mod_lruvec_state
>>                                 |
>>                                  --1.48%--__mod_node_page_state
>>
>>   9.93%--page_add_file_rmap_range
>>          |
>>           --2.67%--__mod_lruvec_page_state
>>                     |
>>                     |--1.95%--__mod_memcg_lruvec_state
>>                     |          |
>>                     |           --1.57%--cgroup_rstat_updated
>>                     |
>>                      --0.61%--__mod_lruvec_state
>>                                |
>>                                 --0.54%--__mod_node_page_state
>>
>> The running time of __mode_lruvec_page_state() is reduced a lot.
> 
> Nice.
> 
>> +++ b/mm/filemap.c
>> @@ -3364,11 +3364,22 @@ 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 ref_count = 0, count = 0, nr_mapped = 0;
>>  
>>  	do {
>> -		if (PageHWPoison(page))
>> +		if (PageHWPoison(page)) {
>> +			if (nr_mapped) {
>> +				vmf->pte -= nr_mapped;
>> +				do_set_pte_range(vmf, folio,
>> +						start + count - nr_mapped,
>> +						addr - nr_mapped * PAGE_SIZE,
>> +						nr_mapped);
>> +
>> +			}
>> +
>> +			nr_mapped = 0;
>>  			continue;
>> +		}
> 
> Having subtracted nr_mapped from vmf->pte, we then need to add it again.
> 
> But this is all too complicated.  What if we don't update vmf->pte
> each time around the loop?  ie something like this:
Thanks a lot for the suggestion. I like vmf->pte[count] a lot. Will
update the code accordingly in next version.


Regards
Yin, Fengwei

> 
> 	do {
> 		if (PageHWPoisoned(page))
> 			goto map;
>  		if (mmap_miss > 0)
>  			mmap_miss--;
> 		/*
> 		 * If there're PTE markers, we'll leave them to be handled
> 		 * in the specific fault path, and it'll prohibit the
> 		 * fault-around logic.
> 		 */
> 		if (!pte_none(vmf->pte[count]))
> 			goto map;
>  		if (vmf->address == addr + count * PAGE_SIZE)
>  			ret = VM_FAULT_NOPAGE;
> 		continue;
> map:
> 		if (count > 1) {
> 			do_set_pte_range(vmf, folio, start, addr, count - 1);
> 			folio_ref_add(folio, count - 1);
> 		}
> 		start += count;
> 		vmf->pte += count;
> 		addr += count * PAGE_SIZE;
> 		nr_pages -= count;
> 		count = 0;
> 	} while (page++, ++count < nr_pages);
> 
> 	if (count > 0) {
> 		do_set_pte_range(vmf, folio, start, addr, count);
> 		folio_ref_add(folio, count);
> 	} else {
> 		/* Make sure the PTE points to the correct page table */
> 		vmf->pte--;
> 	}