[PATCH 2/2] KVM: PPC: Book3S HV: Optimise TLB flushing when a vcpu moves between threads in a core

[PATCH 2/2] KVM: PPC: Book3S HV: Optimise TLB flushing when a vcpu moves between threads in a core

[Nicholas Piggin]

As explained in the comment, there is no need to flush TLBs on all
threads in a core when a vcpu moves between threads in the same core.

Thread migrations can be a significant proportion of vcpu migrations,
so this can help reduce the TLB flushing and IPI traffic.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
I believe we can do this and have the TLB coherency correct as per
the architecture, but would appreciate someone else verifying my
thinking.

Thanks,
Nick

 arch/powerpc/kvm/book3s_hv.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index 752daf43f780..53d0cbfe5933 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -2584,7 +2584,7 @@ static void kvmppc_release_hwthread(int cpu)
 	tpaca->kvm_hstate.kvm_split_mode = NULL;
 }
 
-static void radix_flush_cpu(struct kvm *kvm, int cpu, struct kvm_vcpu *vcpu)
+static void radix_flush_cpu(struct kvm *kvm, int cpu, bool core, struct kvm_vcpu *vcpu)
 {
 	struct kvm_nested_guest *nested = vcpu->arch.nested;
 	cpumask_t *cpu_in_guest;
@@ -2599,6 +2599,14 @@ static void radix_flush_cpu(struct kvm *kvm, int cpu, struct kvm_vcpu *vcpu)
 		cpu_in_guest = &kvm->arch.cpu_in_guest;
 	}
 
+	if (!core) {
+		cpumask_set_cpu(cpu, need_tlb_flush);
+		smp_mb();
+		if (cpumask_test_cpu(cpu, cpu_in_guest))
+			smp_call_function_single(cpu, do_nothing, NULL, 1);
+		return;
+	}
+
 	cpu = cpu_first_thread_sibling(cpu);
 	for (i = 0; i < threads_per_core; ++i)
 		cpumask_set_cpu(cpu + i, need_tlb_flush);
@@ -2655,7 +2663,23 @@ static void kvmppc_prepare_radix_vcpu(struct kvm_vcpu *vcpu, int pcpu)
 		if (prev_cpu < 0)
 			return; /* first run */
 
-		radix_flush_cpu(kvm, prev_cpu, vcpu);
+		/*
+		 * If changing cores, all threads on the old core should
+		 * flush, because TLBs can be shared between threads. More
+		 * precisely, the thread we previously ran on should be
+		 * flushed, and the thread to first run a vcpu on the old
+		 * core should flush, but we don't keep enough information
+		 * around to track that, so we flush all.
+		 *
+		 * If changing threads in the same core, only the old thread
+		 * need be flushed.
+		 */
+		if (cpu_first_thread_sibling(prev_cpu) !+		    cpu_first_thread_sibling(pcpu))
+			radix_flush_cpu(kvm, prev_cpu, true, vcpu);
+		else
+			radix_flush_cpu(kvm, prev_cpu, false, vcpu);
+
 	}
 }
 
-- 
2.23.0

Re: [PATCH 2/2] KVM: PPC: Book3S HV: Optimise TLB flushing when a vcpu moves between threads in a co

[Aneesh Kumar K.V]

Nicholas Piggin <npiggin@gmail.com> writes:

> As explained in the comment, there is no need to flush TLBs on all
> threads in a core when a vcpu moves between threads in the same core.
>
> Thread migrations can be a significant proportion of vcpu migrations,
> so this can help reduce the TLB flushing and IPI traffic.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> I believe we can do this and have the TLB coherency correct as per
> the architecture, but would appreciate someone else verifying my
> thinking.
>
> Thanks,
> Nick
>
>  arch/powerpc/kvm/book3s_hv.c | 28 ++++++++++++++++++++++++++--
>  1 file changed, 26 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index 752daf43f780..53d0cbfe5933 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -2584,7 +2584,7 @@ static void kvmppc_release_hwthread(int cpu)
>  	tpaca->kvm_hstate.kvm_split_mode = NULL;
>  }
>  
> -static void radix_flush_cpu(struct kvm *kvm, int cpu, struct kvm_vcpu *vcpu)
> +static void radix_flush_cpu(struct kvm *kvm, int cpu, bool core, struct kvm_vcpu *vcpu)
>  {

Can we rename 'core' to something like 'core_sched'  or 'within_core' 

>  	struct kvm_nested_guest *nested = vcpu->arch.nested;
>  	cpumask_t *cpu_in_guest;
> @@ -2599,6 +2599,14 @@ static void radix_flush_cpu(struct kvm *kvm, int cpu, struct kvm_vcpu *vcpu)
>  		cpu_in_guest = &kvm->arch.cpu_in_guest;
>  	}
>

and do
      if (core_sched) {

> +	if (!core) {
> +		cpumask_set_cpu(cpu, need_tlb_flush);
> +		smp_mb();
> +		if (cpumask_test_cpu(cpu, cpu_in_guest))
> +			smp_call_function_single(cpu, do_nothing, NULL, 1);
> +		return;
> +	}
> +
>  	cpu = cpu_first_thread_sibling(cpu);
>  	for (i = 0; i < threads_per_core; ++i)
>  		cpumask_set_cpu(cpu + i, need_tlb_flush);
> @@ -2655,7 +2663,23 @@ static void kvmppc_prepare_radix_vcpu(struct kvm_vcpu *vcpu, int pcpu)
>  		if (prev_cpu < 0)
>  			return; /* first run */
>  
> -		radix_flush_cpu(kvm, prev_cpu, vcpu);
> +		/*
> +		 * If changing cores, all threads on the old core should
> +		 * flush, because TLBs can be shared between threads. More
> +		 * precisely, the thread we previously ran on should be
> +		 * flushed, and the thread to first run a vcpu on the old
> +		 * core should flush, but we don't keep enough information
> +		 * around to track that, so we flush all.
> +		 *
> +		 * If changing threads in the same core, only the old thread
> +		 * need be flushed.
> +		 */
> +		if (cpu_first_thread_sibling(prev_cpu) !> +		    cpu_first_thread_sibling(pcpu))
> +			radix_flush_cpu(kvm, prev_cpu, true, vcpu);
> +		else
> +			radix_flush_cpu(kvm, prev_cpu, false, vcpu);
> +
>  	}
>  }
>  
> -- 
> 2.23.0

Re: [PATCH 2/2] KVM: PPC: Book3S HV: Optimise TLB flushing when a vcpu moves between threads in a co

[Nicholas Piggin]

Excerpts from Aneesh Kumar K.V's message of January 19, 2021 7:45 pm:
> Nicholas Piggin <npiggin@gmail.com> writes:
> 
>> As explained in the comment, there is no need to flush TLBs on all
>> threads in a core when a vcpu moves between threads in the same core.
>>
>> Thread migrations can be a significant proportion of vcpu migrations,
>> so this can help reduce the TLB flushing and IPI traffic.
>>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>> I believe we can do this and have the TLB coherency correct as per
>> the architecture, but would appreciate someone else verifying my
>> thinking.
>>
>> Thanks,
>> Nick
>>
>>  arch/powerpc/kvm/book3s_hv.c | 28 ++++++++++++++++++++++++++--
>>  1 file changed, 26 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
>> index 752daf43f780..53d0cbfe5933 100644
>> --- a/arch/powerpc/kvm/book3s_hv.c
>> +++ b/arch/powerpc/kvm/book3s_hv.c
>> @@ -2584,7 +2584,7 @@ static void kvmppc_release_hwthread(int cpu)
>>  	tpaca->kvm_hstate.kvm_split_mode = NULL;
>>  }
>>  
>> -static void radix_flush_cpu(struct kvm *kvm, int cpu, struct kvm_vcpu *vcpu)
>> +static void radix_flush_cpu(struct kvm *kvm, int cpu, bool core, struct kvm_vcpu *vcpu)
>>  {
> 
> Can we rename 'core' to something like 'core_sched'  or 'within_core' 
> 
>>  	struct kvm_nested_guest *nested = vcpu->arch.nested;
>>  	cpumask_t *cpu_in_guest;
>> @@ -2599,6 +2599,14 @@ static void radix_flush_cpu(struct kvm *kvm, int cpu, struct kvm_vcpu *vcpu)
>>  		cpu_in_guest = &kvm->arch.cpu_in_guest;
>>  	}
>>
> 
> and do
>       if (core_sched) {

This function is called to flush guest TLBs on this cpu / all threads on 
this cpu core. I don't think it helps to bring any "why" logic into it
because the caller has already dealt with that.

Thanks,
Nick

> 
>> +	if (!core) {
>> +		cpumask_set_cpu(cpu, need_tlb_flush);
>> +		smp_mb();
>> +		if (cpumask_test_cpu(cpu, cpu_in_guest))
>> +			smp_call_function_single(cpu, do_nothing, NULL, 1);
>> +		return;
>> +	}
>> +
>>  	cpu = cpu_first_thread_sibling(cpu);
>>  	for (i = 0; i < threads_per_core; ++i)
>>  		cpumask_set_cpu(cpu + i, need_tlb_flush);
>> @@ -2655,7 +2663,23 @@ static void kvmppc_prepare_radix_vcpu(struct kvm_vcpu *vcpu, int pcpu)
>>  		if (prev_cpu < 0)
>>  			return; /* first run */
>>  
>> -		radix_flush_cpu(kvm, prev_cpu, vcpu);
>> +		/*
>> +		 * If changing cores, all threads on the old core should
>> +		 * flush, because TLBs can be shared between threads. More
>> +		 * precisely, the thread we previously ran on should be
>> +		 * flushed, and the thread to first run a vcpu on the old
>> +		 * core should flush, but we don't keep enough information
>> +		 * around to track that, so we flush all.
>> +		 *
>> +		 * If changing threads in the same core, only the old thread
>> +		 * need be flushed.
>> +		 */
>> +		if (cpu_first_thread_sibling(prev_cpu) !=
>> +		    cpu_first_thread_sibling(pcpu))
>> +			radix_flush_cpu(kvm, prev_cpu, true, vcpu);
>> +		else
>> +			radix_flush_cpu(kvm, prev_cpu, false, vcpu);
>> +
>>  	}
>>  }
>>  
>> -- 
>> 2.23.0
> 

Re: [PATCH 2/2] KVM: PPC: Book3S HV: Optimise TLB flushing when a vcpu moves between threads in a co

[Paul Mackerras]

On Mon, Jan 18, 2021 at 10:26:09PM +1000, Nicholas Piggin wrote:
> As explained in the comment, there is no need to flush TLBs on all
> threads in a core when a vcpu moves between threads in the same core.
> 
> Thread migrations can be a significant proportion of vcpu migrations,
> so this can help reduce the TLB flushing and IPI traffic.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> I believe we can do this and have the TLB coherency correct as per
> the architecture, but would appreciate someone else verifying my
> thinking.

So far I have not been able to convince myself that migrating within a
core is really different from migrating across cores as far as the
architecture is concerned.  If you're trying to allow for an
implementation where TLB entries are shared but tlbiel only works
(effectively and completely) on the local thread, then I don't think
you can do this.  If a TLB entry is created on T0, then the vcpu moves
to T1 and does a tlbiel, then the guest task on that vcpu migrates to
the vcpu that is on T2, it might still see a stale TLB entry.

Paul.

Re: [PATCH 2/2] KVM: PPC: Book3S HV: Optimise TLB flushing when a vcpu moves between threads in a co

[Paul Mackerras]

On Wed, Jan 20, 2021 at 10:26:51AM +1000, Nicholas Piggin wrote:
> Excerpts from Aneesh Kumar K.V's message of January 19, 2021 7:45 pm:
> > Nicholas Piggin <npiggin@gmail.com> writes:
> > 
> >> As explained in the comment, there is no need to flush TLBs on all
> >> threads in a core when a vcpu moves between threads in the same core.
> >>
> >> Thread migrations can be a significant proportion of vcpu migrations,
> >> so this can help reduce the TLB flushing and IPI traffic.
> >>
> >> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> >> ---
> >> I believe we can do this and have the TLB coherency correct as per
> >> the architecture, but would appreciate someone else verifying my
> >> thinking.
> >>
> >> Thanks,
> >> Nick
> >>
> >>  arch/powerpc/kvm/book3s_hv.c | 28 ++++++++++++++++++++++++++--
> >>  1 file changed, 26 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> >> index 752daf43f780..53d0cbfe5933 100644
> >> --- a/arch/powerpc/kvm/book3s_hv.c
> >> +++ b/arch/powerpc/kvm/book3s_hv.c
> >> @@ -2584,7 +2584,7 @@ static void kvmppc_release_hwthread(int cpu)
> >>  	tpaca->kvm_hstate.kvm_split_mode = NULL;
> >>  }
> >>  
> >> -static void radix_flush_cpu(struct kvm *kvm, int cpu, struct kvm_vcpu *vcpu)
> >> +static void radix_flush_cpu(struct kvm *kvm, int cpu, bool core, struct kvm_vcpu *vcpu)
> >>  {
> > 
> > Can we rename 'core' to something like 'core_sched'  or 'within_core' 
> > 
> >>  	struct kvm_nested_guest *nested = vcpu->arch.nested;
> >>  	cpumask_t *cpu_in_guest;
> >> @@ -2599,6 +2599,14 @@ static void radix_flush_cpu(struct kvm *kvm, int cpu, struct kvm_vcpu *vcpu)
> >>  		cpu_in_guest = &kvm->arch.cpu_in_guest;
> >>  	}
> >>
> > 
> > and do
> >       if (core_sched) {
> 
> This function is called to flush guest TLBs on this cpu / all threads on 
> this cpu core. I don't think it helps to bring any "why" logic into it
> because the caller has already dealt with that.

I agree with Aneesh that the name "core" doesn't really help the
reader to know what the parameter means.  Either it needs a comment or
a more descriptive name.

Paul.

Re: [PATCH 2/2] KVM: PPC: Book3S HV: Optimise TLB flushing when a vcpu moves between threads in a co

[Nicholas Piggin]

Excerpts from Paul Mackerras's message of February 9, 2021 5:26 pm:
> On Wed, Jan 20, 2021 at 10:26:51AM +1000, Nicholas Piggin wrote:
>> Excerpts from Aneesh Kumar K.V's message of January 19, 2021 7:45 pm:
>> > Nicholas Piggin <npiggin@gmail.com> writes:
>> > 
>> >> As explained in the comment, there is no need to flush TLBs on all
>> >> threads in a core when a vcpu moves between threads in the same core.
>> >>
>> >> Thread migrations can be a significant proportion of vcpu migrations,
>> >> so this can help reduce the TLB flushing and IPI traffic.
>> >>
>> >> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> >> ---
>> >> I believe we can do this and have the TLB coherency correct as per
>> >> the architecture, but would appreciate someone else verifying my
>> >> thinking.
>> >>
>> >> Thanks,
>> >> Nick
>> >>
>> >>  arch/powerpc/kvm/book3s_hv.c | 28 ++++++++++++++++++++++++++--
>> >>  1 file changed, 26 insertions(+), 2 deletions(-)
>> >>
>> >> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
>> >> index 752daf43f780..53d0cbfe5933 100644
>> >> --- a/arch/powerpc/kvm/book3s_hv.c
>> >> +++ b/arch/powerpc/kvm/book3s_hv.c
>> >> @@ -2584,7 +2584,7 @@ static void kvmppc_release_hwthread(int cpu)
>> >>  	tpaca->kvm_hstate.kvm_split_mode = NULL;
>> >>  }
>> >>  
>> >> -static void radix_flush_cpu(struct kvm *kvm, int cpu, struct kvm_vcpu *vcpu)
>> >> +static void radix_flush_cpu(struct kvm *kvm, int cpu, bool core, struct kvm_vcpu *vcpu)
>> >>  {
>> > 
>> > Can we rename 'core' to something like 'core_sched'  or 'within_core' 
>> > 
>> >>  	struct kvm_nested_guest *nested = vcpu->arch.nested;
>> >>  	cpumask_t *cpu_in_guest;
>> >> @@ -2599,6 +2599,14 @@ static void radix_flush_cpu(struct kvm *kvm, int cpu, struct kvm_vcpu *vcpu)
>> >>  		cpu_in_guest = &kvm->arch.cpu_in_guest;
>> >>  	}
>> >>
>> > 
>> > and do
>> >       if (core_sched) {
>> 
>> This function is called to flush guest TLBs on this cpu / all threads on 
>> this cpu core. I don't think it helps to bring any "why" logic into it
>> because the caller has already dealt with that.
> 
> I agree with Aneesh that the name "core" doesn't really help the
> reader to know what the parameter means.  Either it needs a comment or
> a more descriptive name.

Okay. 'all_threads_in_core' is less typing than a comment :)

Thanks,
Nick

Re: [PATCH 2/2] KVM: PPC: Book3S HV: Optimise TLB flushing when a vcpu moves between threads in a co

[Nicholas Piggin]

Excerpts from Paul Mackerras's message of February 9, 2021 5:23 pm:
> On Mon, Jan 18, 2021 at 10:26:09PM +1000, Nicholas Piggin wrote:
>> As explained in the comment, there is no need to flush TLBs on all
>> threads in a core when a vcpu moves between threads in the same core.
>> 
>> Thread migrations can be a significant proportion of vcpu migrations,
>> so this can help reduce the TLB flushing and IPI traffic.
>> 
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>> I believe we can do this and have the TLB coherency correct as per
>> the architecture, but would appreciate someone else verifying my
>> thinking.
> 
> So far I have not been able to convince myself that migrating within a
> core is really different from migrating across cores as far as the
> architecture is concerned.  If you're trying to allow for an
> implementation where TLB entries are shared but tlbiel only works
> (effectively and completely) on the local thread, then I don't think
> you can do this.  If a TLB entry is created on T0, then the vcpu moves
> to T1 and does a tlbiel, then the guest task on that vcpu migrates to
> the vcpu that is on T2, it might still see a stale TLB entry.

The difference is that the guest TLBIEL will still execute on the same 
core, so it should take care of the shared / core-wide translations that 
were set up. Therefore you just have to worry about the private ones, 
and in that case you only need to invalidate the threads that it ran on.

Thanks,
Nick