Re: [PATCH 0/5] KVM: x86: Clean up rmap zap helpers

[Sean Christopherson]

On Thu, Jul 14, 2022, Sean Christopherson wrote:
> On Thu, Jul 14, 2022, Paolo Bonzini wrote:
> > On 7/12/22 03:55, Sean Christopherson wrote:
> > > Clean up the rmap helpers (mostly renames) to yield a more coherent set of
> > > APIs, and to purge the irritating and inconsistent "rmapp" (p is for pointer)
> > > nomenclature.
> > > 
> > > Patch 1 is a tangentially related fix for a benign bug.
> > > 
> > > Sean Christopherson (5):
> > >    KVM: x86/mmu: Return a u64 (the old SPTE) from
> > >      mmu_spte_clear_track_bits()
> > >    KVM: x86/mmu: Rename rmap zap helpers to better show relationships
> > >    KVM: x86/mmu: Remove underscores from __pte_list_remove()
> > >    KVM: x86/mmu: Use innermost rmap zap helper when recycling rmaps
> > >    KVM: x86/mmu: Drop the "p is for pointer" from rmap helpers
> > > 
> > >   arch/x86/kvm/mmu/mmu.c | 73 +++++++++++++++++++++---------------------
> > >   1 file changed, 36 insertions(+), 37 deletions(-)
> > > 
> > > 
> > > base-commit: b9b71f43683ae9d76b0989249607bbe8c9eb6c5c
> > 
> > I'm not sure I dig the ____, I'll take a closer look tomorrow or next week
> > since it's dinner time here.
> 
> Yeah, I'm not a fan of it either.  And rereading things, my proposed names also
> create an inconsistency; the zap path is the only user of kvm_handle_gfn_range()
> that uses a plural "rmaps".
> 
>   $ git grep kvm_handle_gfn_range
>   arch/x86/kvm/mmu/mmu.c:static __always_inline bool kvm_handle_gfn_range(struct kvm *kvm,
>   arch/x86/kvm/mmu/mmu.c:         flush = kvm_handle_gfn_range(kvm, range, kvm_zap_rmaps);
>   arch/x86/kvm/mmu/mmu.c:         flush = kvm_handle_gfn_range(kvm, range, kvm_set_pte_rmap);
>   arch/x86/kvm/mmu/mmu.c:         young = kvm_handle_gfn_range(kvm, range, kvm_age_rmap);
>   arch/x86/kvm/mmu/mmu.c:         young = kvm_handle_gfn_range(kvm, range, kvm_test_age_rmap);
> 
> Make "rmaps" plural is probably a mistake.  The helper zaps multiple SPTEs for a
> given rmap list, but from a certain point of view it's just a single "rmap".
> 
> What about:
> 
>   kvm_zap_rmapp => kvm_zap_rmap    // to align with kvm_handle_gfn_range() usage
>   kvm_zap_rmap  => __kvm_zap_rmap  // to pair with kvm_zap_rmap()
>   
> and
> 
>   pte_list_remove  => kvm_zap_one_rmap_spte  
>   pte_list_destroy => kvm_zap_all_rmap_sptes
> 
> That will yield a better series too, as I can move patch 5 to be patch 2, then
> split what was patch 2 (the rename) into separate patches to first align kvm_zap_rmap()
> and __kvm_zap_rmap(), and then rename the pte_list_remove/destroy helpers.

And also:

  __kvm_zap_rmaps => kvm_rmap_zap_gfn_range

instead of renaming it to __kvm_zap_gfn_range() to make it clear that it zaps only
rmap-based MMUs, to align with kvm_rmap_zap_collapsible_sptes(), and to avoid the
plural "rmaps".