[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]

PiA+IC1zdGF0aWMgREVGSU5FX1BFUl9DUFUoc3RydWN0IGt2bV92Y3B1ICosIGxhc3RfdmNwdV9v
bl9jcHUpOw0KPiA+ICtzdGF0aWMgREVGSU5FX1BFUl9DUFUoc3RydWN0IGt2bV92Y3B1ICogW0tW
TVBQQ19OUl9MUElEU10sDQo+IGxhc3RfdmNwdV9vbl9jcHUpOw0KPiANCj4gSG1tLCBJIGRpZG4n
dCBrbm93IHlvdSBjb3VsZCBleHByZXNzIHR5cGVzIGxpa2UgdGhhdC4gIElzIHRoaXMgc3BlY2lh
bA0KPiBzeW50YXggdGhhdCBvbmx5IHdvcmtzIGZvciB0eXBlb2Y/DQoNClllcywgQUZBSUsuDQoN
Cj4gTm8gc3BhY2UgYWZ0ZXIgKg0KDQpDaGVja3BhdGNoIGNvbXBsYWlucyBhYm91dCB0aGUgbWlz
c2luZyBzcGFjZSA7KQ0KDQo+IA0KPiBOYW1lIHNob3VsZCBiZSBhZGp1c3RlZCB0byBtYXRjaCwg
c29tZXRoaW5nIGxpa2UgbGFzdF92Y3B1X29mX2xwaWQgKHdpdGgNCj4gdGhlIF9vbl9jcHUgYmVp
bmcgaW1wbGllZCBieSB0aGUgZmFjdCB0aGF0IGl0J3MgUEVSX0NQVSkuDQoNCkkgd2FzIHRoaW5r
aW5nIHRvIHRoZSBsb25nIG5hbWUgYnV0IGl0IHdhcyBub3QgYXBwZWFsaW5nLCBJIHdpbGwgY2hh
bmdlIGl0IHRvDQpsYXN0X3ZjcHVfb2ZfbHBpZC4NCg0KLU1pa2UNCg==

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]

PiAtLS0tLU9yaWdpbmFsIE1lc3NhZ2UtLS0tLQ0KPiBGcm9tOiBXb29kIFNjb3R0LUIwNzQyMQ0K
PiBTZW50OiBUdWVzZGF5LCBKdW5lIDE3LCAyMDE0IDEwOjQ4IFBNDQo+IFRvOiBDYXJhbWFuIE1p
aGFpIENsYXVkaXUtQjAyMDA4DQo+IENjOiBrdm0tcHBjQHZnZXIua2VybmVsLm9yZzsga3ZtQHZn
ZXIua2VybmVsLm9yZzsgbGludXhwcGMtDQo+IGRldkBsaXN0cy5vemxhYnMub3JnDQo+IFN1Ympl
Y3Q6IFJlOiBbUEFUQ0ggdjNdIEtWTTogUFBDOiBlNTAwbWM6IEVuaGFuY2UgdGxiIGludmFsaWRh
dGlvbg0KPiBjb25kaXRpb24gb24gdmNwdSBzY2hlZHVsZQ0KPiANCj4gT24gVHVlLCAyMDE0LTA2
LTE3IGF0IDE0OjQyIC0wNTAwLCBDYXJhbWFuIE1paGFpIENsYXVkaXUtQjAyMDA4IHdyb3RlOg0K
PiA+ID4gPiAtc3RhdGljIERFRklORV9QRVJfQ1BVKHN0cnVjdCBrdm1fdmNwdSAqLCBsYXN0X3Zj
cHVfb25fY3B1KTsNCj4gPiA+ID4gK3N0YXRpYyBERUZJTkVfUEVSX0NQVShzdHJ1Y3Qga3ZtX3Zj
cHUgKiBbS1ZNUFBDX05SX0xQSURTXSwNCj4gPiA+IGxhc3RfdmNwdV9vbl9jcHUpOw0KPiA+ID4N
Cj4gPiA+IEhtbSwgSSBkaWRuJ3Qga25vdyB5b3UgY291bGQgZXhwcmVzcyB0eXBlcyBsaWtlIHRo
YXQuICBJcyB0aGlzDQo+IHNwZWNpYWwNCj4gPiA+IHN5bnRheCB0aGF0IG9ubHkgd29ya3MgZm9y
IHR5cGVvZj8NCj4gPg0KPiA+IFllcywgQUZBSUsuDQo+ID4NCj4gPiA+IE5vIHNwYWNlIGFmdGVy
ICoNCj4gPg0KPiA+IENoZWNrcGF0Y2ggY29tcGxhaW5zIGFib3V0IHRoZSBtaXNzaW5nIHNwYWNl
IDspDQo+IA0KPiBDaGVja3BhdGNoIGlzIHdyb25nLCB3aGljaCBpc24ndCBzdXJwcmlzaW5nIGdp
dmVuIHRoYXQgdGhpcyBpcyB1bnVzdWFsDQo+IHN5bnRheC4gIFdlIGRvbid0IG5vcm1hbGx5IHB1
dCBhIHNwYWNlIGFmdGVyICogd2hlbiB1c2VkIHRvIHJlcHJlc2VudCBhDQo+IHBvaW50ZXIuDQoN
ClRoaXMgaXMgbm90IHNvbWV0aGluZyBuZXcuIFNlZSBbUEFUQ0ggMDQvMTBdIHBlcmNwdTogY2xl
YW51cCBwZXJjcHUgYXJyYXkNCmRlZmluaXRpb25zOg0KDQoJaHR0cHM6Ly9sa21sLm9yZy9sa21s
LzIwMDkvNi8yNC8yNg0KDQotTWlrZQ0K

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]

PiAtLS0tLU9yaWdpbmFsIE1lc3NhZ2UtLS0tLQ0KPiBGcm9tOiBXb29kIFNjb3R0LUIwNzQyMQ0K
PiBTZW50OiBUdWVzZGF5LCBKdW5lIDE3LCAyMDE0IDExOjA1IFBNDQo+IFRvOiBDYXJhbWFuIE1p
aGFpIENsYXVkaXUtQjAyMDA4DQo+IENjOiBrdm0tcHBjQHZnZXIua2VybmVsLm9yZzsga3ZtQHZn
ZXIua2VybmVsLm9yZzsgbGludXhwcGMtDQo+IGRldkBsaXN0cy5vemxhYnMub3JnDQo+IFN1Ympl
Y3Q6IFJlOiBbUEFUQ0ggdjNdIEtWTTogUFBDOiBlNTAwbWM6IEVuaGFuY2UgdGxiIGludmFsaWRh
dGlvbg0KPiBjb25kaXRpb24gb24gdmNwdSBzY2hlZHVsZQ0KPiANCj4gT24gVHVlLCAyMDE0LTA2
LTE3IGF0IDE1OjAyIC0wNTAwLCBDYXJhbWFuIE1paGFpIENsYXVkaXUtQjAyMDA4IHdyb3RlOg0K
PiA+ID4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gPiA+IEZyb206IFdvb2QgU2NvdHQt
QjA3NDIxDQo+ID4gPiBTZW50OiBUdWVzZGF5LCBKdW5lIDE3LCAyMDE0IDEwOjQ4IFBNDQo+ID4g
PiBUbzogQ2FyYW1hbiBNaWhhaSBDbGF1ZGl1LUIwMjAwOA0KPiA+ID4gQ2M6IGt2bS1wcGNAdmdl
ci5rZXJuZWwub3JnOyBrdm1Admdlci5rZXJuZWwub3JnOyBsaW51eHBwYy0NCj4gPiA+IGRldkBs
aXN0cy5vemxhYnMub3JnDQo+ID4gPiBTdWJqZWN0OiBSZTogW1BBVENIIHYzXSBLVk06IFBQQzog
ZTUwMG1jOiBFbmhhbmNlIHRsYiBpbnZhbGlkYXRpb24NCj4gPiA+IGNvbmRpdGlvbiBvbiB2Y3B1
IHNjaGVkdWxlDQo+ID4gPg0KPiA+ID4gT24gVHVlLCAyMDE0LTA2LTE3IGF0IDE0OjQyIC0wNTAw
LCBDYXJhbWFuIE1paGFpIENsYXVkaXUtQjAyMDA4DQo+IHdyb3RlOg0KPiA+ID4gPiA+ID4gLXN0
YXRpYyBERUZJTkVfUEVSX0NQVShzdHJ1Y3Qga3ZtX3ZjcHUgKiwgbGFzdF92Y3B1X29uX2NwdSk7
DQo+ID4gPiA+ID4gPiArc3RhdGljIERFRklORV9QRVJfQ1BVKHN0cnVjdCBrdm1fdmNwdSAqIFtL
Vk1QUENfTlJfTFBJRFNdLA0KPiA+ID4gPiA+IGxhc3RfdmNwdV9vbl9jcHUpOw0KPiA+ID4gPiA+
DQo+ID4gPiA+ID4gSG1tLCBJIGRpZG4ndCBrbm93IHlvdSBjb3VsZCBleHByZXNzIHR5cGVzIGxp
a2UgdGhhdC4gIElzIHRoaXMNCj4gPiA+IHNwZWNpYWwNCj4gPiA+ID4gPiBzeW50YXggdGhhdCBv
bmx5IHdvcmtzIGZvciB0eXBlb2Y/DQo+ID4gPiA+DQo+ID4gPiA+IFllcywgQUZBSUsuDQo+ID4g
PiA+DQo+ID4gPiA+ID4gTm8gc3BhY2UgYWZ0ZXIgKg0KPiA+ID4gPg0KPiA+ID4gPiBDaGVja3Bh
dGNoIGNvbXBsYWlucyBhYm91dCB0aGUgbWlzc2luZyBzcGFjZSA7KQ0KPiA+ID4NCj4gPiA+IENo
ZWNrcGF0Y2ggaXMgd3JvbmcsIHdoaWNoIGlzbid0IHN1cnByaXNpbmcgZ2l2ZW4gdGhhdCB0aGlz
IGlzDQo+IHVudXN1YWwNCj4gPiA+IHN5bnRheC4gIFdlIGRvbid0IG5vcm1hbGx5IHB1dCBhIHNw
YWNlIGFmdGVyICogd2hlbiB1c2VkIHRvIHJlcHJlc2VudA0KPiBhDQo+ID4gPiBwb2ludGVyLg0K
PiA+DQo+ID4gVGhpcyBpcyBub3Qgc29tZXRoaW5nIG5ldy4gU2VlIFtQQVRDSCAwNC8xMF0gcGVy
Y3B1OiBjbGVhbnVwIHBlcmNwdQ0KPiBhcnJheQ0KPiA+IGRlZmluaXRpb25zOg0KPiA+DQo+ID4g
CWh0dHBzOi8vbGttbC5vcmcvbGttbC8yMDA5LzYvMjQvMjYNCj4gDQo+IEkgZGlkbid0IHNheSBp
dCB3YXMgbmV3LCBqdXN0IHVudXN1YWwsIGFuZCBjaGVja3BhdGNoIGRvZXNuJ3QgcmVjb2duaXpl
DQo+IGl0LiAgQ2hlY2twYXRjaCBzaG91bGRuJ3QgYmUgYmxpbmRseSBhbmQgc2lsZW50bHkgb2Jl
eWVkIHdoZW4gaXQgc2F5cw0KPiBzb21ldGhpbmcgc3RyYW5nZS4NCg0KSSBhZ3JlZSB3aXRoIHlv
dSBhYm91dCB0aGUgc3ludGF4IGFuZCBJIGtub3cgb3RoZXIgY2FzZXMgd2hlcmUgY2hlY2twYXRj
aA0KaXMgYSBtb3Jvbi4gRm9yIHNpbWlsYXIgY29ybmVyIGNhc2VzIGNoZWNrcGF0Y2ggbWFpbnRh
aW5lcnMgZGlkIG5vdCB3YW50ZWQNCihvciBmb3VuZCBpdCBkaWZmaWN1bHQpIHRvIG1ha2UgYW4g
ZXhjZXB0aW9uLiBJIHdvdWxkIGFsc28gbGlrZSB0byBzZWUgQWxleA0Kb3BpbmlvbiBvbiB0aGlz
Lg0KDQotTWlrZQ0KDQoNCg==

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