diff mbox

KVM: Don't use vcpu->requests for steal time accounting

Message ID 20121214193718.efd714cf.yoshikawa_takuya_b1@lab.ntt.co.jp (mailing list archive)
State New, archived
Headers show

Commit Message

Takuya Yoshikawa Dec. 14, 2012, 10:37 a.m. UTC 
We can check if accum_steal has any positive value instead of using
KVM_REQ_STEAL_UPDATE bit in vcpu->requests; and this is the way we
usually do for accounting for something in the kernel.

Signed-off-by: Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp>
---
 arch/x86/kvm/x86.c       |   11 +++++------
 include/linux/kvm_host.h |   41 ++++++++++++++++++++---------------------
 2 files changed, 25 insertions(+), 27 deletions(-)

Comments

Gleb Natapov Dec. 14, 2012, 11:28 a.m. UTC | #1 
On Fri, Dec 14, 2012 at 07:37:18PM +0900, Takuya Yoshikawa wrote:
> We can check if accum_steal has any positive value instead of using
> KVM_REQ_STEAL_UPDATE bit in vcpu->requests; and this is the way we
> usually do for accounting for something in the kernel.
> 
Now you added check that will be done on each guest entry, requests
mechanism prevents that.

> Signed-off-by: Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp>
> ---
>  arch/x86/kvm/x86.c       |   11 +++++------
>  include/linux/kvm_host.h |   41 ++++++++++++++++++++---------------------
>  2 files changed, 25 insertions(+), 27 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 57c76e8..fab4c3e 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1857,6 +1857,9 @@ static void accumulate_steal_time(struct kvm_vcpu *vcpu)
>  
>  static void record_steal_time(struct kvm_vcpu *vcpu)
>  {
> +	if (!vcpu->arch.st.accum_steal)
> +		return;
> +
>  	if (!(vcpu->arch.st.msr_val & KVM_MSR_ENABLED))
>  		return;
>  
> @@ -1992,9 +1995,6 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  		preempt_disable();
>  		accumulate_steal_time(vcpu);
>  		preempt_enable();
> -
> -		kvm_make_request(KVM_REQ_STEAL_UPDATE, vcpu);
> -
>  		break;
>  	case MSR_KVM_PV_EOI_EN:
>  		if (kvm_lapic_enable_pv_eoi(vcpu, data))
> @@ -2668,7 +2668,6 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>  	}
>  
>  	accumulate_steal_time(vcpu);
> -	kvm_make_request(KVM_REQ_STEAL_UPDATE, vcpu);
>  }
>  
>  void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
> @@ -5645,8 +5644,6 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>  			r = 1;
>  			goto out;
>  		}
> -		if (kvm_check_request(KVM_REQ_STEAL_UPDATE, vcpu))
> -			record_steal_time(vcpu);
>  		if (kvm_check_request(KVM_REQ_NMI, vcpu))
>  			process_nmi(vcpu);
>  		req_immediate_exit =
> @@ -5672,6 +5669,8 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>  		}
>  	}
>  
> +	record_steal_time(vcpu);
> +
>  	r = kvm_mmu_reload(vcpu);
>  	if (unlikely(r)) {
>  		goto cancel_injection;
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 91ae127..5476ffc 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -112,27 +112,26 @@ static inline bool is_error_page(struct page *page)
>  /*
>   * vcpu->requests bit members
>   */
> -#define KVM_REQ_TLB_FLUSH          0
> -#define KVM_REQ_MIGRATE_TIMER      1
> -#define KVM_REQ_REPORT_TPR_ACCESS  2
> -#define KVM_REQ_MMU_RELOAD         3
> -#define KVM_REQ_TRIPLE_FAULT       4
> -#define KVM_REQ_PENDING_TIMER      5
> -#define KVM_REQ_UNHALT             6
> -#define KVM_REQ_MMU_SYNC           7
> -#define KVM_REQ_CLOCK_UPDATE       8
> -#define KVM_REQ_KICK               9
> -#define KVM_REQ_DEACTIVATE_FPU    10
> -#define KVM_REQ_EVENT             11
> -#define KVM_REQ_APF_HALT          12
> -#define KVM_REQ_STEAL_UPDATE      13
> -#define KVM_REQ_NMI               14
> -#define KVM_REQ_IMMEDIATE_EXIT    15
> -#define KVM_REQ_PMU               16
> -#define KVM_REQ_PMI               17
> -#define KVM_REQ_WATCHDOG          18
> -#define KVM_REQ_MASTERCLOCK_UPDATE 19
> -#define KVM_REQ_MCLOCK_INPROGRESS 20
> +#define KVM_REQ_TLB_FLUSH		0
> +#define KVM_REQ_MIGRATE_TIMER		1
> +#define KVM_REQ_REPORT_TPR_ACCESS	2
> +#define KVM_REQ_MMU_RELOAD		3
> +#define KVM_REQ_TRIPLE_FAULT		4
> +#define KVM_REQ_PENDING_TIMER		5
> +#define KVM_REQ_UNHALT			6
> +#define KVM_REQ_MMU_SYNC		7
> +#define KVM_REQ_CLOCK_UPDATE		8
> +#define KVM_REQ_KICK			9
> +#define KVM_REQ_DEACTIVATE_FPU		10
> +#define KVM_REQ_EVENT			11
> +#define KVM_REQ_APF_HALT		12
> +#define KVM_REQ_NMI			13
> +#define KVM_REQ_IMMEDIATE_EXIT		14
> +#define KVM_REQ_PMU			15
> +#define KVM_REQ_PMI			16
> +#define KVM_REQ_WATCHDOG		17
> +#define KVM_REQ_MASTERCLOCK_UPDATE	18
> +#define KVM_REQ_MCLOCK_INPROGRESS	19
>  
>  #define KVM_USERSPACE_IRQ_SOURCE_ID		0
>  #define KVM_IRQFD_RESAMPLE_IRQ_SOURCE_ID	1
> -- 
> 1.7.5.4

--
			Gleb.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Takuya Yoshikawa Dec. 14, 2012, 3:12 p.m. UTC | #2 
On Fri, 14 Dec 2012 13:28:15 +0200
Gleb Natapov <gleb@redhat.com> wrote:

> On Fri, Dec 14, 2012 at 07:37:18PM +0900, Takuya Yoshikawa wrote:
> > We can check if accum_steal has any positive value instead of using
> > KVM_REQ_STEAL_UPDATE bit in vcpu->requests; and this is the way we
> > usually do for accounting for something in the kernel.
> > 
> Now you added check that will be done on each guest entry, requests
> mechanism prevents that.

Yes, +1 "if" for the case we have nothing in requests.

I'm not sure if setting and clearing a bit for that minor
optimization is worth it.

Thanks,
	Takuya
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Gleb Natapov Dec. 14, 2012, 3:41 p.m. UTC | #3 
On Sat, Dec 15, 2012 at 12:12:08AM +0900, Takuya Yoshikawa wrote:
> On Fri, 14 Dec 2012 13:28:15 +0200
> Gleb Natapov <gleb@redhat.com> wrote:
> 
> > On Fri, Dec 14, 2012 at 07:37:18PM +0900, Takuya Yoshikawa wrote:
> > > We can check if accum_steal has any positive value instead of using
> > > KVM_REQ_STEAL_UPDATE bit in vcpu->requests; and this is the way we
> > > usually do for accounting for something in the kernel.
> > > 
> > Now you added check that will be done on each guest entry, requests
> > mechanism prevents that.
> 
> Yes, +1 "if" for the case we have nothing in requests.
> 
Almost any bit in requests can be replaced by one "if". Those
if's add up.

> I'm not sure if setting and clearing a bit for that minor
> optimization is worth it.
> 
Setting/clearing it should be much more rare than guest entry.

--
			Gleb.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 57c76e8..fab4c3e 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1857,6 +1857,9 @@  static void accumulate_steal_time(struct kvm_vcpu *vcpu)
 
 static void record_steal_time(struct kvm_vcpu *vcpu)
 {
+	if (!vcpu->arch.st.accum_steal)
+		return;
+
 	if (!(vcpu->arch.st.msr_val & KVM_MSR_ENABLED))
 		return;
 
@@ -1992,9 +1995,6 @@  int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		preempt_disable();
 		accumulate_steal_time(vcpu);
 		preempt_enable();
-
-		kvm_make_request(KVM_REQ_STEAL_UPDATE, vcpu);
-
 		break;
 	case MSR_KVM_PV_EOI_EN:
 		if (kvm_lapic_enable_pv_eoi(vcpu, data))
@@ -2668,7 +2668,6 @@  void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
 	}
 
 	accumulate_steal_time(vcpu);
-	kvm_make_request(KVM_REQ_STEAL_UPDATE, vcpu);
 }
 
 void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
@@ -5645,8 +5644,6 @@  static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 			r = 1;
 			goto out;
 		}
-		if (kvm_check_request(KVM_REQ_STEAL_UPDATE, vcpu))
-			record_steal_time(vcpu);
 		if (kvm_check_request(KVM_REQ_NMI, vcpu))
 			process_nmi(vcpu);
 		req_immediate_exit =
@@ -5672,6 +5669,8 @@  static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 		}
 	}
 
+	record_steal_time(vcpu);
+
 	r = kvm_mmu_reload(vcpu);
 	if (unlikely(r)) {
 		goto cancel_injection;
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 91ae127..5476ffc 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -112,27 +112,26 @@  static inline bool is_error_page(struct page *page)
 /*
  * vcpu->requests bit members
  */
-#define KVM_REQ_TLB_FLUSH          0
-#define KVM_REQ_MIGRATE_TIMER      1
-#define KVM_REQ_REPORT_TPR_ACCESS  2
-#define KVM_REQ_MMU_RELOAD         3
-#define KVM_REQ_TRIPLE_FAULT       4
-#define KVM_REQ_PENDING_TIMER      5
-#define KVM_REQ_UNHALT             6
-#define KVM_REQ_MMU_SYNC           7
-#define KVM_REQ_CLOCK_UPDATE       8
-#define KVM_REQ_KICK               9
-#define KVM_REQ_DEACTIVATE_FPU    10
-#define KVM_REQ_EVENT             11
-#define KVM_REQ_APF_HALT          12
-#define KVM_REQ_STEAL_UPDATE      13
-#define KVM_REQ_NMI               14
-#define KVM_REQ_IMMEDIATE_EXIT    15
-#define KVM_REQ_PMU               16
-#define KVM_REQ_PMI               17
-#define KVM_REQ_WATCHDOG          18
-#define KVM_REQ_MASTERCLOCK_UPDATE 19
-#define KVM_REQ_MCLOCK_INPROGRESS 20
+#define KVM_REQ_TLB_FLUSH		0
+#define KVM_REQ_MIGRATE_TIMER		1
+#define KVM_REQ_REPORT_TPR_ACCESS	2
+#define KVM_REQ_MMU_RELOAD		3
+#define KVM_REQ_TRIPLE_FAULT		4
+#define KVM_REQ_PENDING_TIMER		5
+#define KVM_REQ_UNHALT			6
+#define KVM_REQ_MMU_SYNC		7
+#define KVM_REQ_CLOCK_UPDATE		8
+#define KVM_REQ_KICK			9
+#define KVM_REQ_DEACTIVATE_FPU		10
+#define KVM_REQ_EVENT			11
+#define KVM_REQ_APF_HALT		12
+#define KVM_REQ_NMI			13
+#define KVM_REQ_IMMEDIATE_EXIT		14
+#define KVM_REQ_PMU			15
+#define KVM_REQ_PMI			16
+#define KVM_REQ_WATCHDOG		17
+#define KVM_REQ_MASTERCLOCK_UPDATE	18
+#define KVM_REQ_MCLOCK_INPROGRESS	19
 
 #define KVM_USERSPACE_IRQ_SOURCE_ID		0
 #define KVM_IRQFD_RESAMPLE_IRQ_SOURCE_ID	1