[PATCH] Using the tlb flush util function where applicable
Using kvm_mmu_flush_tlb as the other places to make sure vcpu stat is incremented Signed-off-by: Liang Chen --- arch/x86/kvm/vmx.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index bfe11cf..439682e 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -1810,7 +1810,7 @@ static void vmx_vcpu_load(struct kvm_vcpu *vcpu, int cpu) struct desc_ptr *gdt = &__get_cpu_var(host_gdt); unsigned long sysenter_esp; - kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu); + kvm_mmu_flush_tlb(vcpu); local_irq_disable(); crash_disable_local_vmclear(cpu); -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Using the tlb flush util function where applicable
2014-09-12 17:06-0400, Liang Chen:
> Using kvm_mmu_flush_tlb as the other places to make sure vcpu
> stat is incremented
>
> Signed-off-by: Liang Chen
> ---
Good catch.
> arch/x86/kvm/vmx.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index bfe11cf..439682e 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -1810,7 +1810,7 @@ static void vmx_vcpu_load(struct kvm_vcpu *vcpu, int
> cpu)
> struct desc_ptr *gdt = &__get_cpu_var(host_gdt);
> unsigned long sysenter_esp;
>
> - kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu);
> + kvm_mmu_flush_tlb(vcpu);
> local_irq_disable();
> crash_disable_local_vmclear(cpu);
>
And to hijack this thread ...
I noticed three other deficits in stat.tlb_flush, patch below.
Do you prefer the current behavior?
--- 8< ---
KVM: x86: count actual tlb flushes
- we count KVM_REQ_TLB_FLUSH requests, not actual flushes
(KVM can have multiple requests for one flush)
- flushes from kvm_flush_remote_tlbs aren't counted
- it's easy to make a direct request by mistake
Solve these by postponing the counting to kvm_check_request().
Signed-off-by: Radim Krčmář
---
(And what about a possible followup patch that replaces
kvm_mmu_flush_tlb() with kvm_make_request() again?
It would free the namespace a bit and we could call something
similarly named from vcpu_enter_guest() to do the job.)
diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 9314678..b41fd97 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -3452,7 +3452,6 @@ static void nonpaging_init_context(struct kvm_vcpu *vcpu,
void kvm_mmu_flush_tlb(struct kvm_vcpu *vcpu)
{
- ++vcpu->stat.tlb_flush;
kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu);
}
EXPORT_SYMBOL_GPL(kvm_mmu_flush_tlb);
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 916e895..0f0ad08 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6041,8 +6041,10 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
}
if (kvm_check_request(KVM_REQ_MMU_SYNC, vcpu))
kvm_mmu_sync_roots(vcpu);
- if (kvm_check_request(KVM_REQ_TLB_FLUSH, vcpu))
+ if (kvm_check_request(KVM_REQ_TLB_FLUSH, vcpu)) {
+ ++vcpu->stat.tlb_flush;
kvm_x86_ops->tlb_flush(vcpu);
+ }
if (kvm_check_request(KVM_REQ_REPORT_TPR_ACCESS, vcpu)) {
vcpu->run->exit_reason = KVM_EXIT_TPR_ACCESS;
r = 0;
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Using the tlb flush util function where applicable
On 09/15/2014 03:33 PM, Radim Krčmář wrote:
> 2014-09-12 17:06-0400, Liang Chen:
>> Using kvm_mmu_flush_tlb as the other places to make sure vcpu
>> stat is incremented
>>
>> Signed-off-by: Liang Chen
>> ---
> Good catch.
>
>> arch/x86/kvm/vmx.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index bfe11cf..439682e 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -1810,7 +1810,7 @@ static void vmx_vcpu_load(struct kvm_vcpu *vcpu, int
>> cpu)
>> struct desc_ptr *gdt = &__get_cpu_var(host_gdt);
>> unsigned long sysenter_esp;
>>
>> -kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu);
>> +kvm_mmu_flush_tlb(vcpu);
>> local_irq_disable();
>> crash_disable_local_vmclear(cpu);
>>
> And to hijack this thread ...
> I noticed three other deficits in stat.tlb_flush, patch below.
>
> Do you prefer the current behavior?
Yeah, you are right. I think the patch below gives a more thorough solution to
make sure the counter really shows the number of tlb flushes - not the time
kvm_mmu_flush_tlb is called. Thanks!
> --- 8< ---
> KVM: x86: count actual tlb flushes
>
> - we count KVM_REQ_TLB_FLUSH requests, not actual flushes
> (KVM can have multiple requests for one flush)
> - flushes from kvm_flush_remote_tlbs aren't counted
> - it's easy to make a direct request by mistake
>
> Solve these by postponing the counting to kvm_check_request().
>
> Signed-off-by: Radim Krčmář
> ---
> (And what about a possible followup patch that replaces
> kvm_mmu_flush_tlb() with kvm_make_request() again?
> It would free the namespace a bit and we could call something
> similarly named from vcpu_enter_guest() to do the job.)
That seems good. I can take the labor to make the followup patch ;)
Thanks,
Liang Chen
> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index 9314678..b41fd97 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -3452,7 +3452,6 @@ static void nonpaging_init_context(struct kvm_vcpu
> *vcpu,
>
> void kvm_mmu_flush_tlb(struct kvm_vcpu *vcpu)
> {
> - ++vcpu->stat.tlb_flush;
> kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu);
> }
> EXPORT_SYMBOL_GPL(kvm_mmu_flush_tlb);
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 916e895..0f0ad08 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6041,8 +6041,10 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
> }
> if (kvm_check_request(KVM_REQ_MMU_SYNC, vcpu))
> kvm_mmu_sync_roots(vcpu);
> - if (kvm_check_request(KVM_REQ_TLB_FLUSH, vcpu))
> + if (kvm_check_request(KVM_REQ_TLB_FLUSH, vcpu)) {
> + ++vcpu->stat.tlb_flush;
> kvm_x86_ops->tlb_flush(vcpu);
> + }
> if (kvm_check_request(KVM_REQ_REPORT_TPR_ACCESS, vcpu)) {
> vcpu->run->exit_reason = KVM_EXIT_TPR_ACCESS;
> r = 0;
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Using the tlb flush util function where applicable
Il 16/09/2014 00:49, Liang Chen ha scritto: >> > --- >> > (And what about a possible followup patch that replaces >> > kvm_mmu_flush_tlb() with kvm_make_request() again? >> > It would free the namespace a bit and we could call something >> > similarly named from vcpu_enter_guest() to do the job.) > That seems good. I can take the labor to make the followup patch ;) Ok, so I'll not apply your first patch. Thanks! Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Using the tlb flush util function where applicable
Hi Radim,
On Mon, Sep 15, 2014 at 09:33:52PM +0200, Radim Krčmář wrote:
>2014-09-12 17:06-0400, Liang Chen:
>> Using kvm_mmu_flush_tlb as the other places to make sure vcpu
>> stat is incremented
>>
>> Signed-off-by: Liang Chen
>> ---
>
>Good catch.
>
>> arch/x86/kvm/vmx.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index bfe11cf..439682e 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -1810,7 +1810,7 @@ static void vmx_vcpu_load(struct kvm_vcpu *vcpu, int
>> cpu)
>> struct desc_ptr *gdt = &__get_cpu_var(host_gdt);
>> unsigned long sysenter_esp;
>>
>> -kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu);
>> +kvm_mmu_flush_tlb(vcpu);
>> local_irq_disable();
>> crash_disable_local_vmclear(cpu);
>>
>
>And to hijack this thread ...
>I noticed three other deficits in stat.tlb_flush, patch below.
>
>Do you prefer the current behavior?
>
>--- 8< ---
>KVM: x86: count actual tlb flushes
>
>- we count KVM_REQ_TLB_FLUSH requests, not actual flushes
So there maybe multiple requests accumulated at the point of kvm_check_request,
if your patch account these accumulations correctly?
Regards,
Wanpeng Li
> (KVM can have multiple requests for one flush)
>- flushes from kvm_flush_remote_tlbs aren't counted
>- it's easy to make a direct request by mistake
>
>Solve these by postponing the counting to kvm_check_request().
>
>Signed-off-by: Radim Krčmář
>---
>(And what about a possible followup patch that replaces
> kvm_mmu_flush_tlb() with kvm_make_request() again?
> It would free the namespace a bit and we could call something
> similarly named from vcpu_enter_guest() to do the job.)
>
>diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
>index 9314678..b41fd97 100644
>--- a/arch/x86/kvm/mmu.c
>+++ b/arch/x86/kvm/mmu.c
>@@ -3452,7 +3452,6 @@ static void nonpaging_init_context(struct kvm_vcpu *vcpu,
>
> void kvm_mmu_flush_tlb(struct kvm_vcpu *vcpu)
> {
>- ++vcpu->stat.tlb_flush;
> kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu);
> }
> EXPORT_SYMBOL_GPL(kvm_mmu_flush_tlb);
>diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>index 916e895..0f0ad08 100644
>--- a/arch/x86/kvm/x86.c
>+++ b/arch/x86/kvm/x86.c
>@@ -6041,8 +6041,10 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
> }
> if (kvm_check_request(KVM_REQ_MMU_SYNC, vcpu))
> kvm_mmu_sync_roots(vcpu);
>- if (kvm_check_request(KVM_REQ_TLB_FLUSH, vcpu))
>+ if (kvm_check_request(KVM_REQ_TLB_FLUSH, vcpu)) {
>+ ++vcpu->stat.tlb_flush;
> kvm_x86_ops->tlb_flush(vcpu);
>+ }
> if (kvm_check_request(KVM_REQ_REPORT_TPR_ACCESS, vcpu)) {
> vcpu->run->exit_reason = KVM_EXIT_TPR_ACCESS;
> r = 0;
>--
>To unsubscribe from this list: send the line "unsubscribe kvm" in
>the body of a message to majord...@vger.kernel.org
>More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Using the tlb flush util function where applicable
2014-09-17 08:15+0800, Wanpeng Li:
> Hi Radim,
> On Mon, Sep 15, 2014 at 09:33:52PM +0200, Radim Krčmář wrote:
> >Do you prefer the current behavior?
> >
> >--- 8< ---
> >KVM: x86: count actual tlb flushes
> >
> >- we count KVM_REQ_TLB_FLUSH requests, not actual flushes
>
> So there maybe multiple requests accumulated at the point of
> kvm_check_request,
> if your patch account these accumulations correctly?
It will ignore request accumulations and count it as one TLB flush,
we have to decide what is correct (the value is just statistics)
a) count local KVM_REQ_TLB_FLUSH requests
b) count all TLB flushes
c) both (a) and (b)
I was thinking that when you look at /sys/kernel/debug/kvm/tlb_flushes,
you are interested in the number of TLB flushes that VMs did, not
requests, so you won't have to add remote_tlb_flushes multiplied by
maximal vcpu count and guess their amount from this upper bound.
And that we don't even care about requests, so (c) is just complication.
---
I tried to get an idea about the number of coalesced requests and added
a counter called tlb_flush_real, making option (c).
After a night of reading virtio-rng (different experiment) on 1 VCPU VM:
# cat /sys/kernel/debug/kvm/{tlb_flush{,_real},remote_tlb_flush}
5927
5206
0
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Using the tlb flush util function where applicable
Il 17/09/2014 12:45, Radim Krčmář ha scritto: > a) count local KVM_REQ_TLB_FLUSH requests > b) count all TLB flushes > c) both (a) and (b) > > I was thinking that when you look at /sys/kernel/debug/kvm/tlb_flushes, > you are interested in the number of TLB flushes that VMs did, not > requests, so you won't have to add remote_tlb_flushes multiplied by > maximal vcpu count and guess their amount from this upper bound. I agree. The difference isn't too big, but if two requests are coalesced they will only cause a hit once. Paolo > And that we don't even care about requests, so (c) is just complication. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
