Re: [PATCH v2 3/7] KVM: Add paravirt kvm_flush_tlb_others

[Marcelo Tosatti <mtosatti@redhat.com>]

On Tue, Jul 03, 2012 at 01:49:49PM +0530, Nikunj A Dadhania wrote:
> On Tue, 3 Jul 2012 04:55:35 -0300, Marcelo Tosatti <mtosatti@redhat.com> wrote:
> > > 
> > >    if (!zero_mask)
> > > 	   goto again;
> > 
> > Can you please measure increased vmentry/vmexit overhead? x86/vmexit.c 
> > of git://git.kernel.org/pub/scm/virt/kvm/kvm-unit-tests.git should 
> > help.
> >
> Sure will get back with the result.
> 
> > > +	/* 
> > > +	 * Guest might have seen us offline and would have set
> > > +	 * flush_on_enter. 
> > > +	 */
> > > +	kvm_read_guest_cached(vcpu->kvm, ghc, vs, 2*sizeof(__u32));
> > > +	if (vs->flush_on_enter) 
> > > +		kvm_x86_ops->tlb_flush(vcpu);
> > 
> > 
> > So flush_tlb_page which was an invlpg now flushes the entire TLB. Did
> > you take that into account?
> > 
> When the vcpu is sleeping/pre-empted out, multiple request for flush_tlb
> could have happened. And now when we are here, it is cleaning up all the
> TLB.

Yes, cases where there are sufficient exits transforming one TLB entry
invalidation into full TLB invalidation should go unnoticed.

> One other approach would be to queue the addresses, that brings us with
> the question: how many request to queue? This would require us adding
> more syncronization between guest and host for updating the area where
> these addresses is shared.

Sounds unnecessarily complicated.

> > > +again:
> > > +		for_each_cpu(cpu, to_cpumask(f->flush_cpumask)) {
> > > +			v_state = &per_cpu(vcpu_state, cpu);
> > > +
> > > +			if (!v_state->state) {
> > 
> > Should use ACCESS_ONCE to make sure the value is not register cached.
> > \
> > > +				v_state->flush_on_enter = 1;
> > > +				smp_mb();
> > > +				if (!v_state->state)
> > 
> > And here.
> > 
> Sure will add this check for both in my next version.
> 
> > > +					cpumask_clear_cpu(cpu, to_cpumask(f->flush_cpumask));
> > > +			}
> > > +		}
> > > +
> > > +		if (cpumask_empty(to_cpumask(f->flush_cpumask)))
> > > +			goto out;
> > > +
> > > +		apic->send_IPI_mask(to_cpumask(f->flush_cpumask),
> > > +				    INVALIDATE_TLB_VECTOR_START + sender);
> > > +
> > > +		loop = 1000;
> > > +		while (!cpumask_empty(to_cpumask(f->flush_cpumask)) && --loop)
> > > +			cpu_relax();
> > > +
> > > +		if (!cpumask_empty(to_cpumask(f->flush_cpumask)))
> > > +			goto again;
> > 
> > Is this necessary in addition to the in-guest-mode/out-guest-mode
> > detection? If so, why?
> > 
> The "case 3" where we initially saw the vcpu was running, and a flush
> ipi is send to the vcpu. During this time the vcpu might be pre-empted,
> so we come out of the loop=1000 with !empty flushmask. We then re-verify
> the flushmask against the current running vcpu and make sure that the
> vcpu that was pre-empted is un-marked and we can proceed out of the
> kvm_flush_tlb_others_ipi without waiting for sleeping/pre-empted vcpus.
> 
> Regards
> Nikunj