| Message ID | 1403032176-28362-1-git-send-email-mihai.caraman@freescale.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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
> > -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
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
> -----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
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
> -----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
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
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);
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(-)