[PATCH v2 1/2] KVM: LAPIC: Optimize timer latency consider world switch time

[PATCH v2 1/2] KVM: LAPIC: Optimize timer latency consider world switch time

[Wanpeng Li]

From: Wanpeng Li <wanpengli@tencent.com>

Advance lapic timer tries to hidden the hypervisor overhead between the
host emulated timer fires and the guest awares the timer is fired. However,
even though after more sustaining optimizations, kvm-unit-tests/tscdeadline_latency 
still awares ~1000 cycles latency since we lost the time between the end of 
wait_lapic_expire and the guest awares the timer is fired. There are 
codes between the end of wait_lapic_expire and the world switch, furthermore, 
the world switch itself also has overhead. Actually the guest_tsc is equal 
to the target deadline time in wait_lapic_expire is too late, guest will
aware the latency between the end of wait_lapic_expire() and after vmentry 
to the guest. This patch takes this time into consideration. 

The vmentry_lapic_timer_advance_ns module parameter should be well tuned by 
host admin, setting bit 0 to 1 to finally cache parameter in KVM. This patch 
can reduce average cyclictest latency from 3us to 2us on Skylake server. 
(guest w/ nohz=off, idle=poll, host w/ preemption_timer=N, the cyclictest 
latency is not too sensitive when preemption_timer=Y for this optimization in 
my testing), kvm-unit-tests/tscdeadline_latency can reach 0.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Radim Krčmář <rkrcmar@redhat.com>
Cc: Sean Christopherson <sean.j.christopherson@intel.com>
Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
---
NOTE: rebase on https://lkml.org/lkml/2019/5/20/449
v1 -> v2:
 * rename get_vmentry_advance_delta to get_vmentry_advance_cycles
 * cache vmentry_advance_cycles by setting param bit 0 
 * add param max limit 

 arch/x86/kvm/lapic.c   | 38 +++++++++++++++++++++++++++++++++++---
 arch/x86/kvm/lapic.h   |  3 +++
 arch/x86/kvm/vmx/vmx.c |  2 +-
 arch/x86/kvm/x86.c     |  9 +++++++++
 arch/x86/kvm/x86.h     |  2 ++
 5 files changed, 50 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index fcf42a3..60587b5 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -1531,6 +1531,38 @@ static inline void adjust_lapic_timer_advance(struct kvm_vcpu *vcpu,
 	apic->lapic_timer.timer_advance_ns = timer_advance_ns;
 }
 
+#define MAX_VMENTRY_ADVANCE_NS 1000
+
+u64 compute_vmentry_advance_cycles(struct kvm_vcpu *vcpu)
+{
+	u64 cycles;
+	struct kvm_lapic *apic = vcpu->arch.apic;
+	u64 val = min_t(u32, vmentry_lapic_timer_advance_ns, MAX_VMENTRY_ADVANCE_NS);
+
+	cycles = (val & ~1ULL) * vcpu->arch.virtual_tsc_khz;
+	do_div(cycles, 1000000);
+
+	/* setting bit 0 locks the value, it is cached */
+	if (val & 1)
+		apic->lapic_timer.vmentry_advance_cycles = cycles;
+
+	return cycles;
+}
+
+inline u64 get_vmentry_advance_cycles(struct kvm_vcpu *vcpu)
+{
+	struct kvm_lapic *apic = vcpu->arch.apic;
+
+	if (!vmentry_lapic_timer_advance_ns)
+		return 0;
+
+	if (likely(apic->lapic_timer.vmentry_advance_cycles))
+		return apic->lapic_timer.vmentry_advance_cycles;
+
+	return compute_vmentry_advance_cycles(vcpu);
+}
+EXPORT_SYMBOL_GPL(get_vmentry_advance_cycles);
+
 void kvm_wait_lapic_expire(struct kvm_vcpu *vcpu)
 {
 	struct kvm_lapic *apic = vcpu->arch.apic;
@@ -1544,7 +1576,7 @@ void kvm_wait_lapic_expire(struct kvm_vcpu *vcpu)
 
 	tsc_deadline = apic->lapic_timer.expired_tscdeadline;
 	apic->lapic_timer.expired_tscdeadline = 0;
-	guest_tsc = kvm_read_l1_tsc(vcpu, rdtsc());
+	guest_tsc = kvm_read_l1_tsc(vcpu, rdtsc()) + get_vmentry_advance_cycles(vcpu);
 	apic->lapic_timer.advance_expire_delta = guest_tsc - tsc_deadline;
 
 	if (guest_tsc < tsc_deadline)
@@ -1572,7 +1604,7 @@ static void start_sw_tscdeadline(struct kvm_lapic *apic)
 	local_irq_save(flags);
 
 	now = ktime_get();
-	guest_tsc = kvm_read_l1_tsc(vcpu, rdtsc());
+	guest_tsc = kvm_read_l1_tsc(vcpu, rdtsc()) + get_vmentry_advance_cycles(vcpu);
 
 	ns = (tscdeadline - guest_tsc) * 1000000ULL;
 	do_div(ns, this_tsc_khz);
@@ -2329,7 +2361,7 @@ int kvm_create_lapic(struct kvm_vcpu *vcpu, int timer_advance_ns)
 		apic->lapic_timer.timer_advance_ns = timer_advance_ns;
 		apic->lapic_timer.timer_advance_adjust_done = true;
 	}
-
+	apic->lapic_timer.vmentry_advance_cycles = 0;
 
 	/*
 	 * APIC is created enabled. This will prevent kvm_lapic_set_base from
diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
index f974a3d..70854a9 100644
--- a/arch/x86/kvm/lapic.h
+++ b/arch/x86/kvm/lapic.h
@@ -33,6 +33,7 @@ struct kvm_timer {
 	u64 expired_tscdeadline;
 	u32 timer_advance_ns;
 	s64 advance_expire_delta;
+	u64 vmentry_advance_cycles;
 	atomic_t pending;			/* accumulated triggered timers */
 	bool hv_timer_in_use;
 	bool timer_advance_adjust_done;
@@ -221,6 +222,8 @@ static inline int kvm_lapic_latched_init(struct kvm_vcpu *vcpu)
 bool kvm_apic_pending_eoi(struct kvm_vcpu *vcpu, int vector);
 
 void kvm_wait_lapic_expire(struct kvm_vcpu *vcpu);
+u64 compute_vmentry_advance_cycles(struct kvm_vcpu *vcpu);
+inline u64 get_vmentry_advance_cycles(struct kvm_vcpu *vcpu);
 
 bool kvm_intr_is_single_vcpu_fast(struct kvm *kvm, struct kvm_lapic_irq *irq,
 			struct kvm_vcpu **dest_vcpu);
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index a341663..255b5d5 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -7047,7 +7047,7 @@ static int vmx_set_hv_timer(struct kvm_vcpu *vcpu, u64 guest_deadline_tsc,
 
 	vmx = to_vmx(vcpu);
 	tscl = rdtsc();
-	guest_tscl = kvm_read_l1_tsc(vcpu, tscl);
+	guest_tscl = kvm_read_l1_tsc(vcpu, tscl) + get_vmentry_advance_cycles(vcpu);
 	delta_tsc = max(guest_deadline_tsc, guest_tscl) - guest_tscl;
 	lapic_timer_advance_cycles = nsec_to_cycles(vcpu,
 						    ktimer->timer_advance_ns);
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 69c3672e..0d4eb27 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -145,6 +145,13 @@ module_param(tsc_tolerance_ppm, uint, S_IRUGO | S_IWUSR);
 static int __read_mostly lapic_timer_advance_ns = -1;
 module_param(lapic_timer_advance_ns, int, S_IRUGO | S_IWUSR);
 
+/*
+ * lapic timer vmentry advance (tscdeadline mode only) in nanoseconds. Setting
+ * bit 0 to 1 after well manually tuning to cache vmentry advance time.
+ */
+u32 __read_mostly vmentry_lapic_timer_advance_ns = 0;
+module_param(vmentry_lapic_timer_advance_ns, uint, S_IRUGO | S_IWUSR);
+
 static bool __read_mostly vector_hashing = true;
 module_param(vector_hashing, bool, S_IRUGO);
 
@@ -1592,6 +1599,8 @@ static int kvm_set_tsc_khz(struct kvm_vcpu *vcpu, u32 user_tsc_khz)
 	kvm_get_time_scale(user_tsc_khz * 1000LL, NSEC_PER_SEC,
 			   &vcpu->arch.virtual_tsc_shift,
 			   &vcpu->arch.virtual_tsc_mult);
+	if (vcpu->arch.apic && user_tsc_khz != vcpu->arch.virtual_tsc_khz)
+		compute_vmentry_advance_cycles(vcpu);
 	vcpu->arch.virtual_tsc_khz = user_tsc_khz;
 
 	/*
diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index 275b3b6..b0a3b84 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -294,6 +294,8 @@ extern u64 kvm_supported_xcr0(void);
 
 extern unsigned int min_timer_period_us;
 
+extern unsigned int vmentry_lapic_timer_advance_ns;
+
 extern bool enable_vmware_backdoor;
 
 extern struct static_key kvm_no_apic_vcpu;
-- 
2.7.4

[PATCH v2 2/2] KVM: LAPIC: remove the trailing newline used in the fmt parameter of TP_printk

[Wanpeng Li]

From: Wanpeng Li <wanpengli@tencent.com>

The trailing newlines will lead to extra newlines in the trace file
which looks like the following output, so remove it.

qemu-system-x86-15695 [002] ...1 15774.839240: kvm_hv_timer_state: vcpu_id 0 hv_timer 1

qemu-system-x86-15695 [002] ...1 15774.839309: kvm_hv_timer_state: vcpu_id 0 hv_timer 1

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Radim Krčmář <rkrcmar@redhat.com>
Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
---
 arch/x86/kvm/trace.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
index 4d47a26..b5c831e 100644
--- a/arch/x86/kvm/trace.h
+++ b/arch/x86/kvm/trace.h
@@ -1365,7 +1365,7 @@ TRACE_EVENT(kvm_hv_timer_state,
 			__entry->vcpu_id = vcpu_id;
 			__entry->hv_timer_in_use = hv_timer_in_use;
 			),
-		TP_printk("vcpu_id %x hv_timer %x\n",
+		TP_printk("vcpu_id %x hv_timer %x",
 			__entry->vcpu_id,
 			__entry->hv_timer_in_use)
 );
-- 
2.7.4

Re: [PATCH v2 2/2] KVM: LAPIC: remove the trailing newline used in the fmt parameter of TP_printk

[Joe Perches]

On Fri, 2019-05-31 at 14:40 +0800, Wanpeng Li wrote:
> The trailing newlines will lead to extra newlines in the trace file
[]
> diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
[]
> @@ -1365,7 +1365,7 @@ TRACE_EVENT(kvm_hv_timer_state,
>  			__entry->vcpu_id = vcpu_id;
>  			__entry->hv_timer_in_use = hv_timer_in_use;
>  			),
> -		TP_printk("vcpu_id %x hv_timer %x\n",
> +		TP_printk("vcpu_id %x hv_timer %x",
>  			__entry->vcpu_id,
>  			__entry->hv_timer_in_use)
>  );

Not about the kvm subsystem, but generically there are
many of these that could be removed.

$ git grep -w TP_printk | grep '\\n' | wc -l
45

Also, aren't all TP_printk formats supposed to be single line?

If not, these are odd as well.

$ git grep -w TP_printk | grep '\\n[^"]'
include/trace/events/9p.h:	    TP_printk("clnt %lu %s(tag = %d)\n%.3x: %16ph\n%.3x: %16ph\n",
net/tipc/trace.h:	TP_printk("%s\n%s", __get_str(header), __get_str(buf))
net/tipc/trace.h:	TP_printk("%s\n%s", __get_str(header), __get_str(buf))
net/tipc/trace.h:	TP_printk("<%u> %s\n%s%s", __entry->portid, __get_str(header),
net/tipc/trace.h:	TP_printk("<%s> %s\n%s", __entry->name, __get_str(header),
net/tipc/trace.h:	TP_printk("<%x> %s\n%s", __entry->addr, __get_str(header),

Perhaps the documentation files around these formats
	Documentation/trace/events.rst
	Documentation/trace/tracepoints.rst
could be improved as well.

Re: [PATCH v2 2/2] KVM: LAPIC: remove the trailing newline used in the fmt parameter of TP_printk

[Steven Rostedt]

On Fri, 31 May 2019 11:57:04 -0700
Joe Perches <joe@perches.com> wrote:

> On Fri, 2019-05-31 at 14:40 +0800, Wanpeng Li wrote:
> > The trailing newlines will lead to extra newlines in the trace file  
> []
> > diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h  
> []
> > @@ -1365,7 +1365,7 @@ TRACE_EVENT(kvm_hv_timer_state,
> >  			__entry->vcpu_id = vcpu_id;
> >  			__entry->hv_timer_in_use = hv_timer_in_use;
> >  			),
> > -		TP_printk("vcpu_id %x hv_timer %x\n",
> > +		TP_printk("vcpu_id %x hv_timer %x",
> >  			__entry->vcpu_id,
> >  			__entry->hv_timer_in_use)
> >  );  
> 
> Not about the kvm subsystem, but generically there are
> many of these that could be removed.
> 
> $ git grep -w TP_printk | grep '\\n' | wc -l
> 45
> 
> Also, aren't all TP_printk formats supposed to be single line?

Yeah they should be, otherwise it makes the trace look funny. We do
have some legitimate ones (stack traces for example), but really,
unless there's a good reason, it shouldn't have them.

> 
> If not, these are odd as well.
> 
> $ git grep -w TP_printk | grep '\\n[^"]'
> include/trace/events/9p.h:	    TP_printk("clnt %lu %s(tag = %d)\n%.3x: %16ph\n%.3x: %16ph\n",
> net/tipc/trace.h:	TP_printk("%s\n%s", __get_str(header), __get_str(buf))
> net/tipc/trace.h:	TP_printk("%s\n%s", __get_str(header), __get_str(buf))
> net/tipc/trace.h:	TP_printk("<%u> %s\n%s%s", __entry->portid, __get_str(header),
> net/tipc/trace.h:	TP_printk("<%s> %s\n%s", __entry->name, __get_str(header),
> net/tipc/trace.h:	TP_printk("<%x> %s\n%s", __entry->addr, __get_str(header),
> 
> Perhaps the documentation files around these formats
> 	Documentation/trace/events.rst
> 	Documentation/trace/tracepoints.rst
> could be improved as well.
> 

Sure, like most documentation ;-)

-- Steve

Re: [PATCH v2 1/2] KVM: LAPIC: Optimize timer latency consider world switch time

[Sean Christopherson]

On Fri, May 31, 2019 at 02:40:13PM +0800, Wanpeng Li wrote:
> From: Wanpeng Li <wanpengli@tencent.com>
> 
> Advance lapic timer tries to hidden the hypervisor overhead between the
> host emulated timer fires and the guest awares the timer is fired. However,
> even though after more sustaining optimizations, kvm-unit-tests/tscdeadline_latency 
> still awares ~1000 cycles latency since we lost the time between the end of 
> wait_lapic_expire and the guest awares the timer is fired. There are 
> codes between the end of wait_lapic_expire and the world switch, furthermore, 
> the world switch itself also has overhead. Actually the guest_tsc is equal 
> to the target deadline time in wait_lapic_expire is too late, guest will
> aware the latency between the end of wait_lapic_expire() and after vmentry 
> to the guest. This patch takes this time into consideration. 
> 
> The vmentry_lapic_timer_advance_ns module parameter should be well tuned by 
> host admin, setting bit 0 to 1 to finally cache parameter in KVM. This patch 
> can reduce average cyclictest latency from 3us to 2us on Skylake server. 
> (guest w/ nohz=off, idle=poll, host w/ preemption_timer=N, the cyclictest 
> latency is not too sensitive when preemption_timer=Y for this optimization in 
> my testing), kvm-unit-tests/tscdeadline_latency can reach 0.
> 
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Radim Krčmář <rkrcmar@redhat.com>
> Cc: Sean Christopherson <sean.j.christopherson@intel.com>
> Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
> ---
> NOTE: rebase on https://lkml.org/lkml/2019/5/20/449
> v1 -> v2:
>  * rename get_vmentry_advance_delta to get_vmentry_advance_cycles
>  * cache vmentry_advance_cycles by setting param bit 0 
>  * add param max limit 
> 
>  arch/x86/kvm/lapic.c   | 38 +++++++++++++++++++++++++++++++++++---
>  arch/x86/kvm/lapic.h   |  3 +++
>  arch/x86/kvm/vmx/vmx.c |  2 +-
>  arch/x86/kvm/x86.c     |  9 +++++++++
>  arch/x86/kvm/x86.h     |  2 ++
>  5 files changed, 50 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index fcf42a3..60587b5 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -1531,6 +1531,38 @@ static inline void adjust_lapic_timer_advance(struct kvm_vcpu *vcpu,
>  	apic->lapic_timer.timer_advance_ns = timer_advance_ns;
>  }
>  
> +#define MAX_VMENTRY_ADVANCE_NS 1000
> +
> +u64 compute_vmentry_advance_cycles(struct kvm_vcpu *vcpu)

This can be static, unless get_vmentry_advance_cycles() is moved to
lapic.h, in which case compute_vmentry_advance_cycles() would need to be
exported.

> +{
> +	u64 cycles;
> +	struct kvm_lapic *apic = vcpu->arch.apic;
> +	u64 val = min_t(u32, vmentry_lapic_timer_advance_ns, MAX_VMENTRY_ADVANCE_NS);
> +
> +	cycles = (val & ~1ULL) * vcpu->arch.virtual_tsc_khz;
> +	do_div(cycles, 1000000);
> +
> +	/* setting bit 0 locks the value, it is cached */
> +	if (val & 1)
> +		apic->lapic_timer.vmentry_advance_cycles = cycles;
> +
> +	return cycles;
> +}
> +
> +inline u64 get_vmentry_advance_cycles(struct kvm_vcpu *vcpu)

This shouldn't be 'inline' since it's exported from a C file.  That being
said, I think it's short enough to define as a 'static inline' in lapic.h.

> +{
> +	struct kvm_lapic *apic = vcpu->arch.apic;
> +
> +	if (!vmentry_lapic_timer_advance_ns)
> +		return 0;
> +
> +	if (likely(apic->lapic_timer.vmentry_advance_cycles))
> +		return apic->lapic_timer.vmentry_advance_cycles;
> +
> +	return compute_vmentry_advance_cycles(vcpu);
> +}
> +EXPORT_SYMBOL_GPL(get_vmentry_advance_cycles);
> +
>  void kvm_wait_lapic_expire(struct kvm_vcpu *vcpu)
>  {
>  	struct kvm_lapic *apic = vcpu->arch.apic;
> @@ -1544,7 +1576,7 @@ void kvm_wait_lapic_expire(struct kvm_vcpu *vcpu)
>  
>  	tsc_deadline = apic->lapic_timer.expired_tscdeadline;
>  	apic->lapic_timer.expired_tscdeadline = 0;
> -	guest_tsc = kvm_read_l1_tsc(vcpu, rdtsc());
> +	guest_tsc = kvm_read_l1_tsc(vcpu, rdtsc()) + get_vmentry_advance_cycles(vcpu);
>  	apic->lapic_timer.advance_expire_delta = guest_tsc - tsc_deadline;
>  
>  	if (guest_tsc < tsc_deadline)
> @@ -1572,7 +1604,7 @@ static void start_sw_tscdeadline(struct kvm_lapic *apic)
>  	local_irq_save(flags);
>  
>  	now = ktime_get();
> -	guest_tsc = kvm_read_l1_tsc(vcpu, rdtsc());
> +	guest_tsc = kvm_read_l1_tsc(vcpu, rdtsc()) + get_vmentry_advance_cycles(vcpu);
>  
>  	ns = (tscdeadline - guest_tsc) * 1000000ULL;
>  	do_div(ns, this_tsc_khz);
> @@ -2329,7 +2361,7 @@ int kvm_create_lapic(struct kvm_vcpu *vcpu, int timer_advance_ns)
>  		apic->lapic_timer.timer_advance_ns = timer_advance_ns;
>  		apic->lapic_timer.timer_advance_adjust_done = true;
>  	}
> -
> +	apic->lapic_timer.vmentry_advance_cycles = 0;
>  
>  	/*
>  	 * APIC is created enabled. This will prevent kvm_lapic_set_base from
> diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
> index f974a3d..70854a9 100644
> --- a/arch/x86/kvm/lapic.h
> +++ b/arch/x86/kvm/lapic.h
> @@ -33,6 +33,7 @@ struct kvm_timer {
>  	u64 expired_tscdeadline;
>  	u32 timer_advance_ns;
>  	s64 advance_expire_delta;
> +	u64 vmentry_advance_cycles;
>  	atomic_t pending;			/* accumulated triggered timers */
>  	bool hv_timer_in_use;
>  	bool timer_advance_adjust_done;
> @@ -221,6 +222,8 @@ static inline int kvm_lapic_latched_init(struct kvm_vcpu *vcpu)
>  bool kvm_apic_pending_eoi(struct kvm_vcpu *vcpu, int vector);
>  
>  void kvm_wait_lapic_expire(struct kvm_vcpu *vcpu);
> +u64 compute_vmentry_advance_cycles(struct kvm_vcpu *vcpu);
> +inline u64 get_vmentry_advance_cycles(struct kvm_vcpu *vcpu);
>  
>  bool kvm_intr_is_single_vcpu_fast(struct kvm *kvm, struct kvm_lapic_irq *irq,
>  			struct kvm_vcpu **dest_vcpu);
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index a341663..255b5d5 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -7047,7 +7047,7 @@ static int vmx_set_hv_timer(struct kvm_vcpu *vcpu, u64 guest_deadline_tsc,
>  
>  	vmx = to_vmx(vcpu);
>  	tscl = rdtsc();
> -	guest_tscl = kvm_read_l1_tsc(vcpu, tscl);
> +	guest_tscl = kvm_read_l1_tsc(vcpu, tscl) + get_vmentry_advance_cycles(vcpu);
>  	delta_tsc = max(guest_deadline_tsc, guest_tscl) - guest_tscl;
>  	lapic_timer_advance_cycles = nsec_to_cycles(vcpu,
>  						    ktimer->timer_advance_ns);
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 69c3672e..0d4eb27 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -145,6 +145,13 @@ module_param(tsc_tolerance_ppm, uint, S_IRUGO | S_IWUSR);
>  static int __read_mostly lapic_timer_advance_ns = -1;
>  module_param(lapic_timer_advance_ns, int, S_IRUGO | S_IWUSR);
>  
> +/*
> + * lapic timer vmentry advance (tscdeadline mode only) in nanoseconds. Setting
> + * bit 0 to 1 after well manually tuning to cache vmentry advance time.
> + */
> +u32 __read_mostly vmentry_lapic_timer_advance_ns = 0;
> +module_param(vmentry_lapic_timer_advance_ns, uint, S_IRUGO | S_IWUSR);
> +
>  static bool __read_mostly vector_hashing = true;
>  module_param(vector_hashing, bool, S_IRUGO);
>  
> @@ -1592,6 +1599,8 @@ static int kvm_set_tsc_khz(struct kvm_vcpu *vcpu, u32 user_tsc_khz)
>  	kvm_get_time_scale(user_tsc_khz * 1000LL, NSEC_PER_SEC,
>  			   &vcpu->arch.virtual_tsc_shift,
>  			   &vcpu->arch.virtual_tsc_mult);
> +	if (vcpu->arch.apic && user_tsc_khz != vcpu->arch.virtual_tsc_khz)
> +		compute_vmentry_advance_cycles(vcpu);
>  	vcpu->arch.virtual_tsc_khz = user_tsc_khz;
>  
>  	/*
> diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
> index 275b3b6..b0a3b84 100644
> --- a/arch/x86/kvm/x86.h
> +++ b/arch/x86/kvm/x86.h
> @@ -294,6 +294,8 @@ extern u64 kvm_supported_xcr0(void);
>  
>  extern unsigned int min_timer_period_us;
>  
> +extern unsigned int vmentry_lapic_timer_advance_ns;
> +
>  extern bool enable_vmware_backdoor;
>  
>  extern struct static_key kvm_no_apic_vcpu;
> -- 
> 2.7.4
> 

Re: [PATCH v2 1/2] KVM: LAPIC: Optimize timer latency consider world switch time

[Wanpeng Li]

On Tue, 11 Jun 2019 at 09:21, Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
>
> On Fri, May 31, 2019 at 02:40:13PM +0800, Wanpeng Li wrote:
> > From: Wanpeng Li <wanpengli@tencent.com>
> >
> > Advance lapic timer tries to hidden the hypervisor overhead between the
> > host emulated timer fires and the guest awares the timer is fired. However,
> > even though after more sustaining optimizations, kvm-unit-tests/tscdeadline_latency
> > still awares ~1000 cycles latency since we lost the time between the end of
> > wait_lapic_expire and the guest awares the timer is fired. There are
> > codes between the end of wait_lapic_expire and the world switch, furthermore,
> > the world switch itself also has overhead. Actually the guest_tsc is equal
> > to the target deadline time in wait_lapic_expire is too late, guest will
> > aware the latency between the end of wait_lapic_expire() and after vmentry
> > to the guest. This patch takes this time into consideration.
> >
> > The vmentry_lapic_timer_advance_ns module parameter should be well tuned by
> > host admin, setting bit 0 to 1 to finally cache parameter in KVM. This patch
> > can reduce average cyclictest latency from 3us to 2us on Skylake server.
> > (guest w/ nohz=off, idle=poll, host w/ preemption_timer=N, the cyclictest
> > latency is not too sensitive when preemption_timer=Y for this optimization in
> > my testing), kvm-unit-tests/tscdeadline_latency can reach 0.
> >
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Cc: Radim Krčmář <rkrcmar@redhat.com>
> > Cc: Sean Christopherson <sean.j.christopherson@intel.com>
> > Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
> > ---
> > NOTE: rebase on https://lkml.org/lkml/2019/5/20/449
> > v1 -> v2:
> >  * rename get_vmentry_advance_delta to get_vmentry_advance_cycles
> >  * cache vmentry_advance_cycles by setting param bit 0
> >  * add param max limit
> >
> >  arch/x86/kvm/lapic.c   | 38 +++++++++++++++++++++++++++++++++++---
> >  arch/x86/kvm/lapic.h   |  3 +++
> >  arch/x86/kvm/vmx/vmx.c |  2 +-
> >  arch/x86/kvm/x86.c     |  9 +++++++++
> >  arch/x86/kvm/x86.h     |  2 ++
> >  5 files changed, 50 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> > index fcf42a3..60587b5 100644
> > --- a/arch/x86/kvm/lapic.c
> > +++ b/arch/x86/kvm/lapic.c
> > @@ -1531,6 +1531,38 @@ static inline void adjust_lapic_timer_advance(struct kvm_vcpu *vcpu,
> >       apic->lapic_timer.timer_advance_ns = timer_advance_ns;
> >  }
> >
> > +#define MAX_VMENTRY_ADVANCE_NS 1000
> > +
> > +u64 compute_vmentry_advance_cycles(struct kvm_vcpu *vcpu)
>
> This can be static, unless get_vmentry_advance_cycles() is moved to
> lapic.h, in which case compute_vmentry_advance_cycles() would need to be
> exported.

Thanks for the review, Sean. I think Paolo has already drop this one.
https://lkml.org/lkml/2019/5/31/210

Regards,
Wanpeng Li

Re: [PATCH v2 1/2] KVM: LAPIC: Optimize timer latency consider world switch time

[Sean Christopherson]

On Tue, Jun 11, 2019 at 09:38:18AM +0800, Wanpeng Li wrote:
> On Tue, 11 Jun 2019 at 09:21, Sean Christopherson
> <sean.j.christopherson@intel.com> wrote:
> >
> > On Fri, May 31, 2019 at 02:40:13PM +0800, Wanpeng Li wrote:
> > > From: Wanpeng Li <wanpengli@tencent.com>
> > >
> > > Advance lapic timer tries to hidden the hypervisor overhead between the
> > > host emulated timer fires and the guest awares the timer is fired. However,
> > > even though after more sustaining optimizations, kvm-unit-tests/tscdeadline_latency
> > > still awares ~1000 cycles latency since we lost the time between the end of
> > > wait_lapic_expire and the guest awares the timer is fired. There are
> > > codes between the end of wait_lapic_expire and the world switch, furthermore,
> > > the world switch itself also has overhead. Actually the guest_tsc is equal
> > > to the target deadline time in wait_lapic_expire is too late, guest will
> > > aware the latency between the end of wait_lapic_expire() and after vmentry
> > > to the guest. This patch takes this time into consideration.
> > >
> > > The vmentry_lapic_timer_advance_ns module parameter should be well tuned by
> > > host admin, setting bit 0 to 1 to finally cache parameter in KVM. This patch
> > > can reduce average cyclictest latency from 3us to 2us on Skylake server.
> > > (guest w/ nohz=off, idle=poll, host w/ preemption_timer=N, the cyclictest
> > > latency is not too sensitive when preemption_timer=Y for this optimization in
> > > my testing), kvm-unit-tests/tscdeadline_latency can reach 0.
> > >
> > > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > > Cc: Radim Krčmář <rkrcmar@redhat.com>
> > > Cc: Sean Christopherson <sean.j.christopherson@intel.com>
> > > Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
> > > ---
> > > NOTE: rebase on https://lkml.org/lkml/2019/5/20/449
> > > v1 -> v2:
> > >  * rename get_vmentry_advance_delta to get_vmentry_advance_cycles
> > >  * cache vmentry_advance_cycles by setting param bit 0
> > >  * add param max limit
> > >
> > >  arch/x86/kvm/lapic.c   | 38 +++++++++++++++++++++++++++++++++++---
> > >  arch/x86/kvm/lapic.h   |  3 +++
> > >  arch/x86/kvm/vmx/vmx.c |  2 +-
> > >  arch/x86/kvm/x86.c     |  9 +++++++++
> > >  arch/x86/kvm/x86.h     |  2 ++
> > >  5 files changed, 50 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> > > index fcf42a3..60587b5 100644
> > > --- a/arch/x86/kvm/lapic.c
> > > +++ b/arch/x86/kvm/lapic.c
> > > @@ -1531,6 +1531,38 @@ static inline void adjust_lapic_timer_advance(struct kvm_vcpu *vcpu,
> > >       apic->lapic_timer.timer_advance_ns = timer_advance_ns;
> > >  }
> > >
> > > +#define MAX_VMENTRY_ADVANCE_NS 1000
> > > +
> > > +u64 compute_vmentry_advance_cycles(struct kvm_vcpu *vcpu)
> >
> > This can be static, unless get_vmentry_advance_cycles() is moved to
> > lapic.h, in which case compute_vmentry_advance_cycles() would need to be
> > exported.
> 
> Thanks for the review, Sean. I think Paolo has already drop this one.
> https://lkml.org/lkml/2019/5/31/210

I couldn't tell if Paolo's response was "no, don't do that" or "let's be
careful".  :-)

Re: [PATCH v2 1/2] KVM: LAPIC: Optimize timer latency consider world switch time

[Wanpeng Li]

On Tue, 11 Jun 2019 at 09:21, Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
>
> On Fri, May 31, 2019 at 02:40:13PM +0800, Wanpeng Li wrote:
> > From: Wanpeng Li <wanpengli@tencent.com>
> >
> > Advance lapic timer tries to hidden the hypervisor overhead between the
> > host emulated timer fires and the guest awares the timer is fired. However,
> > even though after more sustaining optimizations, kvm-unit-tests/tscdeadline_latency
> > still awares ~1000 cycles latency since we lost the time between the end of
> > wait_lapic_expire and the guest awares the timer is fired. There are
> > codes between the end of wait_lapic_expire and the world switch, furthermore,
> > the world switch itself also has overhead. Actually the guest_tsc is equal
> > to the target deadline time in wait_lapic_expire is too late, guest will
> > aware the latency between the end of wait_lapic_expire() and after vmentry
> > to the guest. This patch takes this time into consideration.
> >
> > The vmentry_lapic_timer_advance_ns module parameter should be well tuned by
> > host admin, setting bit 0 to 1 to finally cache parameter in KVM. This patch
> > can reduce average cyclictest latency from 3us to 2us on Skylake server.
> > (guest w/ nohz=off, idle=poll, host w/ preemption_timer=N, the cyclictest
> > latency is not too sensitive when preemption_timer=Y for this optimization in
> > my testing), kvm-unit-tests/tscdeadline_latency can reach 0.
> >
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Cc: Radim Krčmář <rkrcmar@redhat.com>
> > Cc: Sean Christopherson <sean.j.christopherson@intel.com>
> > Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
> > ---
> > NOTE: rebase on https://lkml.org/lkml/2019/5/20/449
> > v1 -> v2:
> >  * rename get_vmentry_advance_delta to get_vmentry_advance_cycles
> >  * cache vmentry_advance_cycles by setting param bit 0
> >  * add param max limit
> >
> >  arch/x86/kvm/lapic.c   | 38 +++++++++++++++++++++++++++++++++++---
> >  arch/x86/kvm/lapic.h   |  3 +++
> >  arch/x86/kvm/vmx/vmx.c |  2 +-
> >  arch/x86/kvm/x86.c     |  9 +++++++++
> >  arch/x86/kvm/x86.h     |  2 ++
> >  5 files changed, 50 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> > index fcf42a3..60587b5 100644
> > --- a/arch/x86/kvm/lapic.c
> > +++ b/arch/x86/kvm/lapic.c
> > @@ -1531,6 +1531,38 @@ static inline void adjust_lapic_timer_advance(struct kvm_vcpu *vcpu,
> >       apic->lapic_timer.timer_advance_ns = timer_advance_ns;
> >  }
> >
> > +#define MAX_VMENTRY_ADVANCE_NS 1000
> > +
> > +u64 compute_vmentry_advance_cycles(struct kvm_vcpu *vcpu)
>
> This can be static, unless get_vmentry_advance_cycles() is moved to

compute_vmentry_advance_cycles() is also used in x86.c

> lapic.h, in which case compute_vmentry_advance_cycles() would need to be
> exported.
>
> > +{
> > +     u64 cycles;
> > +     struct kvm_lapic *apic = vcpu->arch.apic;
> > +     u64 val = min_t(u32, vmentry_lapic_timer_advance_ns, MAX_VMENTRY_ADVANCE_NS);
> > +
> > +     cycles = (val & ~1ULL) * vcpu->arch.virtual_tsc_khz;
> > +     do_div(cycles, 1000000);
> > +
> > +     /* setting bit 0 locks the value, it is cached */
> > +     if (val & 1)
> > +             apic->lapic_timer.vmentry_advance_cycles = cycles;
> > +
> > +     return cycles;
> > +}
> > +
> > +inline u64 get_vmentry_advance_cycles(struct kvm_vcpu *vcpu)
>
> This shouldn't be 'inline' since it's exported from a C file.  That being

Agreed.

> said, I think it's short enough to define as a 'static inline' in lapic.h.
>
> > +{
> > +     struct kvm_lapic *apic = vcpu->arch.apic;
> > +
> > +     if (!vmentry_lapic_timer_advance_ns)
> > +             return 0;
> > +
> > +     if (likely(apic->lapic_timer.vmentry_advance_cycles))
> > +             return apic->lapic_timer.vmentry_advance_cycles;
> > +
> > +     return compute_vmentry_advance_cycles(vcpu);
> > +}
> > +EXPORT_SYMBOL_GPL(get_vmentry_advance_cycles);
> > +
> >  void kvm_wait_lapic_expire(struct kvm_vcpu *vcpu)
> >  {
> >       struct kvm_lapic *apic = vcpu->arch.apic;
> > @@ -1544,7 +1576,7 @@ void kvm_wait_lapic_expire(struct kvm_vcpu *vcpu)
> >
> >       tsc_deadline = apic->lapic_timer.expired_tscdeadline;
> >       apic->lapic_timer.expired_tscdeadline = 0;
> > -     guest_tsc = kvm_read_l1_tsc(vcpu, rdtsc());
> > +     guest_tsc = kvm_read_l1_tsc(vcpu, rdtsc()) + get_vmentry_advance_cycles(vcpu);
> >       apic->lapic_timer.advance_expire_delta = guest_tsc - tsc_deadline;
> >
> >       if (guest_tsc < tsc_deadline)
> > @@ -1572,7 +1604,7 @@ static void start_sw_tscdeadline(struct kvm_lapic *apic)
> >       local_irq_save(flags);
> >
> >       now = ktime_get();
> > -     guest_tsc = kvm_read_l1_tsc(vcpu, rdtsc());
> > +     guest_tsc = kvm_read_l1_tsc(vcpu, rdtsc()) + get_vmentry_advance_cycles(vcpu);
> >
> >       ns = (tscdeadline - guest_tsc) * 1000000ULL;
> >       do_div(ns, this_tsc_khz);
> > @@ -2329,7 +2361,7 @@ int kvm_create_lapic(struct kvm_vcpu *vcpu, int timer_advance_ns)
> >               apic->lapic_timer.timer_advance_ns = timer_advance_ns;
> >               apic->lapic_timer.timer_advance_adjust_done = true;
> >       }
> > -
> > +     apic->lapic_timer.vmentry_advance_cycles = 0;
> >
> >       /*
> >        * APIC is created enabled. This will prevent kvm_lapic_set_base from
> > diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
> > index f974a3d..70854a9 100644
> > --- a/arch/x86/kvm/lapic.h
> > +++ b/arch/x86/kvm/lapic.h
> > @@ -33,6 +33,7 @@ struct kvm_timer {
> >       u64 expired_tscdeadline;
> >       u32 timer_advance_ns;
> >       s64 advance_expire_delta;
> > +     u64 vmentry_advance_cycles;
> >       atomic_t pending;                       /* accumulated triggered timers */
> >       bool hv_timer_in_use;
> >       bool timer_advance_adjust_done;
> > @@ -221,6 +222,8 @@ static inline int kvm_lapic_latched_init(struct kvm_vcpu *vcpu)
> >  bool kvm_apic_pending_eoi(struct kvm_vcpu *vcpu, int vector);
> >
> >  void kvm_wait_lapic_expire(struct kvm_vcpu *vcpu);
> > +u64 compute_vmentry_advance_cycles(struct kvm_vcpu *vcpu);
> > +inline u64 get_vmentry_advance_cycles(struct kvm_vcpu *vcpu);
> >
> >  bool kvm_intr_is_single_vcpu_fast(struct kvm *kvm, struct kvm_lapic_irq *irq,
> >                       struct kvm_vcpu **dest_vcpu);
> > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> > index a341663..255b5d5 100644
> > --- a/arch/x86/kvm/vmx/vmx.c
> > +++ b/arch/x86/kvm/vmx/vmx.c
> > @@ -7047,7 +7047,7 @@ static int vmx_set_hv_timer(struct kvm_vcpu *vcpu, u64 guest_deadline_tsc,
> >
> >       vmx = to_vmx(vcpu);
> >       tscl = rdtsc();
> > -     guest_tscl = kvm_read_l1_tsc(vcpu, tscl);
> > +     guest_tscl = kvm_read_l1_tsc(vcpu, tscl) + get_vmentry_advance_cycles(vcpu);
> >       delta_tsc = max(guest_deadline_tsc, guest_tscl) - guest_tscl;
> >       lapic_timer_advance_cycles = nsec_to_cycles(vcpu,
> >                                                   ktimer->timer_advance_ns);
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 69c3672e..0d4eb27 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -145,6 +145,13 @@ module_param(tsc_tolerance_ppm, uint, S_IRUGO | S_IWUSR);
> >  static int __read_mostly lapic_timer_advance_ns = -1;
> >  module_param(lapic_timer_advance_ns, int, S_IRUGO | S_IWUSR);
> >
> > +/*
> > + * lapic timer vmentry advance (tscdeadline mode only) in nanoseconds. Setting
> > + * bit 0 to 1 after well manually tuning to cache vmentry advance time.
> > + */
> > +u32 __read_mostly vmentry_lapic_timer_advance_ns = 0;
> > +module_param(vmentry_lapic_timer_advance_ns, uint, S_IRUGO | S_IWUSR);
> > +
> >  static bool __read_mostly vector_hashing = true;
> >  module_param(vector_hashing, bool, S_IRUGO);
> >
> > @@ -1592,6 +1599,8 @@ static int kvm_set_tsc_khz(struct kvm_vcpu *vcpu, u32 user_tsc_khz)
> >       kvm_get_time_scale(user_tsc_khz * 1000LL, NSEC_PER_SEC,
> >                          &vcpu->arch.virtual_tsc_shift,
> >                          &vcpu->arch.virtual_tsc_mult);
> > +     if (vcpu->arch.apic && user_tsc_khz != vcpu->arch.virtual_tsc_khz)
> > +             compute_vmentry_advance_cycles(vcpu);
> >       vcpu->arch.virtual_tsc_khz = user_tsc_khz;
> >
> >       /*
> > diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
> > index 275b3b6..b0a3b84 100644
> > --- a/arch/x86/kvm/x86.h
> > +++ b/arch/x86/kvm/x86.h
> > @@ -294,6 +294,8 @@ extern u64 kvm_supported_xcr0(void);
> >
> >  extern unsigned int min_timer_period_us;
> >
> > +extern unsigned int vmentry_lapic_timer_advance_ns;
> > +
> >  extern bool enable_vmware_backdoor;
> >
> >  extern struct static_key kvm_no_apic_vcpu;
> > --
> > 2.7.4
> >