* Re: [PATCH 2/2] KVM: PPC: Book3S HV: Optimise TLB flushing when a vcpu moves between threads in a co
2021-01-18 12:26 [PATCH 2/2] KVM: PPC: Book3S HV: Optimise TLB flushing when a vcpu moves between threads in a core Nicholas Piggin
@ 2021-01-19 9:57 ` Aneesh Kumar K.V
2021-01-20 0:26 ` Nicholas Piggin
` (4 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Aneesh Kumar K.V @ 2021-01-19 9:57 UTC (permalink / raw)
To: kvm-ppc
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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 2/2] KVM: PPC: Book3S HV: Optimise TLB flushing when a vcpu moves between threads in a co
2021-01-18 12:26 [PATCH 2/2] KVM: PPC: Book3S HV: Optimise TLB flushing when a vcpu moves between threads in a core Nicholas Piggin
2021-01-19 9:57 ` [PATCH 2/2] KVM: PPC: Book3S HV: Optimise TLB flushing when a vcpu moves between threads in a co Aneesh Kumar K.V
@ 2021-01-20 0:26 ` Nicholas Piggin
2021-02-09 7:23 ` Paul Mackerras
` (3 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Nicholas Piggin @ 2021-01-20 0:26 UTC (permalink / raw)
To: kvm-ppc
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
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 2/2] KVM: PPC: Book3S HV: Optimise TLB flushing when a vcpu moves between threads in a co
2021-01-18 12:26 [PATCH 2/2] KVM: PPC: Book3S HV: Optimise TLB flushing when a vcpu moves between threads in a core Nicholas Piggin
2021-01-19 9:57 ` [PATCH 2/2] KVM: PPC: Book3S HV: Optimise TLB flushing when a vcpu moves between threads in a co Aneesh Kumar K.V
2021-01-20 0:26 ` Nicholas Piggin
@ 2021-02-09 7:23 ` Paul Mackerras
2021-02-09 7:26 ` Paul Mackerras
` (2 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Paul Mackerras @ 2021-02-09 7:23 UTC (permalink / raw)
To: kvm-ppc
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.
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 2/2] KVM: PPC: Book3S HV: Optimise TLB flushing when a vcpu moves between threads in a co
2021-01-18 12:26 [PATCH 2/2] KVM: PPC: Book3S HV: Optimise TLB flushing when a vcpu moves between threads in a core Nicholas Piggin
` (2 preceding siblings ...)
2021-02-09 7:23 ` Paul Mackerras
@ 2021-02-09 7:26 ` Paul Mackerras
2021-02-09 7:59 ` Nicholas Piggin
2021-02-09 8:47 ` Nicholas Piggin
5 siblings, 0 replies; 7+ messages in thread
From: Paul Mackerras @ 2021-02-09 7:26 UTC (permalink / raw)
To: kvm-ppc
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.
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 2/2] KVM: PPC: Book3S HV: Optimise TLB flushing when a vcpu moves between threads in a co
2021-01-18 12:26 [PATCH 2/2] KVM: PPC: Book3S HV: Optimise TLB flushing when a vcpu moves between threads in a core Nicholas Piggin
` (3 preceding siblings ...)
2021-02-09 7:26 ` Paul Mackerras
@ 2021-02-09 7:59 ` Nicholas Piggin
2021-02-09 8:47 ` Nicholas Piggin
5 siblings, 0 replies; 7+ messages in thread
From: Nicholas Piggin @ 2021-02-09 7:59 UTC (permalink / raw)
To: kvm-ppc
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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 2/2] KVM: PPC: Book3S HV: Optimise TLB flushing when a vcpu moves between threads in a co
2021-01-18 12:26 [PATCH 2/2] KVM: PPC: Book3S HV: Optimise TLB flushing when a vcpu moves between threads in a core Nicholas Piggin
` (4 preceding siblings ...)
2021-02-09 7:59 ` Nicholas Piggin
@ 2021-02-09 8:47 ` Nicholas Piggin
5 siblings, 0 replies; 7+ messages in thread
From: Nicholas Piggin @ 2021-02-09 8:47 UTC (permalink / raw)
To: kvm-ppc
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
^ permalink raw reply [flat|nested] 7+ messages in thread