Re: [PATCH v4 2/4] KVM: X86: Add paravirt remote TLB flush

[Peter Zijlstra]

On Mon, Nov 13, 2017 at 11:46:34AM +0100, Peter Zijlstra wrote:
> On Mon, Nov 13, 2017 at 04:26:57PM +0800, Wanpeng Li wrote:
> > 2017-11-13 16:04 GMT+08:00 Peter Zijlstra <peterz@xxxxxxxxxxxxx>:
> 
> > > So if at this point a vCPU gets preempted we'll still spin-wait for it,
> > > which is sub-optimal.
> > >
> > > I think we can come up with something to get around that 'problem' if
> > > indeed it is a problem. But we can easily do that as follow up patches.
> > > Just let me know if you think its worth spending more time on.
> > 
> > You can post your idea, it is always smart. :) Then we can evaluate
> > the complexity and gains.
> 
> I'm not sure I have a fully baked idea just yet, but the general idea
> would be something like:
> 
>  - switch (back) to a dedicated TLB invalidate IPI

Just for PV that is; the !PV code can continue doing what it does today.

>  - introduce KVM_VCPU_IPI_PENDING
> 
>  - change flush_tlb_others() into something like:
> 
>    for_each_cpu(cpu, flushmask) {
> 	 src = &per_cpu(steal_time, cpu);
> 	 state = READ_ONCE(src->preempted);
> 	 do {
> 		 if (state & KVM_VCPU_PREEMPTED) {
> 			 if (try_cmpxchg(&src->preempted, &state,
> 						 state | KVM_VCPU_SHOULD_FLUSH)) {
> 				 __cpumask_clear_cpu(cpu, flushmask);
> 				 break;
> 			 }
> 		 }
> 	 } while (!try_cmpxchg(&src->preempted, &state,
> 				 state | KVM_VCPU_IPI_PENDING));

That can be written like:

	do {
		if (state & KVM_VCPU_PREEMPTED)
			new_state = state | KVM_VCPU_SHOULD_FLUSH;
		else
			new_state = state | KVM_VCPU_IPI_PENDING;
	} while (!try_cmpxchg(&src->preempted, state, new_state);

	if (new_state & KVM_VCPU_IPI_PENDING)
		__cpumask_clear_cpu(cpu, flushmask);

>    }
> 
>    apic->send_IPI_mask(flushmask, CALL_TLB_VECTOR);
> 
>    for_each_cpu(cpu, flushmask) {
> 	 src = &per_cpu(steal_time, cpu);

	/*
	 * The ACQUIRE pairs with the cmpxchg clearing IPI_PENDING,
	 * which is either the TLB IPI handler, or the VMEXIT path.
	 * It ensure that the invalidate happens-before.
	 */
> 	 smp_cond_load_acquire(&src->preempted, !(VAL & KVM_VCPU_IPI_PENDING);
>    }

And here we wait for completion of the invalidate; but because of the
VMEXIT change below, this will never stall on a !running vCPU.

Note that PLE will not help (much) here, without this extra IPI_PENDING
state and the VMEXIT transferring it to SHOULD_FLUSH this vCPU's progress
will be held up until all vCPU's you've IPI'd will have ran the IPI
handler, which in the worst case is still a very long time.

>  - have the TLB invalidate handler do something like:
> 
>    state = READ_ONCE(src->preempted);
>    if (!(state & KVM_VCPU_IPI_PENDING))
> 	   return;
> 
>    local_flush_tlb();
> 
>    do {
>    } while (!try_cmpxchg(&src->preempted, &state,
> 			 state & ~KVM_VCPU_IPI_PENDING));

That needs to be:

	/*
	 * Clear KVM_VCPU_IPI_PENDING to 'complete' flush_tlb_others().
	 */
	do {
		/*
		 * VMEXIT could have cleared this for us, in which case
		 * we're done.
		 */
		if (!(state & KVM_VCPU_IPI_PENDING))
			return;

	} while (!try_cmpxchg(&src->preempted, state,
				state & ~KVM_VCPU_IPI_PENDING));

>  - then at VMEXIT time do something like:
> 
	/*
	 * If we have IPI_PENDING set at VMEXIT time, transfer it to
	 * SHOULD_FLUSH. Clearing IPI_PENDING here allows the
	 * flush_others() vCPU to continue while the SHOULD_FLUSH
	 * guarantees this vCPU will flush TLBs before it continues
	 * execution.
	 */

>    state = READ_ONCE(src->preempted);
>    do {
> 	if (!(state & KVM_VCPU_IPI_PENDING))
> 		break;
>    } while (!try_cmpxchg(&src->preempted, state,
> 			 (state & ~KVM_VCPU_IPI_PENDING) |
> 			 KVM_VCPU_SHOULD_FLUSH));
> 
>    and clear any possible pending TLB_VECTOR in the guest state to avoid
>    raising that IPI spuriously on enter again.
>