Re: [RFC PATCH 0/4] KVM: x86/mmu: Rework marking folios dirty/accessed

[Paolo Bonzini]

On Fri, Apr 5, 2024 at 8:53 AM David Hildenbrand <david@xxxxxxxxxx> wrote:
> >       mmu_notifier_invalidate_range_start(&range);
> >       tlb_start_vma(&tlb, vma);
> >       walk_page_range(vma->vm_mm, range.start, range.end,
> >                       &madvise_free_walk_ops, &tlb);
> >       tlb_end_vma(&tlb, vma);
> >       mmu_notifier_invalidate_range_end(&range);
> >
>
> Indeed, we do setup the MMU notifier invalidation. We do the start/end
> ... I was looking for PTE notifications.
>
> I spotted the set_pte_at(), not a set_pte_at_notify() like we do in
> other code. Maybe that's not required here (digging through
> documentation I'm still left clueless). [...]
> Absolutely unclear to me when we *must* to use it, or if it is. Likely
> its a pure optimization when we're *changing* a PTE.

Yes, .change_pte() is just an optimization. The original point of it
was for KSM, so that KVM could flip the sPTE to a new location without
first zapping it. At the time there was also an .invalidate_page()
callback, and both of them were *not* bracketed by calls to
mmu_notifier_invalidate_range_{start,end}()

Later on, both callbacks were changed to occur within an
invalidate_range_start/end() block.

Commit 6bdb913f0a70 ("mm: wrap calls to set_pte_at_notify with
invalidate_range_start and invalidate_range_end", 2012-10-09) did so
for .change_pte(). The reason to do so was a bit roundabout, namely to
allow sleepable .invalidate_page() hooks (because .change_pte() is not
sleepable and at the time .invalidate_page() was used as a fallback
for .change_pte()).

This however made KVM's usage of the .change_pte() callback completely
moot, because KVM unmaps the sPTEs during .invalidate_range_start()
and therefore .change_pte() has no hope of succeeding.

(Commit 369ea8242c0f ("mm/rmap: update to new mmu_notifier semantic
v2", 2017-08-31) is where the other set of non-bracketed calls to MMU
notifier callbacks was changed; calls to
mmu_notifier_invalidate_page() were replaced by calls to
mmu_notifier_invalidate_range(), bracketed by calls to
mmu_notifier_invalidate_range_{start,end}()).

Since KVM is the only user of .change_pte(), we can remove
change_pte() and set_pte_at_notify() completely.

Paolo

> __replace_page() uses it with __replace_page() when replacing the mapped
> page. And migrate_vma_insert_page() uses it with MMU_NOTIFY_MIGRATE.
>
> Other code (e.g., khugepaged) doesn't seem to use it as well.
>
> ... so I guess it's fine? Confusing.
>
> --
> Cheers,
>
> David / dhildenb
>