RE: [RFC v1 1/3] mm/mmu_notifier: Add a new notifier for mapping updates (new pages)

["Kasireddy, Vivek" <vivek.kasireddy@intel.com>]

Hi Jason,

> 
> On Wed, Jul 19, 2023 at 12:05:29AM +0000, Kasireddy, Vivek wrote:
> 
> > > If there is no change to the PTEs then it is hard to see why this
> > > would be part of a mmu_notifier.
> > IIUC, the PTEs do get changed but only when a new page is faulted in.
> > For shmem, it looks like the PTEs are updated in handle_pte_fault()
> > after shmem_fault() gets called and for hugetlbfs, this seems to
> > happen in hugetlb_fault().
> 
> That sounds about right
> 
> > Instead of introducing a new notifier, I did think about reusing
> > (or overloading) .change_pte() but I did not fully understand the impact
> > it would have on KVM, the only user of .change_pte().
> 
> Yes, change_pte will be called, I think, but under various locks. 
AFAICT, change_pte does not seem to get called in my use-case either
during invalidate or when a new page is faulted in.

>Why would you need to change it?
What I meant to say is instead of adding a new notifier for mapping updates,
I initially considered just calling change_pte() when a new page is faulted in
but I was concerned that doing so might adversely impact existing users (of
change_pte) such as KVM.

> 
> What you are doing here doesn't make any sense within the design of
> mmu_notifiers, eg:
> 
> > @ -2164,8 +2165,12 @@ static vm_fault_t shmem_fault(struct vm_fault
> *vmf)
> >  				  gfp, vma, vmf, &ret);
> >  	if (err)
> >  		return vmf_error(err);
> > -	if (folio)
> > +	if (folio) {
> >  		vmf->page = folio_file_page(folio, vmf->pgoff);
> > +		if (ret == VM_FAULT_LOCKED)
> > +			mmu_notifier_update_mapping(vma->vm_mm, vmf-
> >address,
> > +						    page_to_pfn(vmf->page));
> > +	}
> >  	return ret;
> 
> Hasn't even updated the PTEs yet, but it is invoking a callback??
I was counting on the fragile assumption that once we have a valid page,
the PTE would be eventually updated after shmem_fault(), which doesn't
make sense in retrospect. Instead, would something like below be OK?
@@ -5234,6 +5237,14 @@ vm_fault_t handle_mm_fault(struct vm_area_struct *vma, unsigned long address,

        lru_gen_exit_fault();

+       if (vma_is_shmem(vma) || is_vm_hugetlb_page(vma)) {
+               if (!follow_pte(vma->vm_mm, address, &ptep, &ptl)) {
+                       pfn = pte_pfn(ptep_get(ptep));
+                       pte_unmap_unlock(ptep, ptl);
+                       mmu_notifier_update_mapping(vma->vm_mm, address, pfn);
+               }
+       }


Thanks,
Vivek

> 
> Jason