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

[Sean Christopherson]

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.