Re: [RFC PATCH v3 3/4] mm: add do_set_pte_range()

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

On 2/3/2023 10:30 PM, Chih-En Lin wrote:
> On Fri, Feb 03, 2023 at 09:38:15PM +0800, Yin, Fengwei wrote:
>>
>>
>> On 2/3/2023 9:32 PM, Chih-En Lin wrote:
>>> On Fri, Feb 03, 2023 at 09:16:35PM +0800, Yin Fengwei wrote:
>>>> do_set_pte_range() allows to setup page table entries for a
>>>> specific range. It calls folio_add_file_rmap_range() to take
>>>> advantage of batched rmap update for large folio.
>>>>
>>>> Signed-off-by: Yin Fengwei <fengwei.yin@intel.com>
>>>> ---
>>>>  include/linux/mm.h |  3 +++
>>>>  mm/filemap.c       |  1 -
>>>>  mm/memory.c        | 59 ++++++++++++++++++++++++++++++----------------
>>>>  3 files changed, 42 insertions(+), 21 deletions(-)
>>>>
>>>> diff --git a/include/linux/mm.h b/include/linux/mm.h
>>>> index d6f8f41514cc..93192f04b276 100644
>>>> --- a/include/linux/mm.h
>>>> +++ b/include/linux/mm.h
>>>> @@ -1162,6 +1162,9 @@ static inline pte_t maybe_mkwrite(pte_t pte, struct vm_area_struct *vma)
>>>>  
>>>>  vm_fault_t do_set_pmd(struct vm_fault *vmf, struct page *page);
>>>>  void do_set_pte(struct vm_fault *vmf, struct page *page, unsigned long addr);
>>>> +void do_set_pte_range(struct vm_fault *vmf, struct folio *folio,
>>>> +		unsigned long addr, pte_t *pte,
>>>> +		unsigned long start, unsigned int nr);
>>>>  
>>>>  vm_fault_t finish_fault(struct vm_fault *vmf);
>>>>  vm_fault_t finish_mkwrite_fault(struct vm_fault *vmf);
>>>> diff --git a/mm/filemap.c b/mm/filemap.c
>>>> index f444684db9f2..74046a3a0ff5 100644
>>>> --- a/mm/filemap.c
>>>> +++ b/mm/filemap.c
>>>> @@ -3386,7 +3386,6 @@ static vm_fault_t filemap_map_folio_range(struct vm_fault *vmf,
>>>>  
>>>>  		ref_count++;
>>>>  		do_set_pte(vmf, page, addr);
>>>> -		update_mmu_cache(vma, addr, vmf->pte);
>>>>  	} while (vmf->pte++, page++, addr += PAGE_SIZE, ++count < nr_pages);
>>>>  
>>>>  	/* Restore the vmf->pte */
>>>> diff --git a/mm/memory.c b/mm/memory.c
>>>> index 7a04a1130ec1..3754b2ef166a 100644
>>>> --- a/mm/memory.c
>>>> +++ b/mm/memory.c
>>>> @@ -4257,36 +4257,58 @@ vm_fault_t do_set_pmd(struct vm_fault *vmf, struct page *page)
>>>>  }
>>>>  #endif
>>>>  
>>>> -void do_set_pte(struct vm_fault *vmf, struct page *page, unsigned long addr)
>>>> +void do_set_pte_range(struct vm_fault *vmf, struct folio *folio,
>>>> +		unsigned long addr, pte_t *pte,
>>>> +		unsigned long start, unsigned int nr)
>>>>  {
>>>>  	struct vm_area_struct *vma = vmf->vma;
>>>>  	bool uffd_wp = pte_marker_uffd_wp(vmf->orig_pte);
>>>>  	bool write = vmf->flags & FAULT_FLAG_WRITE;
>>>> +	bool cow = write && !(vma->vm_flags & VM_SHARED);
>>>
>>> Why don't use is_cow_mapping()?
>>> Is there anything messed up with VM_MAYWRITE?
>> My understanding is it's not related with the mapping. It's related with
>> what operation triggers fault here. Say, if it's a writable mapping, and
>> if the read operation triggers fault here, no cow or maybe_mkwrite() needed
>> here. Thanks.
> 
> Sorry, I didn't describe the thing properly.
> It makes sense for the relationship with the write/read fault.
> I'm just wondering if "!(vma->vm_flags & VM_SHARED)" is enough to determine
> the COW page? And, I also found it in do_fault().
> 
> Like, copy_present_pte() use is_cow_mapping() for COW page and
> "vm_flags & VM_SHARED" for shared mapping.
> 
> So, I looked up the commit that introduced the is_cow_mapping(),
> 67121172f9753 ("Allow arbitrary read-only shared pfn-remapping too").
> 
> Here is the commit message:
> "
>     The VM layer (for historical reasons) turns a read-only shared mmap into
>     a private-like mapping with the VM_MAYWRITE bit clear.  Thus checking
>     just VM_SHARED isn't actually sufficient.
>     
>     So use a trivial helper function for the cases where we wanted to inquire
>     if a mapping was COW-like or not.
> "
> 
> hmm, but it is v2.6.15.
> So is "vm_flags & VM_SHARED" enough to check the COW page now?
Thanks for the detail info here. Yes. VM_MAYWRITE bit of vma->vm_flags needs
be checked for COW also.

In the page fault path, the VM_MAYWRITE bit was checked in sanitize_fault_flags():
                /* Write faults on read-only mappings are impossible ... */
                if (WARN_ON_ONCE(!(vma->vm_flags & VM_MAYWRITE)))
                        return VM_FAULT_SIGSEGV;

and bail out early if it's write fault and no VM_MAYWRITE.

My understanding is that sanitize_fault_flags() is called first before hit
do_set_pte()/do_set_pte_range().


Regards
Yin, Fengwei

> 
> Thanks,
> Chih-En Lin