Re: [PATCH v3] KVM: PPC: e500mc: Enhance tlb invalidation condition on vcpu schedule
On 17.06.14 22:36, mihai.cara...@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; k...@vger.kernel.org; linuxppc- d...@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; k...@vger.kernel.org; linuxppc- d...@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 -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v3] KVM: PPC: e500mc: Enhance tlb invalidation condition on vcpu schedule
> -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; k...@vger.kernel.org; linuxppc- > d...@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; k...@vger.kernel.org; linuxppc- > > > d...@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
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; k...@vger.kernel.org; linuxppc- > > d...@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 -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v3] KVM: PPC: e500mc: Enhance tlb invalidation condition on vcpu schedule
> -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; k...@vger.kernel.org; linuxppc-
> d...@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
N�r��yb�X��ǧv�^�){.n�+jir)w* jg��� �ݢj/���z�ޖ��2�ޙ&�)ߡ�a���� �G���h��j:+v���w��٥
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. -Scott -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v3] KVM: PPC: e500mc: Enhance tlb invalidation condition on vcpu schedule
> > -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
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
> Cc: Scott Wood
> ---
> 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
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3] KVM: PPC: e500mc: Enhance tlb invalidation condition on vcpu schedule
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
Cc: Scott Wood
---
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
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
