On Fri, Aug 16, 2024 at 6:00 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote: > > On Thu, Jul 25, 2024, David Matlack wrote: > > On 2024-07-24 01:10 AM, James Houghton wrote: > > > Optimize both kvm_age_gfn and kvm_test_age_gfn's interaction with the > > > > nit: Use () when referring to functions. > > > > > shadow MMU by, rather than checking if our memslot has rmaps, check if > > > there are any indirect_shadow_pages at all. > > > > What is optimized by checking indirect_shadow_pages instead of > > have_rmaps and what's the benefit? Smells like a premature optimization. > > Checking indirect_shadow_pages avoids taking mmu_lock for write when KVM doesn't > currently have shadow MMU pages, but did at some point in the past, whereas > kvm_memslots_have_rmaps() is sticky and will return true forever. Thanks for the clear explanation. > > > Also, for kvm_test_age_gfn, reorder the TDP MMU check to be first. If we > > > find that the range is young, we do not need to check the shadow MMU. > > > > This should be a separate commit since it's a logically distinct change > > and no dependency on the other change in this commit (other than both > > touch the same function). Done. > > Splitting the commits up will also make it easier to write more specific > > short logs (instead of "optimize a little bit" :) > > +1. Especially code movement and refactoring, e.g. factoring out > tdp_mmu_clear_spte_bits_atomic() would ideally be in a standalone patch that's > dead simple to review. I have now split out the creation of tdp_mmu_clear_spte_bits_atomic() into its own patch. Though I'm not entirely convinced splitting out every refactor like that is always a good thing.
