KVM Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2 0/3] KVM: Properly account for guest CPU time
@ 2021-04-13  7:16 Wanpeng Li
  2021-04-13  7:16 ` [PATCH v2 1/3] context_tracking: Split guest_enter/exit_irqoff Wanpeng Li
                   ` (4 more replies)
  0 siblings, 5 replies; 15+ messages in thread
From: Wanpeng Li @ 2021-04-13  7:16 UTC (permalink / raw)
  To: linux-kernel, kvm
  Cc: Paolo Bonzini, Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li,
	Jim Mattson, Joerg Roedel, Thomas Gleixner, Michael Tokarev

The bugzilla https://bugzilla.kernel.org/show_bug.cgi?id=209831
reported that the guest time remains 0 when running a while true
loop in the guest.

The commit 87fa7f3e98a131 ("x86/kvm: Move context tracking where it
belongs") moves guest_exit_irqoff() close to vmexit breaks the
tick-based time accouting when the ticks that happen after IRQs are
disabled are incorrectly accounted to the host/system time. This is
because we exit the guest state too early.

This patchset splits both context tracking logic and the time accounting 
logic from guest_enter/exit_irqoff(), keep context tracking around the 
actual vmentry/exit code, have the virt time specific helpers which 
can be placed at the proper spots in kvm. In addition, it will not 
break the world outside of x86.

v1 -> v2:
 * split context_tracking from guest_enter/exit_irqoff
 * provide separate vtime accounting functions for consistent
 * place the virt time specific helpers at the proper splot 

Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Sean Christopherson <seanjc@google.com>
Cc: Michael Tokarev <mjt@tls.msk.ru>

Wanpeng Li (3):
  context_tracking: Split guest_enter/exit_irqoff
  context_tracking: Provide separate vtime accounting functions
  x86/kvm: Fix vtime accounting

 arch/x86/kvm/svm/svm.c           |  6 ++-
 arch/x86/kvm/vmx/vmx.c           |  6 ++-
 arch/x86/kvm/x86.c               |  1 +
 include/linux/context_tracking.h | 84 +++++++++++++++++++++++++++++++---------
 4 files changed, 74 insertions(+), 23 deletions(-)

-- 
2.7.4


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v2 1/3] context_tracking: Split guest_enter/exit_irqoff
  2021-04-13  7:16 [PATCH v2 0/3] KVM: Properly account for guest CPU time Wanpeng Li
@ 2021-04-13  7:16 ` Wanpeng Li
  2021-04-13  7:35   ` Christian Borntraeger
  2021-04-13  7:16 ` [PATCH v2 2/3] context_tracking: Provide separate vtime accounting functions Wanpeng Li
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 15+ messages in thread
From: Wanpeng Li @ 2021-04-13  7:16 UTC (permalink / raw)
  To: linux-kernel, kvm
  Cc: Paolo Bonzini, Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li,
	Jim Mattson, Joerg Roedel, Thomas Gleixner, Michael Tokarev

From: Wanpeng Li <wanpengli@tencent.com>

Split context_tracking part from guest_enter/exit_irqoff, it will be 
called separately in later patches.

Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Sean Christopherson <seanjc@google.com>
Cc: Michael Tokarev <mjt@tls.msk.ru>
Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
---
 include/linux/context_tracking.h | 42 +++++++++++++++++++++++++++++-----------
 1 file changed, 31 insertions(+), 11 deletions(-)

diff --git a/include/linux/context_tracking.h b/include/linux/context_tracking.h
index bceb064..d8ad844 100644
--- a/include/linux/context_tracking.h
+++ b/include/linux/context_tracking.h
@@ -104,16 +104,8 @@ static inline void context_tracking_init(void) { }
 
 
 #ifdef CONFIG_VIRT_CPU_ACCOUNTING_GEN
-/* must be called with irqs disabled */
-static __always_inline void guest_enter_irqoff(void)
+static __always_inline void context_guest_enter_irqoff(void)
 {
-	instrumentation_begin();
-	if (vtime_accounting_enabled_this_cpu())
-		vtime_guest_enter(current);
-	else
-		current->flags |= PF_VCPU;
-	instrumentation_end();
-
 	if (context_tracking_enabled())
 		__context_tracking_enter(CONTEXT_GUEST);
 
@@ -131,10 +123,28 @@ static __always_inline void guest_enter_irqoff(void)
 	}
 }
 
-static __always_inline void guest_exit_irqoff(void)
+/* must be called with irqs disabled */
+static __always_inline void guest_enter_irqoff(void)
+{
+	instrumentation_begin();
+	if (vtime_accounting_enabled_this_cpu())
+		vtime_guest_enter(current);
+	else
+		current->flags |= PF_VCPU;
+	instrumentation_end();
+
+	context_guest_enter_irqoff();
+}
+
+static __always_inline void context_guest_exit_irqoff(void)
 {
 	if (context_tracking_enabled())
 		__context_tracking_exit(CONTEXT_GUEST);
+}
+
+static __always_inline void guest_exit_irqoff(void)
+{
+	context_guest_exit_irqoff();
 
 	instrumentation_begin();
 	if (vtime_accounting_enabled_this_cpu())
@@ -145,6 +155,13 @@ static __always_inline void guest_exit_irqoff(void)
 }
 
 #else
+static __always_inline void context_guest_enter_irqoff(void)
+{
+	instrumentation_begin();
+	rcu_virt_note_context_switch(smp_processor_id());
+	instrumentation_end();
+}
+
 static __always_inline void guest_enter_irqoff(void)
 {
 	/*
@@ -155,10 +172,13 @@ static __always_inline void guest_enter_irqoff(void)
 	instrumentation_begin();
 	vtime_account_kernel(current);
 	current->flags |= PF_VCPU;
-	rcu_virt_note_context_switch(smp_processor_id());
 	instrumentation_end();
+
+	context_guest_enter_irqoff();
 }
 
+static __always_inline void context_guest_exit_irqoff(void) { }
+
 static __always_inline void guest_exit_irqoff(void)
 {
 	instrumentation_begin();
-- 
2.7.4


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v2 2/3] context_tracking: Provide separate vtime accounting functions
  2021-04-13  7:16 [PATCH v2 0/3] KVM: Properly account for guest CPU time Wanpeng Li
  2021-04-13  7:16 ` [PATCH v2 1/3] context_tracking: Split guest_enter/exit_irqoff Wanpeng Li
@ 2021-04-13  7:16 ` Wanpeng Li
  2021-04-13  7:16 ` [PATCH v2 3/3] x86/kvm: Fix vtime accounting Wanpeng Li
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 15+ messages in thread
From: Wanpeng Li @ 2021-04-13  7:16 UTC (permalink / raw)
  To: linux-kernel, kvm
  Cc: Paolo Bonzini, Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li,
	Jim Mattson, Joerg Roedel, Thomas Gleixner, Michael Tokarev

From: Wanpeng Li <wanpengli@tencent.com>

Provide separate vtime accounting functions, because having proper 
wrappers for that case would be too consistent and less confusing.

Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Sean Christopherson <seanjc@google.com>
Cc: Michael Tokarev <mjt@tls.msk.ru>
Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
---
 include/linux/context_tracking.h | 50 ++++++++++++++++++++++++++++++----------
 1 file changed, 38 insertions(+), 12 deletions(-)

diff --git a/include/linux/context_tracking.h b/include/linux/context_tracking.h
index d8ad844..491f889 100644
--- a/include/linux/context_tracking.h
+++ b/include/linux/context_tracking.h
@@ -102,6 +102,40 @@ extern void context_tracking_init(void);
 static inline void context_tracking_init(void) { }
 #endif /* CONFIG_CONTEXT_TRACKING_FORCE */
 
+static __always_inline void vtime_account_guest_enter(void)
+{
+	if (IS_ENABLED(CONFIG_VIRT_CPU_ACCOUNTING_GEN)) {
+		if (vtime_accounting_enabled_this_cpu())
+			vtime_guest_enter(current);
+		else
+			current->flags |= PF_VCPU;
+	} else {
+		vtime_account_kernel(current);
+		current->flags |= PF_VCPU;
+	}
+}
+
+static __always_inline void __vtime_account_guest_exit(void)
+{
+	if (IS_ENABLED(CONFIG_VIRT_CPU_ACCOUNTING_GEN)) {
+		if (vtime_accounting_enabled_this_cpu())
+			vtime_guest_exit(current);
+	} else {
+		vtime_account_kernel(current);
+	}
+}
+
+static __always_inline void vtime_account_guest_exit(void)
+{
+	__vtime_account_guest_exit();
+	current->flags &= ~PF_VCPU;
+}
+
+static __always_inline void vcpu_account_guest_exit(void)
+{
+	if (!vtime_accounting_enabled_this_cpu())
+		current->flags &= ~PF_VCPU;
+}
 
 #ifdef CONFIG_VIRT_CPU_ACCOUNTING_GEN
 static __always_inline void context_guest_enter_irqoff(void)
@@ -127,10 +161,7 @@ static __always_inline void context_guest_enter_irqoff(void)
 static __always_inline void guest_enter_irqoff(void)
 {
 	instrumentation_begin();
-	if (vtime_accounting_enabled_this_cpu())
-		vtime_guest_enter(current);
-	else
-		current->flags |= PF_VCPU;
+	vtime_account_guest_enter();
 	instrumentation_end();
 
 	context_guest_enter_irqoff();
@@ -147,10 +178,7 @@ static __always_inline void guest_exit_irqoff(void)
 	context_guest_exit_irqoff();
 
 	instrumentation_begin();
-	if (vtime_accounting_enabled_this_cpu())
-		vtime_guest_exit(current);
-	else
-		current->flags &= ~PF_VCPU;
+	vtime_account_guest_exit();
 	instrumentation_end();
 }
 
@@ -170,8 +198,7 @@ static __always_inline void guest_enter_irqoff(void)
 	 * to flush.
 	 */
 	instrumentation_begin();
-	vtime_account_kernel(current);
-	current->flags |= PF_VCPU;
+	vtime_account_guest_enter();
 	instrumentation_end();
 
 	context_guest_enter_irqoff();
@@ -183,8 +210,7 @@ static __always_inline void guest_exit_irqoff(void)
 {
 	instrumentation_begin();
 	/* Flush the guest cputime we spent on the guest */
-	vtime_account_kernel(current);
-	current->flags &= ~PF_VCPU;
+	vtime_account_guest_exit();
 	instrumentation_end();
 }
 #endif /* CONFIG_VIRT_CPU_ACCOUNTING_GEN */
-- 
2.7.4


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v2 3/3] x86/kvm: Fix vtime accounting
  2021-04-13  7:16 [PATCH v2 0/3] KVM: Properly account for guest CPU time Wanpeng Li
  2021-04-13  7:16 ` [PATCH v2 1/3] context_tracking: Split guest_enter/exit_irqoff Wanpeng Li
  2021-04-13  7:16 ` [PATCH v2 2/3] context_tracking: Provide separate vtime accounting functions Wanpeng Li
@ 2021-04-13  7:16 ` Wanpeng Li
  2021-04-13  8:32 ` [PATCH v2 0/3] KVM: Properly account for guest CPU time Christian Borntraeger
  2021-04-13 17:25 ` Sean Christopherson
  4 siblings, 0 replies; 15+ messages in thread
From: Wanpeng Li @ 2021-04-13  7:16 UTC (permalink / raw)
  To: linux-kernel, kvm
  Cc: Paolo Bonzini, Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li,
	Jim Mattson, Joerg Roedel, Thomas Gleixner, Michael Tokarev,
	stable

From: Wanpeng Li <wanpengli@tencent.com>

The bugzilla https://bugzilla.kernel.org/show_bug.cgi?id=209831
reported that the guest time remains 0 when running a while true
loop in the guest.

The commit 87fa7f3e98a131 ("x86/kvm: Move context tracking where it
belongs") moves guest_exit_irqoff() close to vmexit breaks the
tick-based time accouting when the ticks that happen after IRQs are
disabled are incorrectly accounted to the host/system time. This is
because we exit the guest state too early.

Keep context tracking around the actual vmentry/exit code, the time 
accounting logic is separated out by preposed patch from 
guest_enter/exit_irqoff(). Keep vtime-based time accounting around 
the actual vmentry/exit code when vtime_accounting_enabled_this_cpu() 
is true, leave PF_VCPU clearing after handle_exit_irqoff() and explicit 
IRQ window for tick-based time accouting.

Fixes: 87fa7f3e98a131 ("x86/kvm: Move context tracking where it belongs")
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Sean Christopherson <seanjc@google.com>
Cc: Michael Tokarev <mjt@tls.msk.ru>
Cc: stable@vger.kernel.org#v5.9-rc1+
Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
---
 arch/x86/kvm/svm/svm.c | 6 ++++--
 arch/x86/kvm/vmx/vmx.c | 6 ++++--
 arch/x86/kvm/x86.c     | 1 +
 3 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 48b396f3..2a4e284 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -3726,11 +3726,12 @@ static noinstr void svm_vcpu_enter_exit(struct kvm_vcpu *vcpu)
 	 * accordingly.
 	 */
 	instrumentation_begin();
+	vtime_account_guest_enter();
 	trace_hardirqs_on_prepare();
 	lockdep_hardirqs_on_prepare(CALLER_ADDR0);
 	instrumentation_end();
 
-	guest_enter_irqoff();
+	context_guest_enter_irqoff();
 	lockdep_hardirqs_on(CALLER_ADDR0);
 
 	if (sev_es_guest(vcpu->kvm)) {
@@ -3758,10 +3759,11 @@ static noinstr void svm_vcpu_enter_exit(struct kvm_vcpu *vcpu)
 	 * into world and some more.
 	 */
 	lockdep_hardirqs_off(CALLER_ADDR0);
-	guest_exit_irqoff();
+	context_guest_exit_irqoff();
 
 	instrumentation_begin();
 	trace_hardirqs_off_finish();
+	__vtime_account_guest_exit();
 	instrumentation_end();
 }
 
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index c05e6e2..23be956 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -6613,11 +6613,12 @@ static noinstr void vmx_vcpu_enter_exit(struct kvm_vcpu *vcpu,
 	 * accordingly.
 	 */
 	instrumentation_begin();
+	vtime_account_guest_enter();
 	trace_hardirqs_on_prepare();
 	lockdep_hardirqs_on_prepare(CALLER_ADDR0);
 	instrumentation_end();
 
-	guest_enter_irqoff();
+	context_guest_enter_irqoff();
 	lockdep_hardirqs_on(CALLER_ADDR0);
 
 	/* L1D Flush includes CPU buffer clear to mitigate MDS */
@@ -6647,10 +6648,11 @@ static noinstr void vmx_vcpu_enter_exit(struct kvm_vcpu *vcpu,
 	 * into world and some more.
 	 */
 	lockdep_hardirqs_off(CALLER_ADDR0);
-	guest_exit_irqoff();
+	context_guest_exit_irqoff();
 
 	instrumentation_begin();
 	trace_hardirqs_off_finish();
+	__vtime_account_guest_exit();
 	instrumentation_end();
 }
 
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index ce9a1d2..0d2dd3f 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -9245,6 +9245,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 	++vcpu->stat.exits;
 	local_irq_disable();
 	kvm_after_interrupt(vcpu);
+	vcpu_account_guest_exit();
 
 	if (lapic_in_kernel(vcpu)) {
 		s64 delta = vcpu->arch.apic->lapic_timer.advance_expire_delta;
-- 
2.7.4


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 1/3] context_tracking: Split guest_enter/exit_irqoff
  2021-04-13  7:16 ` [PATCH v2 1/3] context_tracking: Split guest_enter/exit_irqoff Wanpeng Li
@ 2021-04-13  7:35   ` Christian Borntraeger
  2021-04-13  7:38     ` Wanpeng Li
  0 siblings, 1 reply; 15+ messages in thread
From: Christian Borntraeger @ 2021-04-13  7:35 UTC (permalink / raw)
  To: Wanpeng Li, linux-kernel, kvm
  Cc: Paolo Bonzini, Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li,
	Jim Mattson, Joerg Roedel, Thomas Gleixner, Michael Tokarev



On 13.04.21 09:16, Wanpeng Li wrote:
[...]

> @@ -145,6 +155,13 @@ static __always_inline void guest_exit_irqoff(void)
>   }
> 
>   #else
> +static __always_inline void context_guest_enter_irqoff(void)
> +{
> +	instrumentation_begin();
> +	rcu_virt_note_context_switch(smp_processor_id());
> +	instrumentation_end();
> +}
> +
>   static __always_inline void guest_enter_irqoff(void)
>   {
>   	/*
> @@ -155,10 +172,13 @@ static __always_inline void guest_enter_irqoff(void)
>   	instrumentation_begin();
>   	vtime_account_kernel(current);
>   	current->flags |= PF_VCPU;
> -	rcu_virt_note_context_switch(smp_processor_id());
>   	instrumentation_end();
> +
> +	context_guest_enter_irqoff();

So we now do instrumentation_begin 2 times?


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 1/3] context_tracking: Split guest_enter/exit_irqoff
  2021-04-13  7:35   ` Christian Borntraeger
@ 2021-04-13  7:38     ` Wanpeng Li
  2021-04-13  7:48       ` Christian Borntraeger
  0 siblings, 1 reply; 15+ messages in thread
From: Wanpeng Li @ 2021-04-13  7:38 UTC (permalink / raw)
  To: Christian Borntraeger
  Cc: LKML, kvm, Paolo Bonzini, Sean Christopherson, Vitaly Kuznetsov,
	Wanpeng Li, Jim Mattson, Joerg Roedel, Thomas Gleixner,
	Michael Tokarev

On Tue, 13 Apr 2021 at 15:35, Christian Borntraeger
<borntraeger@de.ibm.com> wrote:
>
>
>
> On 13.04.21 09:16, Wanpeng Li wrote:
> [...]
>
> > @@ -145,6 +155,13 @@ static __always_inline void guest_exit_irqoff(void)
> >   }
> >
> >   #else
> > +static __always_inline void context_guest_enter_irqoff(void)
> > +{
> > +     instrumentation_begin();
> > +     rcu_virt_note_context_switch(smp_processor_id());
> > +     instrumentation_end();
> > +}
> > +
> >   static __always_inline void guest_enter_irqoff(void)
> >   {
> >       /*
> > @@ -155,10 +172,13 @@ static __always_inline void guest_enter_irqoff(void)
> >       instrumentation_begin();
> >       vtime_account_kernel(current);
> >       current->flags |= PF_VCPU;
> > -     rcu_virt_note_context_switch(smp_processor_id());
> >       instrumentation_end();
> > +
> > +     context_guest_enter_irqoff();
>
> So we now do instrumentation_begin 2 times?

Similar to context_guest_enter_irqoff() ifdef CONFIG_VIRT_CPU_ACCOUNTING_GEN.

    Wanpeng

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 1/3] context_tracking: Split guest_enter/exit_irqoff
  2021-04-13  7:38     ` Wanpeng Li
@ 2021-04-13  7:48       ` Christian Borntraeger
  2021-04-13  7:52         ` Wanpeng Li
  0 siblings, 1 reply; 15+ messages in thread
From: Christian Borntraeger @ 2021-04-13  7:48 UTC (permalink / raw)
  To: Wanpeng Li
  Cc: LKML, kvm, Paolo Bonzini, Sean Christopherson, Vitaly Kuznetsov,
	Wanpeng Li, Jim Mattson, Joerg Roedel, Thomas Gleixner,
	Michael Tokarev



On 13.04.21 09:38, Wanpeng Li wrote:
> On Tue, 13 Apr 2021 at 15:35, Christian Borntraeger
> <borntraeger@de.ibm.com> wrote:
>>
>>
>>
>> On 13.04.21 09:16, Wanpeng Li wrote:
>> [...]
>>
>>> @@ -145,6 +155,13 @@ static __always_inline void guest_exit_irqoff(void)
>>>    }
>>>
>>>    #else
THis is the else part


>>> +static __always_inline void context_guest_enter_irqoff(void)
>>> +{
>>> +     instrumentation_begin();

2nd on
>>> +     rcu_virt_note_context_switch(smp_processor_id());
>>> +     instrumentation_end();
2nd off
>>> +}
>>> +
>>>    static __always_inline void guest_enter_irqoff(void)
>>>    {
>>>        /*
>>> @@ -155,10 +172,13 @@ static __always_inline void guest_enter_irqoff(void)
>>>        instrumentation_begin();

first on
>>>        vtime_account_kernel(current);
>>>        current->flags |= PF_VCPU;
>>> -     rcu_virt_note_context_switch(smp_processor_id());
>>>        instrumentation_end();

first off
>>> +
>>> +     context_guest_enter_irqoff();
here we call the 2nd on and off.
>>
>> So we now do instrumentation_begin 2 times?
> 
> Similar to context_guest_enter_irqoff() ifdef CONFIG_VIRT_CPU_ACCOUNTING_GEN.

For the
ifdef CONFIG_VIRT_CPU_ACCOUNTING_GEN part
context_guest_enter_irqoff()
does not have instrumentation_begin/end.

Or did I miss anything.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 1/3] context_tracking: Split guest_enter/exit_irqoff
  2021-04-13  7:48       ` Christian Borntraeger
@ 2021-04-13  7:52         ` Wanpeng Li
  2021-04-13  8:07           ` Christian Borntraeger
  0 siblings, 1 reply; 15+ messages in thread
From: Wanpeng Li @ 2021-04-13  7:52 UTC (permalink / raw)
  To: Christian Borntraeger
  Cc: LKML, kvm, Paolo Bonzini, Sean Christopherson, Vitaly Kuznetsov,
	Wanpeng Li, Jim Mattson, Joerg Roedel, Thomas Gleixner,
	Michael Tokarev

On Tue, 13 Apr 2021 at 15:48, Christian Borntraeger
<borntraeger@de.ibm.com> wrote:
>
>
>
> On 13.04.21 09:38, Wanpeng Li wrote:
> > On Tue, 13 Apr 2021 at 15:35, Christian Borntraeger
> > <borntraeger@de.ibm.com> wrote:
> >>
> >>
> >>
> >> On 13.04.21 09:16, Wanpeng Li wrote:
> >> [...]
> >>
> >>> @@ -145,6 +155,13 @@ static __always_inline void guest_exit_irqoff(void)
> >>>    }
> >>>
> >>>    #else
> THis is the else part
>
>
> >>> +static __always_inline void context_guest_enter_irqoff(void)
> >>> +{
> >>> +     instrumentation_begin();
>
> 2nd on
> >>> +     rcu_virt_note_context_switch(smp_processor_id());
> >>> +     instrumentation_end();
> 2nd off
> >>> +}
> >>> +
> >>>    static __always_inline void guest_enter_irqoff(void)
> >>>    {
> >>>        /*
> >>> @@ -155,10 +172,13 @@ static __always_inline void guest_enter_irqoff(void)
> >>>        instrumentation_begin();
>
> first on
> >>>        vtime_account_kernel(current);
> >>>        current->flags |= PF_VCPU;
> >>> -     rcu_virt_note_context_switch(smp_processor_id());
> >>>        instrumentation_end();
>
> first off
> >>> +
> >>> +     context_guest_enter_irqoff();
> here we call the 2nd on and off.
> >>
> >> So we now do instrumentation_begin 2 times?
> >
> > Similar to context_guest_enter_irqoff() ifdef CONFIG_VIRT_CPU_ACCOUNTING_GEN.
>
> For the
> ifdef CONFIG_VIRT_CPU_ACCOUNTING_GEN part
> context_guest_enter_irqoff()
> does not have instrumentation_begin/end.
>
> Or did I miss anything.

I mean the if (!context_tracking_enabled_this_cpu()) part in the
function context_guest_enter_irqoff() ifdef
CONFIG_VIRT_CPU_ACCOUNTING_GEN. :)

    Wanpeng

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 1/3] context_tracking: Split guest_enter/exit_irqoff
  2021-04-13  7:52         ` Wanpeng Li
@ 2021-04-13  8:07           ` Christian Borntraeger
  0 siblings, 0 replies; 15+ messages in thread
From: Christian Borntraeger @ 2021-04-13  8:07 UTC (permalink / raw)
  To: Wanpeng Li
  Cc: LKML, kvm, Paolo Bonzini, Sean Christopherson, Vitaly Kuznetsov,
	Wanpeng Li, Jim Mattson, Joerg Roedel, Thomas Gleixner,
	Michael Tokarev



On 13.04.21 09:52, Wanpeng Li wrote:
>> Or did I miss anything.
> 
> I mean the if (!context_tracking_enabled_this_cpu()) part in the
> function context_guest_enter_irqoff() ifdef
> CONFIG_VIRT_CPU_ACCOUNTING_GEN. :)

Ah I missed that. Thanks.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 0/3] KVM: Properly account for guest CPU time
  2021-04-13  7:16 [PATCH v2 0/3] KVM: Properly account for guest CPU time Wanpeng Li
                   ` (2 preceding siblings ...)
  2021-04-13  7:16 ` [PATCH v2 3/3] x86/kvm: Fix vtime accounting Wanpeng Li
@ 2021-04-13  8:32 ` Christian Borntraeger
  2021-04-13 17:25 ` Sean Christopherson
  4 siblings, 0 replies; 15+ messages in thread
From: Christian Borntraeger @ 2021-04-13  8:32 UTC (permalink / raw)
  To: Wanpeng Li, linux-kernel, kvm
  Cc: Paolo Bonzini, Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li,
	Jim Mattson, Joerg Roedel, Thomas Gleixner, Michael Tokarev



On 13.04.21 09:16, Wanpeng Li wrote:
> The bugzilla https://bugzilla.kernel.org/show_bug.cgi?id=209831
> reported that the guest time remains 0 when running a while true
> loop in the guest.
> 
> The commit 87fa7f3e98a131 ("x86/kvm: Move context tracking where it
> belongs") moves guest_exit_irqoff() close to vmexit breaks the
> tick-based time accouting when the ticks that happen after IRQs are
> disabled are incorrectly accounted to the host/system time. This is
> because we exit the guest state too early.
> 
> This patchset splits both context tracking logic and the time accounting
> logic from guest_enter/exit_irqoff(), keep context tracking around the
> actual vmentry/exit code, have the virt time specific helpers which
> can be placed at the proper spots in kvm. In addition, it will not
> break the world outside of x86.
> 
> v1 -> v2:
>   * split context_tracking from guest_enter/exit_irqoff
>   * provide separate vtime accounting functions for consistent
>   * place the virt time specific helpers at the proper splot
> 
> Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Sean Christopherson <seanjc@google.com>
> Cc: Michael Tokarev <mjt@tls.msk.ru>
> 
> Wanpeng Li (3):
>    context_tracking: Split guest_enter/exit_irqoff
>    context_tracking: Provide separate vtime accounting functions
>    x86/kvm: Fix vtime accounting
> 
>   arch/x86/kvm/svm/svm.c           |  6 ++-
>   arch/x86/kvm/vmx/vmx.c           |  6 ++-
>   arch/x86/kvm/x86.c               |  1 +
>   include/linux/context_tracking.h | 84 +++++++++++++++++++++++++++++++---------
>   4 files changed, 74 insertions(+), 23 deletions(-)
> 

The non CONFIG_VIRT_CPU_ACCOUNTING_GEN look good.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 0/3] KVM: Properly account for guest CPU time
  2021-04-13  7:16 [PATCH v2 0/3] KVM: Properly account for guest CPU time Wanpeng Li
                   ` (3 preceding siblings ...)
  2021-04-13  8:32 ` [PATCH v2 0/3] KVM: Properly account for guest CPU time Christian Borntraeger
@ 2021-04-13 17:25 ` Sean Christopherson
  2021-04-14  9:36   ` Wanpeng Li
  4 siblings, 1 reply; 15+ messages in thread
From: Sean Christopherson @ 2021-04-13 17:25 UTC (permalink / raw)
  To: Wanpeng Li
  Cc: linux-kernel, kvm, Paolo Bonzini, Vitaly Kuznetsov, Wanpeng Li,
	Jim Mattson, Joerg Roedel, Thomas Gleixner, Michael Tokarev

On Tue, Apr 13, 2021, Wanpeng Li wrote:
> The bugzilla https://bugzilla.kernel.org/show_bug.cgi?id=209831
> reported that the guest time remains 0 when running a while true
> loop in the guest.
> 
> The commit 87fa7f3e98a131 ("x86/kvm: Move context tracking where it
> belongs") moves guest_exit_irqoff() close to vmexit breaks the
> tick-based time accouting when the ticks that happen after IRQs are
> disabled are incorrectly accounted to the host/system time. This is
> because we exit the guest state too early.
> 
> This patchset splits both context tracking logic and the time accounting 
> logic from guest_enter/exit_irqoff(), keep context tracking around the 
> actual vmentry/exit code, have the virt time specific helpers which 
> can be placed at the proper spots in kvm. In addition, it will not 
> break the world outside of x86.

IMO, this is going in the wrong direction.  Rather than separate context tracking,
vtime accounting, and KVM logic, this further intertwines the three.  E.g. the
context tracking code has even more vtime accounting NATIVE vs. GEN vs. TICK
logic baked into it.

Rather than smush everything into context_tracking.h, I think we can cleanly
split the context tracking and vtime accounting code into separate pieces, which
will in turn allow moving the wrapping logic to linux/kvm_host.h.  Once that is
done, splitting the context tracking and time accounting logic for KVM x86
becomes a KVM detail as opposed to requiring dedicated logic in the context
tracking code.

I have untested code that compiles on x86, I'll send an RFC shortly.

> v1 -> v2:
>  * split context_tracking from guest_enter/exit_irqoff
>  * provide separate vtime accounting functions for consistent
>  * place the virt time specific helpers at the proper splot 
> 
> Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Sean Christopherson <seanjc@google.com>
> Cc: Michael Tokarev <mjt@tls.msk.ru>
> 
> Wanpeng Li (3):
>   context_tracking: Split guest_enter/exit_irqoff
>   context_tracking: Provide separate vtime accounting functions
>   x86/kvm: Fix vtime accounting
> 
>  arch/x86/kvm/svm/svm.c           |  6 ++-
>  arch/x86/kvm/vmx/vmx.c           |  6 ++-
>  arch/x86/kvm/x86.c               |  1 +
>  include/linux/context_tracking.h | 84 +++++++++++++++++++++++++++++++---------
>  4 files changed, 74 insertions(+), 23 deletions(-)
> 
> -- 
> 2.7.4
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 0/3] KVM: Properly account for guest CPU time
  2021-04-13 17:25 ` Sean Christopherson
@ 2021-04-14  9:36   ` Wanpeng Li
  2021-04-15  0:49     ` Sean Christopherson
  0 siblings, 1 reply; 15+ messages in thread
From: Wanpeng Li @ 2021-04-14  9:36 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: LKML, kvm, Paolo Bonzini, Vitaly Kuznetsov, Wanpeng Li,
	Jim Mattson, Joerg Roedel, Thomas Gleixner, Michael Tokarev

On Wed, 14 Apr 2021 at 01:25, Sean Christopherson <seanjc@google.com> wrote:
>
> On Tue, Apr 13, 2021, Wanpeng Li wrote:
> > The bugzilla https://bugzilla.kernel.org/show_bug.cgi?id=209831
> > reported that the guest time remains 0 when running a while true
> > loop in the guest.
> >
> > The commit 87fa7f3e98a131 ("x86/kvm: Move context tracking where it
> > belongs") moves guest_exit_irqoff() close to vmexit breaks the
> > tick-based time accouting when the ticks that happen after IRQs are
> > disabled are incorrectly accounted to the host/system time. This is
> > because we exit the guest state too early.
> >
> > This patchset splits both context tracking logic and the time accounting
> > logic from guest_enter/exit_irqoff(), keep context tracking around the
> > actual vmentry/exit code, have the virt time specific helpers which
> > can be placed at the proper spots in kvm. In addition, it will not
> > break the world outside of x86.
>
> IMO, this is going in the wrong direction.  Rather than separate context tracking,
> vtime accounting, and KVM logic, this further intertwines the three.  E.g. the
> context tracking code has even more vtime accounting NATIVE vs. GEN vs. TICK
> logic baked into it.
>
> Rather than smush everything into context_tracking.h, I think we can cleanly
> split the context tracking and vtime accounting code into separate pieces, which
> will in turn allow moving the wrapping logic to linux/kvm_host.h.  Once that is
> done, splitting the context tracking and time accounting logic for KVM x86
> becomes a KVM detail as opposed to requiring dedicated logic in the context
> tracking code.
>
> I have untested code that compiles on x86, I'll send an RFC shortly.

We need an easy to backport fix and then we might have some further
cleanups on top.

    Wanpeng

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 0/3] KVM: Properly account for guest CPU time
  2021-04-14  9:36   ` Wanpeng Li
@ 2021-04-15  0:49     ` Sean Christopherson
  2021-04-15  1:23       ` Wanpeng Li
  0 siblings, 1 reply; 15+ messages in thread
From: Sean Christopherson @ 2021-04-15  0:49 UTC (permalink / raw)
  To: Wanpeng Li
  Cc: LKML, kvm, Paolo Bonzini, Vitaly Kuznetsov, Wanpeng Li,
	Jim Mattson, Joerg Roedel, Thomas Gleixner, Michael Tokarev

On Wed, Apr 14, 2021, Wanpeng Li wrote:
> On Wed, 14 Apr 2021 at 01:25, Sean Christopherson <seanjc@google.com> wrote:
> >
> > On Tue, Apr 13, 2021, Wanpeng Li wrote:
> > > The bugzilla https://bugzilla.kernel.org/show_bug.cgi?id=209831
> > > reported that the guest time remains 0 when running a while true
> > > loop in the guest.
> > >
> > > The commit 87fa7f3e98a131 ("x86/kvm: Move context tracking where it
> > > belongs") moves guest_exit_irqoff() close to vmexit breaks the
> > > tick-based time accouting when the ticks that happen after IRQs are
> > > disabled are incorrectly accounted to the host/system time. This is
> > > because we exit the guest state too early.
> > >
> > > This patchset splits both context tracking logic and the time accounting
> > > logic from guest_enter/exit_irqoff(), keep context tracking around the
> > > actual vmentry/exit code, have the virt time specific helpers which
> > > can be placed at the proper spots in kvm. In addition, it will not
> > > break the world outside of x86.
> >
> > IMO, this is going in the wrong direction.  Rather than separate context tracking,
> > vtime accounting, and KVM logic, this further intertwines the three.  E.g. the
> > context tracking code has even more vtime accounting NATIVE vs. GEN vs. TICK
> > logic baked into it.
> >
> > Rather than smush everything into context_tracking.h, I think we can cleanly
> > split the context tracking and vtime accounting code into separate pieces, which
> > will in turn allow moving the wrapping logic to linux/kvm_host.h.  Once that is
> > done, splitting the context tracking and time accounting logic for KVM x86
> > becomes a KVM detail as opposed to requiring dedicated logic in the context
> > tracking code.
> >
> > I have untested code that compiles on x86, I'll send an RFC shortly.
> 
> We need an easy to backport fix and then we might have some further
> cleanups on top.

I fiddled with this a bit today, I think I have something workable that will be
a relatively clean and short backport.  With luck, I'll get it posted tomorrow.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 0/3] KVM: Properly account for guest CPU time
  2021-04-15  0:49     ` Sean Christopherson
@ 2021-04-15  1:23       ` Wanpeng Li
  2021-04-15 19:02         ` Sean Christopherson
  0 siblings, 1 reply; 15+ messages in thread
From: Wanpeng Li @ 2021-04-15  1:23 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: LKML, kvm, Paolo Bonzini, Vitaly Kuznetsov, Wanpeng Li,
	Jim Mattson, Joerg Roedel, Thomas Gleixner, Michael Tokarev

On Thu, 15 Apr 2021 at 08:49, Sean Christopherson <seanjc@google.com> wrote:
>
> On Wed, Apr 14, 2021, Wanpeng Li wrote:
> > On Wed, 14 Apr 2021 at 01:25, Sean Christopherson <seanjc@google.com> wrote:
> > >
> > > On Tue, Apr 13, 2021, Wanpeng Li wrote:
> > > > The bugzilla https://bugzilla.kernel.org/show_bug.cgi?id=209831
> > > > reported that the guest time remains 0 when running a while true
> > > > loop in the guest.
> > > >
> > > > The commit 87fa7f3e98a131 ("x86/kvm: Move context tracking where it
> > > > belongs") moves guest_exit_irqoff() close to vmexit breaks the
> > > > tick-based time accouting when the ticks that happen after IRQs are
> > > > disabled are incorrectly accounted to the host/system time. This is
> > > > because we exit the guest state too early.
> > > >
> > > > This patchset splits both context tracking logic and the time accounting
> > > > logic from guest_enter/exit_irqoff(), keep context tracking around the
> > > > actual vmentry/exit code, have the virt time specific helpers which
> > > > can be placed at the proper spots in kvm. In addition, it will not
> > > > break the world outside of x86.
> > >
> > > IMO, this is going in the wrong direction.  Rather than separate context tracking,
> > > vtime accounting, and KVM logic, this further intertwines the three.  E.g. the
> > > context tracking code has even more vtime accounting NATIVE vs. GEN vs. TICK
> > > logic baked into it.
> > >
> > > Rather than smush everything into context_tracking.h, I think we can cleanly
> > > split the context tracking and vtime accounting code into separate pieces, which
> > > will in turn allow moving the wrapping logic to linux/kvm_host.h.  Once that is
> > > done, splitting the context tracking and time accounting logic for KVM x86
> > > becomes a KVM detail as opposed to requiring dedicated logic in the context
> > > tracking code.
> > >
> > > I have untested code that compiles on x86, I'll send an RFC shortly.
> >
> > We need an easy to backport fix and then we might have some further
> > cleanups on top.
>
> I fiddled with this a bit today, I think I have something workable that will be
> a relatively clean and short backport.  With luck, I'll get it posted tomorrow.

I think we should improve my posted version instead of posting a lot
of alternative versions to save everybody's time.

    Wanpeng

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 0/3] KVM: Properly account for guest CPU time
  2021-04-15  1:23       ` Wanpeng Li
@ 2021-04-15 19:02         ` Sean Christopherson
  0 siblings, 0 replies; 15+ messages in thread
From: Sean Christopherson @ 2021-04-15 19:02 UTC (permalink / raw)
  To: Wanpeng Li
  Cc: LKML, kvm, Paolo Bonzini, Vitaly Kuznetsov, Wanpeng Li,
	Jim Mattson, Joerg Roedel, Thomas Gleixner, Michael Tokarev

On Thu, Apr 15, 2021, Wanpeng Li wrote:
> On Thu, 15 Apr 2021 at 08:49, Sean Christopherson <seanjc@google.com> wrote:
> >
> > On Wed, Apr 14, 2021, Wanpeng Li wrote:
> > > On Wed, 14 Apr 2021 at 01:25, Sean Christopherson <seanjc@google.com> wrote:
> > > >
> > > > On Tue, Apr 13, 2021, Wanpeng Li wrote:
> > > > > The bugzilla https://bugzilla.kernel.org/show_bug.cgi?id=209831
> > > > > reported that the guest time remains 0 when running a while true
> > > > > loop in the guest.
> > > > >
> > > > > The commit 87fa7f3e98a131 ("x86/kvm: Move context tracking where it
> > > > > belongs") moves guest_exit_irqoff() close to vmexit breaks the
> > > > > tick-based time accouting when the ticks that happen after IRQs are
> > > > > disabled are incorrectly accounted to the host/system time. This is
> > > > > because we exit the guest state too early.
> > > > >
> > > > > This patchset splits both context tracking logic and the time accounting
> > > > > logic from guest_enter/exit_irqoff(), keep context tracking around the
> > > > > actual vmentry/exit code, have the virt time specific helpers which
> > > > > can be placed at the proper spots in kvm. In addition, it will not
> > > > > break the world outside of x86.
> > > >
> > > > IMO, this is going in the wrong direction.  Rather than separate context tracking,
> > > > vtime accounting, and KVM logic, this further intertwines the three.  E.g. the
> > > > context tracking code has even more vtime accounting NATIVE vs. GEN vs. TICK
> > > > logic baked into it.
> > > >
> > > > Rather than smush everything into context_tracking.h, I think we can cleanly
> > > > split the context tracking and vtime accounting code into separate pieces, which
> > > > will in turn allow moving the wrapping logic to linux/kvm_host.h.  Once that is
> > > > done, splitting the context tracking and time accounting logic for KVM x86
> > > > becomes a KVM detail as opposed to requiring dedicated logic in the context
> > > > tracking code.
> > > >
> > > > I have untested code that compiles on x86, I'll send an RFC shortly.
> > >
> > > We need an easy to backport fix and then we might have some further
> > > cleanups on top.
> >
> > I fiddled with this a bit today, I think I have something workable that will be
> > a relatively clean and short backport.  With luck, I'll get it posted tomorrow.
> 
> I think we should improve my posted version instead of posting a lot
> of alternative versions to save everybody's time.

Ya, definitely not looking to throw out more variants.  I'm trying to stack my
cleanups on your code, while also stripping down your patches to the bare minimum
to minimize both the backports and the churn across the cleanups.  It looks like
it's going to work?  Fingers crossed :-)

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, back to index

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-13  7:16 [PATCH v2 0/3] KVM: Properly account for guest CPU time Wanpeng Li
2021-04-13  7:16 ` [PATCH v2 1/3] context_tracking: Split guest_enter/exit_irqoff Wanpeng Li
2021-04-13  7:35   ` Christian Borntraeger
2021-04-13  7:38     ` Wanpeng Li
2021-04-13  7:48       ` Christian Borntraeger
2021-04-13  7:52         ` Wanpeng Li
2021-04-13  8:07           ` Christian Borntraeger
2021-04-13  7:16 ` [PATCH v2 2/3] context_tracking: Provide separate vtime accounting functions Wanpeng Li
2021-04-13  7:16 ` [PATCH v2 3/3] x86/kvm: Fix vtime accounting Wanpeng Li
2021-04-13  8:32 ` [PATCH v2 0/3] KVM: Properly account for guest CPU time Christian Borntraeger
2021-04-13 17:25 ` Sean Christopherson
2021-04-14  9:36   ` Wanpeng Li
2021-04-15  0:49     ` Sean Christopherson
2021-04-15  1:23       ` Wanpeng Li
2021-04-15 19:02         ` Sean Christopherson

KVM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/kvm/0 kvm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 kvm kvm/ https://lore.kernel.org/kvm \
		kvm@vger.kernel.org
	public-inbox-index kvm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.kvm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git