Re: [PATCH 2/2] KVM: x86/mmu: Don't step down in the TDP iterator when zapping all SPTEs

[Sean Christopherson]

On Thu, Aug 12, 2021, Paolo Bonzini wrote:
> On 12/08/21 19:33, Sean Christopherson wrote:
> > On Thu, Aug 12, 2021, Paolo Bonzini wrote:
> > > On 12/08/21 19:07, Sean Christopherson wrote:
> > > > Yeah, I was/am on the fence too, I almost included a blurb in the cover letter
> > > > saying as much.  I'll do that for v2 and let Paolo decide.
> > > 
> > > I think it makes sense to have it.  You can even use the ternary operator
> > 
> > Hah, yeah, I almost used a ternary op.  Honestly don't know why I didn't, guess
> > my brain flipped a coin.
> > 
> > > 
> > > 	/*
> > > 	 * When zapping everything, all entries at the top level
> > > 	 * ultimately go away, and the levels below go down with them.
> > > 	 * So do not bother iterating all the way down to the leaves.
> > 
> > The subtle part is that try_step_down() won't actually iterate down because it
> > explicitly rereads and rechecks the SPTE.
> > 
> > 	if (iter->level == iter->min_level)
> > 		return false;
> > 
> > 	/*
> > 	 * Reread the SPTE before stepping down to avoid traversing into page
> > 	 * tables that are no longer linked from this entry.
> > 	 */
> > 	iter->old_spte = READ_ONCE(*rcu_dereference(iter->sptep));  \
> >                                                                       ---> this is the code that is avoided
> > 	child_pt = spte_to_child_pt(iter->old_spte, iter->level);   /
> > 	if (!child_pt)
> > 		return false;
> 
> Ah, right - so I agree with Ben that it's not too important.

Ya.  There is a measurable performance improvement, but it's really only
meaningful when there aren't many SPTEs to zap, otherwise the cost of zapping
completely dominates the time.

The one thing that makes me want to include the optimization is that it will kick
in if userspace is constantly modifying memslots, e.g. for option ROMs, in which
case many of the memslot-induced zaps will run with relatively few SPTEs.  The
thread doing the zapping isn't a vCPU thread, but it still holds mmu_lock for
read and thus can be a noisy neighbor of sorts.