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
