[PATCH v3] KVM: PPC: e500mc: Enhance tlb invalidation condition on vcpu schedule

[PATCH v3] KVM: PPC: e500mc: Enhance tlb invalidation condition on vcpu schedule

[Mihai Caraman]

On vcpu schedule, the condition checked for tlb pollution is too loose.
The tlb entries of a vcpu become polluted (vs stale) only when a different
vcpu within the same logical partition runs in-between. Optimize the tlb
invalidation condition keeping last_vcpu_on_cpu per logical partition id.

With the new invalidation condition, a guest shows 4% performance improvement
on P5020DS while running a memory stress application with the cpu oversubscribed,
the other guest running a cpu intensive workload.

Guest - old invalidation condition
  real 3.89
  user 3.87
  sys 0.01

Guest - enhanced invalidation condition
  real 3.75
  user 3.73
  sys 0.01

Host
  real 3.70
  user 1.85
  sys 0.00

The memory stress application accesses 4KB pages backed by 75% of available
TLB0 entries:

char foo[ENTRIES][4096] __attribute__ ((aligned (4096)));

int main()
{
	char bar;
	int i, j;

	for (i = 0; i < ITERATIONS; i++)
        	for (j = 0; j < ENTRIES; j++)
            		bar = foo[j][0];

	return 0;
}

Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com>
Cc: Scott Wood <scottwood@freescale.com>
---
v3:
 - use existing logic while keeping last_vcpu_per_cpu per lpid
 
v2:
 - improve patch name and description
 - add performance results


 arch/powerpc/kvm/e500mc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/kvm/e500mc.c b/arch/powerpc/kvm/e500mc.c
index 17e4562..95e33e3 100644
--- a/arch/powerpc/kvm/e500mc.c
+++ b/arch/powerpc/kvm/e500mc.c
@@ -110,7 +110,7 @@ void kvmppc_mmu_msr_notify(struct kvm_vcpu *vcpu, u32 old_msr)
 {
 }
 
-static DEFINE_PER_CPU(struct kvm_vcpu *, last_vcpu_on_cpu);
+static DEFINE_PER_CPU(struct kvm_vcpu * [KVMPPC_NR_LPIDS], last_vcpu_on_cpu);
 
 static void kvmppc_core_vcpu_load_e500mc(struct kvm_vcpu *vcpu, int cpu)
 {
@@ -141,9 +141,9 @@ static void kvmppc_core_vcpu_load_e500mc(struct kvm_vcpu *vcpu, int cpu)
 	mtspr(SPRN_GESR, vcpu->arch.shared->esr);
 
 	if (vcpu->arch.oldpir != mfspr(SPRN_PIR) ||
-	    __get_cpu_var(last_vcpu_on_cpu) != vcpu) {
+	    __get_cpu_var(last_vcpu_on_cpu)[vcpu->kvm->arch.lpid] != vcpu) {
 		kvmppc_e500_tlbil_all(vcpu_e500);
-		__get_cpu_var(last_vcpu_on_cpu) = vcpu;
+		__get_cpu_var(last_vcpu_on_cpu)[vcpu->kvm->arch.lpid] = vcpu;
 	}
 
 	kvmppc_load_guest_fp(vcpu);
-- 
1.7.11.7

Re: [PATCH v3] KVM: PPC: e500mc: Enhance tlb invalidation condition on vcpu schedule

[Scott Wood]

On Tue, 2014-06-17 at 22:09 +0300, Mihai Caraman wrote:
> On vcpu schedule, the condition checked for tlb pollution is too loose.
> The tlb entries of a vcpu become polluted (vs stale) only when a different
> vcpu within the same logical partition runs in-between. Optimize the tlb
> invalidation condition keeping last_vcpu_on_cpu per logical partition id.
> 
> With the new invalidation condition, a guest shows 4% performance improvement
> on P5020DS while running a memory stress application with the cpu oversubscribed,
> the other guest running a cpu intensive workload.
> 
> Guest - old invalidation condition
>   real 3.89
>   user 3.87
>   sys 0.01
> 
> Guest - enhanced invalidation condition
>   real 3.75
>   user 3.73
>   sys 0.01
> 
> Host
>   real 3.70
>   user 1.85
>   sys 0.00
> 
> The memory stress application accesses 4KB pages backed by 75% of available
> TLB0 entries:
> 
> char foo[ENTRIES][4096] __attribute__ ((aligned (4096)));
> 
> int main()
> {
> 	char bar;
> 	int i, j;
> 
> 	for (i = 0; i < ITERATIONS; i++)
>         	for (j = 0; j < ENTRIES; j++)
>             		bar = foo[j][0];
> 
> 	return 0;
> }
> 
> Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com>
> Cc: Scott Wood <scottwood@freescale.com>
> ---
> v3:
>  - use existing logic while keeping last_vcpu_per_cpu per lpid
>  
> v2:
>  - improve patch name and description
>  - add performance results
> 
> 
>  arch/powerpc/kvm/e500mc.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/e500mc.c b/arch/powerpc/kvm/e500mc.c
> index 17e4562..95e33e3 100644
> --- a/arch/powerpc/kvm/e500mc.c
> +++ b/arch/powerpc/kvm/e500mc.c
> @@ -110,7 +110,7 @@ void kvmppc_mmu_msr_notify(struct kvm_vcpu *vcpu, u32 old_msr)
>  {
>  }
>  
> -static DEFINE_PER_CPU(struct kvm_vcpu *, last_vcpu_on_cpu);
> +static DEFINE_PER_CPU(struct kvm_vcpu * [KVMPPC_NR_LPIDS], last_vcpu_on_cpu);

Hmm, I didn't know you could express types like that.  Is this special
syntax that only works for typeof?

No space after *

Name should be adjusted to match, something like last_vcpu_of_lpid (with
the _on_cpu being implied by the fact that it's PER_CPU).

-Scott

RE: [PATCH v3] KVM: PPC: e500mc: Enhance tlb invalidation condition on vcpu schedule

[mihai.caraman]

> > -static DEFINE_PER_CPU(struct kvm_vcpu *, last_vcpu_on_cpu);
> > +static DEFINE_PER_CPU(struct kvm_vcpu * [KVMPPC_NR_LPIDS],
> last_vcpu_on_cpu);
> 
> Hmm, I didn't know you could express types like that.  Is this special
> syntax that only works for typeof?

Yes, AFAIK.

> No space after *

Checkpatch complains about the missing space ;)

> 
> Name should be adjusted to match, something like last_vcpu_of_lpid (with
> the _on_cpu being implied by the fact that it's PER_CPU).

I was thinking to the long name but it was not appealing, I will change it to
last_vcpu_of_lpid.

-Mike

Re: [PATCH v3] KVM: PPC: e500mc: Enhance tlb invalidation condition on vcpu schedule

[Scott Wood]

On Tue, 2014-06-17 at 14:42 -0500, Caraman Mihai Claudiu-B02008 wrote:
> > > -static DEFINE_PER_CPU(struct kvm_vcpu *, last_vcpu_on_cpu);
> > > +static DEFINE_PER_CPU(struct kvm_vcpu * [KVMPPC_NR_LPIDS],
> > last_vcpu_on_cpu);
> > 
> > Hmm, I didn't know you could express types like that.  Is this special
> > syntax that only works for typeof?
> 
> Yes, AFAIK.
> 
> > No space after *
> 
> Checkpatch complains about the missing space ;)

Checkpatch is wrong, which isn't surprising given that this is unusual
syntax.  We don't normally put a space after * when used to represent a
pointer.

-Scott

RE: [PATCH v3] KVM: PPC: e500mc: Enhance tlb invalidation condition on vcpu schedule

[mihai.caraman]

> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Tuesday, June 17, 2014 10:48 PM
> To: Caraman Mihai Claudiu-B02008
> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; linuxppc-
> dev@lists.ozlabs.org
> Subject: Re: [PATCH v3] KVM: PPC: e500mc: Enhance tlb invalidation
> condition on vcpu schedule
> 
> On Tue, 2014-06-17 at 14:42 -0500, Caraman Mihai Claudiu-B02008 wrote:
> > > > -static DEFINE_PER_CPU(struct kvm_vcpu *, last_vcpu_on_cpu);
> > > > +static DEFINE_PER_CPU(struct kvm_vcpu * [KVMPPC_NR_LPIDS],
> > > last_vcpu_on_cpu);
> > >
> > > Hmm, I didn't know you could express types like that.  Is this
> special
> > > syntax that only works for typeof?
> >
> > Yes, AFAIK.
> >
> > > No space after *
> >
> > Checkpatch complains about the missing space ;)
> 
> Checkpatch is wrong, which isn't surprising given that this is unusual
> syntax.  We don't normally put a space after * when used to represent a
> pointer.

This is not something new. See [PATCH 04/10] percpu: cleanup percpu array
definitions:

	https://lkml.org/lkml/2009/6/24/26

-Mike

Re: [PATCH v3] KVM: PPC: e500mc: Enhance tlb invalidation condition on vcpu schedule

[Scott Wood]

On Tue, 2014-06-17 at 15:02 -0500, Caraman Mihai Claudiu-B02008 wrote:
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Tuesday, June 17, 2014 10:48 PM
> > To: Caraman Mihai Claudiu-B02008
> > Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; linuxppc-
> > dev@lists.ozlabs.org
> > Subject: Re: [PATCH v3] KVM: PPC: e500mc: Enhance tlb invalidation
> > condition on vcpu schedule
> > 
> > On Tue, 2014-06-17 at 14:42 -0500, Caraman Mihai Claudiu-B02008 wrote:
> > > > > -static DEFINE_PER_CPU(struct kvm_vcpu *, last_vcpu_on_cpu);
> > > > > +static DEFINE_PER_CPU(struct kvm_vcpu * [KVMPPC_NR_LPIDS],
> > > > last_vcpu_on_cpu);
> > > >
> > > > Hmm, I didn't know you could express types like that.  Is this
> > special
> > > > syntax that only works for typeof?
> > >
> > > Yes, AFAIK.
> > >
> > > > No space after *
> > >
> > > Checkpatch complains about the missing space ;)
> > 
> > Checkpatch is wrong, which isn't surprising given that this is unusual
> > syntax.  We don't normally put a space after * when used to represent a
> > pointer.
> 
> This is not something new. See [PATCH 04/10] percpu: cleanup percpu array
> definitions:
> 
> 	https://lkml.org/lkml/2009/6/24/26

I didn't say it was new, just unusual, and checkpatch doesn't recognize
it.  Checkpatch shouldn't be blindly and silently obeyed when it says
something strange.

-Scott

RE: [PATCH v3] KVM: PPC: e500mc: Enhance tlb invalidation condition on vcpu schedule

[mihai.caraman]

> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Tuesday, June 17, 2014 11:05 PM
> To: Caraman Mihai Claudiu-B02008
> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; linuxppc-
> dev@lists.ozlabs.org
> Subject: Re: [PATCH v3] KVM: PPC: e500mc: Enhance tlb invalidation
> condition on vcpu schedule
> 
> On Tue, 2014-06-17 at 15:02 -0500, Caraman Mihai Claudiu-B02008 wrote:
> > > -----Original Message-----
> > > From: Wood Scott-B07421
> > > Sent: Tuesday, June 17, 2014 10:48 PM
> > > To: Caraman Mihai Claudiu-B02008
> > > Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; linuxppc-
> > > dev@lists.ozlabs.org
> > > Subject: Re: [PATCH v3] KVM: PPC: e500mc: Enhance tlb invalidation
> > > condition on vcpu schedule
> > >
> > > On Tue, 2014-06-17 at 14:42 -0500, Caraman Mihai Claudiu-B02008
> wrote:
> > > > > > -static DEFINE_PER_CPU(struct kvm_vcpu *, last_vcpu_on_cpu);
> > > > > > +static DEFINE_PER_CPU(struct kvm_vcpu * [KVMPPC_NR_LPIDS],
> > > > > last_vcpu_on_cpu);
> > > > >
> > > > > Hmm, I didn't know you could express types like that.  Is this
> > > special
> > > > > syntax that only works for typeof?
> > > >
> > > > Yes, AFAIK.
> > > >
> > > > > No space after *
> > > >
> > > > Checkpatch complains about the missing space ;)
> > >
> > > Checkpatch is wrong, which isn't surprising given that this is
> unusual
> > > syntax.  We don't normally put a space after * when used to represent
> a
> > > pointer.
> >
> > This is not something new. See [PATCH 04/10] percpu: cleanup percpu
> array
> > definitions:
> >
> > 	https://lkml.org/lkml/2009/6/24/26
> 
> I didn't say it was new, just unusual, and checkpatch doesn't recognize
> it.  Checkpatch shouldn't be blindly and silently obeyed when it says
> something strange.

I agree with you about the syntax and I know other cases where checkpatch
is a moron. For similar corner cases checkpatch maintainers did not wanted
(or found it difficult) to make an exception. I would also like to see Alex
opinion on this.

-Mike

Re: [PATCH v3] KVM: PPC: e500mc: Enhance tlb invalidation condition on vcpu schedule

[Alexander Graf]

On 17.06.14 22:36, mihai.caraman@freescale.com wrote:
>> -----Original Message-----
>> From: Wood Scott-B07421
>> Sent: Tuesday, June 17, 2014 11:05 PM
>> To: Caraman Mihai Claudiu-B02008
>> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; linuxppc-
>> dev@lists.ozlabs.org
>> Subject: Re: [PATCH v3] KVM: PPC: e500mc: Enhance tlb invalidation
>> condition on vcpu schedule
>>
>> On Tue, 2014-06-17 at 15:02 -0500, Caraman Mihai Claudiu-B02008 wrote:
>>>> -----Original Message-----
>>>> From: Wood Scott-B07421
>>>> Sent: Tuesday, June 17, 2014 10:48 PM
>>>> To: Caraman Mihai Claudiu-B02008
>>>> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; linuxppc-
>>>> dev@lists.ozlabs.org
>>>> Subject: Re: [PATCH v3] KVM: PPC: e500mc: Enhance tlb invalidation
>>>> condition on vcpu schedule
>>>>
>>>> On Tue, 2014-06-17 at 14:42 -0500, Caraman Mihai Claudiu-B02008
>> wrote:
>>>>>>> -static DEFINE_PER_CPU(struct kvm_vcpu *, last_vcpu_on_cpu);
>>>>>>> +static DEFINE_PER_CPU(struct kvm_vcpu * [KVMPPC_NR_LPIDS],
>>>>>> last_vcpu_on_cpu);
>>>>>>
>>>>>> Hmm, I didn't know you could express types like that.  Is this
>>>> special
>>>>>> syntax that only works for typeof?
>>>>> Yes, AFAIK.
>>>>>
>>>>>> No space after *
>>>>> Checkpatch complains about the missing space ;)
>>>> Checkpatch is wrong, which isn't surprising given that this is
>> unusual
>>>> syntax.  We don't normally put a space after * when used to represent
>> a
>>>> pointer.
>>> This is not something new. See [PATCH 04/10] percpu: cleanup percpu
>> array
>>> definitions:
>>>
>>> 	https://lkml.org/lkml/2009/6/24/26
>> I didn't say it was new, just unusual, and checkpatch doesn't recognize
>> it.  Checkpatch shouldn't be blindly and silently obeyed when it says
>> something strange.
> I agree with you about the syntax and I know other cases where checkpatch
> is a moron. For similar corner cases checkpatch maintainers did not wanted
> (or found it difficult) to make an exception. I would also like to see Alex
> opinion on this.

I usually like to apply common sense :).


Alex