[PATCH v2 1/3] KVM: make halt_poll_ns per-VCPU

[PATCH v2 1/3] KVM: make halt_poll_ns per-VCPU

[Wanpeng Li]

Change halt_poll_ns into per-VCPU variable, seeded from module parameter,
to allow greater flexibility.

Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
---
 include/linux/kvm_host.h | 1 +
 virt/kvm/kvm_main.c      | 5 +++--
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 81089cf..1bef9e2 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -242,6 +242,7 @@ struct kvm_vcpu {
 	int sigset_active;
 	sigset_t sigset;
 	struct kvm_vcpu_stat stat;
+	unsigned int halt_poll_ns;
 
 #ifdef CONFIG_HAS_IOMEM
 	int mmio_needed;
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index d8db2f8f..93db833 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -217,6 +217,7 @@ int kvm_vcpu_init(struct kvm_vcpu *vcpu, struct kvm *kvm, unsigned id)
 	vcpu->kvm = kvm;
 	vcpu->vcpu_id = id;
 	vcpu->pid = NULL;
+	vcpu->halt_poll_ns = halt_poll_ns;
 	init_waitqueue_head(&vcpu->wq);
 	kvm_async_pf_vcpu_init(vcpu);
 
@@ -1930,8 +1931,8 @@ void kvm_vcpu_block(struct kvm_vcpu *vcpu)
 	bool waited = false;
 
 	start = cur = ktime_get();
-	if (halt_poll_ns) {
-		ktime_t stop = ktime_add_ns(ktime_get(), halt_poll_ns);
+	if (vcpu->halt_poll_ns) {
+		ktime_t stop = ktime_add_ns(ktime_get(), vcpu->halt_poll_ns);
 
 		do {
 			/*
-- 
1.9.1

[PATCH v2 2/3] KVM: dynamic halt_poll_ns adjustment

[Wanpeng Li]

There is a downside of halt_poll_ns since poll is still happen for idle 
VCPU which can waste cpu usage. This patch adds the ability to adjust 
halt_poll_ns dynamically. 

There are two new kernel parameters for changing the halt_poll_ns:
halt_poll_ns_grow and halt_poll_ns_shrink. A third new parameter, 
halt_poll_ns_max, controls the maximal halt_poll_ns; it is internally 
rounded down to a closest multiple of halt_poll_ns_grow. The shrink/grow 
matrix is suggested by David: 

if (poll successfully for interrupt): stay the same
  else if (length of kvm_vcpu_block is longer than halt_poll_ns_max): shrink
  else if (length of kvm_vcpu_block is less than halt_poll_ns_max): grow

  halt_poll_ns_shrink/ |
  halt_poll_ns_grow    | grow halt_poll_ns    | shrink halt_poll_ns 
  ---------------------+----------------------+-------------------
  < 1                  |  = halt_poll_ns      |  = 0 
  < halt_poll_ns       | *= halt_poll_ns_grow | /= halt_poll_ns_shrink
  otherwise            | += halt_poll_ns_grow | -= halt_poll_ns_shrink

Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
---
 virt/kvm/kvm_main.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 64 insertions(+), 1 deletion(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 93db833..2a4962b 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -66,9 +66,26 @@
 MODULE_AUTHOR("Qumranet");
 MODULE_LICENSE("GPL");
 
-static unsigned int halt_poll_ns;
+#define KVM_HALT_POLL_NS  500000
+#define KVM_HALT_POLL_NS_GROW   2
+#define KVM_HALT_POLL_NS_SHRINK 0
+#define KVM_HALT_POLL_NS_MAX 2000000
+
+static unsigned int halt_poll_ns = KVM_HALT_POLL_NS;
 module_param(halt_poll_ns, uint, S_IRUGO | S_IWUSR);
 
+/* Default doubles per-vcpu halt_poll_ns. */
+static unsigned int halt_poll_ns_grow = KVM_HALT_POLL_NS_GROW;
+module_param(halt_poll_ns_grow, int, S_IRUGO);
+
+/* Default resets per-vcpu halt_poll_ns . */
+static unsigned int halt_poll_ns_shrink = KVM_HALT_POLL_NS_SHRINK;
+module_param(halt_poll_ns_shrink, int, S_IRUGO);
+
+/* halt polling only reduces halt latency by 10-15 us, 2ms is enough */
+static unsigned int halt_poll_ns_max = KVM_HALT_POLL_NS_MAX;
+module_param(halt_poll_ns_max, int, S_IRUGO);
+
 /*
  * Ordering of locks:
  *
@@ -1907,6 +1924,48 @@ void kvm_vcpu_mark_page_dirty(struct kvm_vcpu *vcpu, gfn_t gfn)
 }
 EXPORT_SYMBOL_GPL(kvm_vcpu_mark_page_dirty);
 
+static unsigned int __grow_halt_poll_ns(unsigned int val)
+{
+	if (halt_poll_ns_grow < 1)
+		return halt_poll_ns;
+
+	val = min(val, halt_poll_ns_max);
+
+	if (val == 0)
+		return halt_poll_ns;
+
+	if (halt_poll_ns_grow < halt_poll_ns)
+		val *= halt_poll_ns_grow;
+	else
+		val += halt_poll_ns_grow;
+
+	return val;
+}
+
+static unsigned int __shrink_halt_poll_ns(int val, int modifier, int minimum)
+{
+	if (modifier < 1)
+		return 0;
+
+	if (modifier < halt_poll_ns)
+		val /= modifier;
+	else
+		val -= modifier;
+
+	return val;
+}
+
+static void grow_halt_poll_ns(struct kvm_vcpu *vcpu)
+{
+	vcpu->halt_poll_ns = __grow_halt_poll_ns(vcpu->halt_poll_ns);
+}
+
+static void shrink_halt_poll_ns(struct kvm_vcpu *vcpu)
+{
+	vcpu->halt_poll_ns = __shrink_halt_poll_ns(vcpu->halt_poll_ns,
+			halt_poll_ns_shrink, halt_poll_ns);
+}
+
 static int kvm_vcpu_check_block(struct kvm_vcpu *vcpu)
 {
 	if (kvm_arch_vcpu_runnable(vcpu)) {
@@ -1954,6 +2013,10 @@ void kvm_vcpu_block(struct kvm_vcpu *vcpu)
 			break;
 
 		waited = true;
+		if (vcpu->halt_poll_ns > halt_poll_ns_max)
+			shrink_halt_poll_ns(vcpu);
+		else
+			grow_halt_poll_ns(vcpu);
 		schedule();
 	}
 
-- 
1.9.1

[PATCH v2 3/3] KVM: trace kvm_halt_poll_ns grow/shrink

[Wanpeng Li]

Tracepoint for dynamic halt_pool_ns, fired on every potential change.

Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
---
 include/trace/events/kvm.h | 30 ++++++++++++++++++++++++++++++
 virt/kvm/kvm_main.c        |  8 ++++++++
 2 files changed, 38 insertions(+)

diff --git a/include/trace/events/kvm.h b/include/trace/events/kvm.h
index a44062d..75ddf80 100644
--- a/include/trace/events/kvm.h
+++ b/include/trace/events/kvm.h
@@ -356,6 +356,36 @@ TRACE_EVENT(
 		  __entry->address)
 );
 
+TRACE_EVENT(kvm_halt_poll_ns,
+	TP_PROTO(bool grow, unsigned int vcpu_id, int new, int old),
+	TP_ARGS(grow, vcpu_id, new, old),
+
+	TP_STRUCT__entry(
+		__field(bool, grow)
+		__field(unsigned int, vcpu_id)
+		__field(int, new)
+		__field(int, old)
+	),
+
+	TP_fast_assign(
+		__entry->grow           = grow;
+		__entry->vcpu_id        = vcpu_id;
+		__entry->new            = new;
+		__entry->old            = old;
+	),
+
+	TP_printk("vcpu %u: halt_pool_ns %d (%s %d)",
+			__entry->vcpu_id,
+			__entry->new,
+			__entry->grow ? "grow" : "shrink",
+			__entry->old)
+);
+
+#define trace_kvm_halt_poll_ns_grow(vcpu_id, new, old) \
+	trace_kvm_halt_poll_ns(true, vcpu_id, new, old)
+#define trace_kvm_halt_poll_ns_shrink(vcpu_id, new, old) \
+	trace_kvm_halt_poll_ns(false, vcpu_id, new, old)
+
 #endif
 
 #endif /* _TRACE_KVM_MAIN_H */
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 2a4962b..04f62e0 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1957,13 +1957,21 @@ static unsigned int __shrink_halt_poll_ns(int val, int modifier, int minimum)
 
 static void grow_halt_poll_ns(struct kvm_vcpu *vcpu)
 {
+	int old = vcpu->halt_poll_ns;
+
 	vcpu->halt_poll_ns = __grow_halt_poll_ns(vcpu->halt_poll_ns);
+
+	trace_kvm_halt_poll_ns_grow(vcpu->vcpu_id, vcpu->halt_poll_ns, old);
 }
 
 static void shrink_halt_poll_ns(struct kvm_vcpu *vcpu)
 {
+	int old = vcpu->halt_poll_ns;
+
 	vcpu->halt_poll_ns = __shrink_halt_poll_ns(vcpu->halt_poll_ns,
 			halt_poll_ns_shrink, halt_poll_ns);
+
+	trace_kvm_halt_poll_ns_shrink(vcpu->vcpu_id, vcpu->halt_poll_ns, old);
 }
 
 static int kvm_vcpu_check_block(struct kvm_vcpu *vcpu)
-- 
1.9.1

Re: [PATCH v2 2/3] KVM: dynamic halt_poll_ns adjustment

[David Matlack]

Thanks for writing v2, Wanpeng.

On Mon, Aug 24, 2015 at 11:35 PM, Wanpeng Li <wanpeng.li@hotmail.com> wrote:
> There is a downside of halt_poll_ns since poll is still happen for idle
> VCPU which can waste cpu usage. This patch adds the ability to adjust
> halt_poll_ns dynamically.

What testing have you done with these patches? Do you know if this removes
the overhead of polling in idle VCPUs? Do we lose any of the performance
from always polling?

>
> There are two new kernel parameters for changing the halt_poll_ns:
> halt_poll_ns_grow and halt_poll_ns_shrink. A third new parameter,
> halt_poll_ns_max, controls the maximal halt_poll_ns; it is internally
> rounded down to a closest multiple of halt_poll_ns_grow. The shrink/grow
> matrix is suggested by David:
>
> if (poll successfully for interrupt): stay the same
>   else if (length of kvm_vcpu_block is longer than halt_poll_ns_max): shrink
>   else if (length of kvm_vcpu_block is less than halt_poll_ns_max): grow

The way you implemented this wasn't what I expected. I thought you would time
the whole function (kvm_vcpu_block). But I like your approach better. It's
simpler and [by inspection] does what we want.

>
>   halt_poll_ns_shrink/ |
>   halt_poll_ns_grow    | grow halt_poll_ns    | shrink halt_poll_ns
>   ---------------------+----------------------+-------------------
>   < 1                  |  = halt_poll_ns      |  = 0
>   < halt_poll_ns       | *= halt_poll_ns_grow | /= halt_poll_ns_shrink
>   otherwise            | += halt_poll_ns_grow | -= halt_poll_ns_shrink

I was curious why you went with this approach rather than just the
middle row, or just the last row. Do you think we'll want the extra
flexibility?

>
> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
> ---
>  virt/kvm/kvm_main.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 64 insertions(+), 1 deletion(-)
>
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 93db833..2a4962b 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -66,9 +66,26 @@
>  MODULE_AUTHOR("Qumranet");
>  MODULE_LICENSE("GPL");
>
> -static unsigned int halt_poll_ns;
> +#define KVM_HALT_POLL_NS  500000
> +#define KVM_HALT_POLL_NS_GROW   2
> +#define KVM_HALT_POLL_NS_SHRINK 0
> +#define KVM_HALT_POLL_NS_MAX 2000000

The macros are not necessary. Also, hard coding the numbers in the param
definitions will make reading the comments above them easier.

> +
> +static unsigned int halt_poll_ns = KVM_HALT_POLL_NS;
>  module_param(halt_poll_ns, uint, S_IRUGO | S_IWUSR);
>
> +/* Default doubles per-vcpu halt_poll_ns. */
> +static unsigned int halt_poll_ns_grow = KVM_HALT_POLL_NS_GROW;
> +module_param(halt_poll_ns_grow, int, S_IRUGO);
> +
> +/* Default resets per-vcpu halt_poll_ns . */
> +static unsigned int halt_poll_ns_shrink = KVM_HALT_POLL_NS_SHRINK;
> +module_param(halt_poll_ns_shrink, int, S_IRUGO);
> +
> +/* halt polling only reduces halt latency by 10-15 us, 2ms is enough */

Ah, I misspoke before. I was thinking about round-trip latency. The latency
of a single halt is reduced by about 5-7 us.

> +static unsigned int halt_poll_ns_max = KVM_HALT_POLL_NS_MAX;
> +module_param(halt_poll_ns_max, int, S_IRUGO);

We can remove halt_poll_ns_max. vcpu->halt_poll_ns can always start at zero
and grow from there. Then we just need one module param to keep
vcpu->halt_poll_ns from growing too large.

[ It would make more sense to remove halt_poll_ns and keep halt_poll_ns_max,
  but since halt_poll_ns already exists in upstream kernels, we probably can't
  remove it. ]

> +
>  /*
>   * Ordering of locks:
>   *
> @@ -1907,6 +1924,48 @@ void kvm_vcpu_mark_page_dirty(struct kvm_vcpu *vcpu, gfn_t gfn)
>  }
>  EXPORT_SYMBOL_GPL(kvm_vcpu_mark_page_dirty);
>
> +static unsigned int __grow_halt_poll_ns(unsigned int val)
> +{
> +       if (halt_poll_ns_grow < 1)
> +               return halt_poll_ns;
> +
> +       val = min(val, halt_poll_ns_max);
> +
> +       if (val == 0)
> +               return halt_poll_ns;
> +
> +       if (halt_poll_ns_grow < halt_poll_ns)
> +               val *= halt_poll_ns_grow;
> +       else
> +               val += halt_poll_ns_grow;
> +
> +       return val;
> +}
> +
> +static unsigned int __shrink_halt_poll_ns(int val, int modifier, int minimum)

minimum never gets used.

> +{
> +       if (modifier < 1)
> +               return 0;
> +
> +       if (modifier < halt_poll_ns)
> +               val /= modifier;
> +       else
> +               val -= modifier;
> +
> +       return val;
> +}
> +
> +static void grow_halt_poll_ns(struct kvm_vcpu *vcpu)

These wrappers aren't necessary.

> +{
> +       vcpu->halt_poll_ns = __grow_halt_poll_ns(vcpu->halt_poll_ns);
> +}
> +
> +static void shrink_halt_poll_ns(struct kvm_vcpu *vcpu)
> +{
> +       vcpu->halt_poll_ns = __shrink_halt_poll_ns(vcpu->halt_poll_ns,
> +                       halt_poll_ns_shrink, halt_poll_ns);
> +}
> +
>  static int kvm_vcpu_check_block(struct kvm_vcpu *vcpu)
>  {
>         if (kvm_arch_vcpu_runnable(vcpu)) {
> @@ -1954,6 +2013,10 @@ void kvm_vcpu_block(struct kvm_vcpu *vcpu)
>                         break;
>
>                 waited = true;
> +               if (vcpu->halt_poll_ns > halt_poll_ns_max)
> +                       shrink_halt_poll_ns(vcpu);
> +               else
> +                       grow_halt_poll_ns(vcpu);

Shouldn't this go after the loop, and before "out:", in case we schedule
more than once? You can gate it on "if (waited)" so it only runs if we
actually scheduled.

>                 schedule();
>         }
>
> --
> 1.9.1
>

Re: [PATCH v2 2/3] KVM: dynamic halt_poll_ns adjustment

[Wanpeng Li]

On 8/26/15 1:19 AM, David Matlack wrote:
> Thanks for writing v2, Wanpeng.
>
> On Mon, Aug 24, 2015 at 11:35 PM, Wanpeng Li <wanpeng.li@hotmail.com> wrote:
>> There is a downside of halt_poll_ns since poll is still happen for idle
>> VCPU which can waste cpu usage. This patch adds the ability to adjust
>> halt_poll_ns dynamically.
> What testing have you done with these patches? Do you know if this removes
> the overhead of polling in idle VCPUs? Do we lose any of the performance
> from always polling?

There is 0.8% kernel compile performance improvement(10 iterations) on 
host w/ 800 idle vCPUs. Btw, I handle all your comments in v3. Many 
thanks for your review, David! :-)

Regards,
Wanpeng Li

Re: [PATCH v2 2/3] KVM: dynamic halt_poll_ns adjustment

[Wanpeng Li]

Hi David,
On 8/26/15 1:19 AM, David Matlack wrote:
> Thanks for writing v2, Wanpeng.
>
> On Mon, Aug 24, 2015 at 11:35 PM, Wanpeng Li <wanpeng.li@hotmail.com> wrote:
>> There is a downside of halt_poll_ns since poll is still happen for idle
>> VCPU which can waste cpu usage. This patch adds the ability to adjust
>> halt_poll_ns dynamically.
> What testing have you done with these patches? Do you know if this removes
> the overhead of polling in idle VCPUs? Do we lose any of the performance
> from always polling?
>
>> There are two new kernel parameters for changing the halt_poll_ns:
>> halt_poll_ns_grow and halt_poll_ns_shrink. A third new parameter,
>> halt_poll_ns_max, controls the maximal halt_poll_ns; it is internally
>> rounded down to a closest multiple of halt_poll_ns_grow. The shrink/grow
>> matrix is suggested by David:
>>
>> if (poll successfully for interrupt): stay the same
>>    else if (length of kvm_vcpu_block is longer than halt_poll_ns_max): shrink
>>    else if (length of kvm_vcpu_block is less than halt_poll_ns_max): grow
> The way you implemented this wasn't what I expected. I thought you would time
> the whole function (kvm_vcpu_block). But I like your approach better. It's
> simpler and [by inspection] does what we want.

I see there is more idle vCPUs overhead w/ this method even more than 
always halt-poll, so I bring back grow vcpu->halt_poll_ns when interrupt 
arrives and shrinks when idle VCPU is detected. The perfomance looks 
good in v4.

Regards,
Wanpeng Li

>
>>    halt_poll_ns_shrink/ |
>>    halt_poll_ns_grow    | grow halt_poll_ns    | shrink halt_poll_ns
>>    ---------------------+----------------------+-------------------
>>    < 1                  |  = halt_poll_ns      |  = 0
>>    < halt_poll_ns       | *= halt_poll_ns_grow | /= halt_poll_ns_shrink
>>    otherwise            | += halt_poll_ns_grow | -= halt_poll_ns_shrink
> I was curious why you went with this approach rather than just the
> middle row, or just the last row. Do you think we'll want the extra
> flexibility?
>
>> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
>> ---
>>   virt/kvm/kvm_main.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 64 insertions(+), 1 deletion(-)
>>
>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>> index 93db833..2a4962b 100644
>> --- a/virt/kvm/kvm_main.c
>> +++ b/virt/kvm/kvm_main.c
>> @@ -66,9 +66,26 @@
>>   MODULE_AUTHOR("Qumranet");
>>   MODULE_LICENSE("GPL");
>>
>> -static unsigned int halt_poll_ns;
>> +#define KVM_HALT_POLL_NS  500000
>> +#define KVM_HALT_POLL_NS_GROW   2
>> +#define KVM_HALT_POLL_NS_SHRINK 0
>> +#define KVM_HALT_POLL_NS_MAX 2000000
> The macros are not necessary. Also, hard coding the numbers in the param
> definitions will make reading the comments above them easier.
>
>> +
>> +static unsigned int halt_poll_ns = KVM_HALT_POLL_NS;
>>   module_param(halt_poll_ns, uint, S_IRUGO | S_IWUSR);
>>
>> +/* Default doubles per-vcpu halt_poll_ns. */
>> +static unsigned int halt_poll_ns_grow = KVM_HALT_POLL_NS_GROW;
>> +module_param(halt_poll_ns_grow, int, S_IRUGO);
>> +
>> +/* Default resets per-vcpu halt_poll_ns . */
>> +static unsigned int halt_poll_ns_shrink = KVM_HALT_POLL_NS_SHRINK;
>> +module_param(halt_poll_ns_shrink, int, S_IRUGO);
>> +
>> +/* halt polling only reduces halt latency by 10-15 us, 2ms is enough */
> Ah, I misspoke before. I was thinking about round-trip latency. The latency
> of a single halt is reduced by about 5-7 us.
>
>> +static unsigned int halt_poll_ns_max = KVM_HALT_POLL_NS_MAX;
>> +module_param(halt_poll_ns_max, int, S_IRUGO);
> We can remove halt_poll_ns_max. vcpu->halt_poll_ns can always start at zero
> and grow from there. Then we just need one module param to keep
> vcpu->halt_poll_ns from growing too large.
>
> [ It would make more sense to remove halt_poll_ns and keep halt_poll_ns_max,
>    but since halt_poll_ns already exists in upstream kernels, we probably can't
>    remove it. ]
>
>> +
>>   /*
>>    * Ordering of locks:
>>    *
>> @@ -1907,6 +1924,48 @@ void kvm_vcpu_mark_page_dirty(struct kvm_vcpu *vcpu, gfn_t gfn)
>>   }
>>   EXPORT_SYMBOL_GPL(kvm_vcpu_mark_page_dirty);
>>
>> +static unsigned int __grow_halt_poll_ns(unsigned int val)
>> +{
>> +       if (halt_poll_ns_grow < 1)
>> +               return halt_poll_ns;
>> +
>> +       val = min(val, halt_poll_ns_max);
>> +
>> +       if (val == 0)
>> +               return halt_poll_ns;
>> +
>> +       if (halt_poll_ns_grow < halt_poll_ns)
>> +               val *= halt_poll_ns_grow;
>> +       else
>> +               val += halt_poll_ns_grow;
>> +
>> +       return val;
>> +}
>> +
>> +static unsigned int __shrink_halt_poll_ns(int val, int modifier, int minimum)
> minimum never gets used.
>
>> +{
>> +       if (modifier < 1)
>> +               return 0;
>> +
>> +       if (modifier < halt_poll_ns)
>> +               val /= modifier;
>> +       else
>> +               val -= modifier;
>> +
>> +       return val;
>> +}
>> +
>> +static void grow_halt_poll_ns(struct kvm_vcpu *vcpu)
> These wrappers aren't necessary.
>
>> +{
>> +       vcpu->halt_poll_ns = __grow_halt_poll_ns(vcpu->halt_poll_ns);
>> +}
>> +
>> +static void shrink_halt_poll_ns(struct kvm_vcpu *vcpu)
>> +{
>> +       vcpu->halt_poll_ns = __shrink_halt_poll_ns(vcpu->halt_poll_ns,
>> +                       halt_poll_ns_shrink, halt_poll_ns);
>> +}
>> +
>>   static int kvm_vcpu_check_block(struct kvm_vcpu *vcpu)
>>   {
>>          if (kvm_arch_vcpu_runnable(vcpu)) {
>> @@ -1954,6 +2013,10 @@ void kvm_vcpu_block(struct kvm_vcpu *vcpu)
>>                          break;
>>
>>                  waited = true;
>> +               if (vcpu->halt_poll_ns > halt_poll_ns_max)
>> +                       shrink_halt_poll_ns(vcpu);
>> +               else
>> +                       grow_halt_poll_ns(vcpu);
> Shouldn't this go after the loop, and before "out:", in case we schedule
> more than once? You can gate it on "if (waited)" so it only runs if we
> actually scheduled.
>
>>                  schedule();
>>          }
>>
>> --
>> 1.9.1
>>

Re: [PATCH v2 2/3] KVM: dynamic halt_poll_ns adjustment

[David Matlack]

On Thu, Aug 27, 2015 at 2:59 AM, Wanpeng Li <wanpeng.li@hotmail.com> wrote:
> Hi David,
> On 8/26/15 1:19 AM, David Matlack wrote:
>>
>> Thanks for writing v2, Wanpeng.
>>
>> On Mon, Aug 24, 2015 at 11:35 PM, Wanpeng Li <wanpeng.li@hotmail.com>
>> wrote:
>>>
>>> There is a downside of halt_poll_ns since poll is still happen for idle
>>> VCPU which can waste cpu usage. This patch adds the ability to adjust
>>> halt_poll_ns dynamically.
>>
>> What testing have you done with these patches? Do you know if this removes
>> the overhead of polling in idle VCPUs? Do we lose any of the performance
>> from always polling?
>>
>>> There are two new kernel parameters for changing the halt_poll_ns:
>>> halt_poll_ns_grow and halt_poll_ns_shrink. A third new parameter,
>>> halt_poll_ns_max, controls the maximal halt_poll_ns; it is internally
>>> rounded down to a closest multiple of halt_poll_ns_grow. The shrink/grow
>>> matrix is suggested by David:
>>>
>>> if (poll successfully for interrupt): stay the same
>>>    else if (length of kvm_vcpu_block is longer than halt_poll_ns_max):
>>> shrink
>>>    else if (length of kvm_vcpu_block is less than halt_poll_ns_max): grow
>>
>> The way you implemented this wasn't what I expected. I thought you would
>> time
>> the whole function (kvm_vcpu_block). But I like your approach better. It's
>> simpler and [by inspection] does what we want.
>
>
> I see there is more idle vCPUs overhead w/ this method even more than always
> halt-poll, so I bring back grow vcpu->halt_poll_ns when interrupt arrives
> and shrinks when idle VCPU is detected. The perfomance looks good in v4.

Why did this patch have a worse idle overhead than always poll?

>
> Regards,
> Wanpeng Li
>
>
>>
>>>    halt_poll_ns_shrink/ |
>>>    halt_poll_ns_grow    | grow halt_poll_ns    | shrink halt_poll_ns
>>>    ---------------------+----------------------+-------------------
>>>    < 1                  |  = halt_poll_ns      |  = 0
>>>    < halt_poll_ns       | *= halt_poll_ns_grow | /= halt_poll_ns_shrink
>>>    otherwise            | += halt_poll_ns_grow | -= halt_poll_ns_shrink
>>
>> I was curious why you went with this approach rather than just the
>> middle row, or just the last row. Do you think we'll want the extra
>> flexibility?
>>
>>> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
>>> ---
>>>   virt/kvm/kvm_main.c | 65
>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>>   1 file changed, 64 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>>> index 93db833..2a4962b 100644
>>> --- a/virt/kvm/kvm_main.c
>>> +++ b/virt/kvm/kvm_main.c
>>> @@ -66,9 +66,26 @@
>>>   MODULE_AUTHOR("Qumranet");
>>>   MODULE_LICENSE("GPL");
>>>
>>> -static unsigned int halt_poll_ns;
>>> +#define KVM_HALT_POLL_NS  500000
>>> +#define KVM_HALT_POLL_NS_GROW   2
>>> +#define KVM_HALT_POLL_NS_SHRINK 0
>>> +#define KVM_HALT_POLL_NS_MAX 2000000
>>
>> The macros are not necessary. Also, hard coding the numbers in the param
>> definitions will make reading the comments above them easier.
>>
>>> +
>>> +static unsigned int halt_poll_ns = KVM_HALT_POLL_NS;
>>>   module_param(halt_poll_ns, uint, S_IRUGO | S_IWUSR);
>>>
>>> +/* Default doubles per-vcpu halt_poll_ns. */
>>> +static unsigned int halt_poll_ns_grow = KVM_HALT_POLL_NS_GROW;
>>> +module_param(halt_poll_ns_grow, int, S_IRUGO);
>>> +
>>> +/* Default resets per-vcpu halt_poll_ns . */
>>> +static unsigned int halt_poll_ns_shrink = KVM_HALT_POLL_NS_SHRINK;
>>> +module_param(halt_poll_ns_shrink, int, S_IRUGO);
>>> +
>>> +/* halt polling only reduces halt latency by 10-15 us, 2ms is enough */
>>
>> Ah, I misspoke before. I was thinking about round-trip latency. The
>> latency
>> of a single halt is reduced by about 5-7 us.
>>
>>> +static unsigned int halt_poll_ns_max = KVM_HALT_POLL_NS_MAX;
>>> +module_param(halt_poll_ns_max, int, S_IRUGO);
>>
>> We can remove halt_poll_ns_max. vcpu->halt_poll_ns can always start at
>> zero
>> and grow from there. Then we just need one module param to keep
>> vcpu->halt_poll_ns from growing too large.
>>
>> [ It would make more sense to remove halt_poll_ns and keep
>> halt_poll_ns_max,
>>    but since halt_poll_ns already exists in upstream kernels, we probably
>> can't
>>    remove it. ]
>>
>>> +
>>>   /*
>>>    * Ordering of locks:
>>>    *
>>> @@ -1907,6 +1924,48 @@ void kvm_vcpu_mark_page_dirty(struct kvm_vcpu
>>> *vcpu, gfn_t gfn)
>>>   }
>>>   EXPORT_SYMBOL_GPL(kvm_vcpu_mark_page_dirty);
>>>
>>> +static unsigned int __grow_halt_poll_ns(unsigned int val)
>>> +{
>>> +       if (halt_poll_ns_grow < 1)
>>> +               return halt_poll_ns;
>>> +
>>> +       val = min(val, halt_poll_ns_max);
>>> +
>>> +       if (val == 0)
>>> +               return halt_poll_ns;
>>> +
>>> +       if (halt_poll_ns_grow < halt_poll_ns)
>>> +               val *= halt_poll_ns_grow;
>>> +       else
>>> +               val += halt_poll_ns_grow;
>>> +
>>> +       return val;
>>> +}
>>> +
>>> +static unsigned int __shrink_halt_poll_ns(int val, int modifier, int
>>> minimum)
>>
>> minimum never gets used.
>>
>>> +{
>>> +       if (modifier < 1)
>>> +               return 0;
>>> +
>>> +       if (modifier < halt_poll_ns)
>>> +               val /= modifier;
>>> +       else
>>> +               val -= modifier;
>>> +
>>> +       return val;
>>> +}
>>> +
>>> +static void grow_halt_poll_ns(struct kvm_vcpu *vcpu)
>>
>> These wrappers aren't necessary.
>>
>>> +{
>>> +       vcpu->halt_poll_ns = __grow_halt_poll_ns(vcpu->halt_poll_ns);
>>> +}
>>> +
>>> +static void shrink_halt_poll_ns(struct kvm_vcpu *vcpu)
>>> +{
>>> +       vcpu->halt_poll_ns = __shrink_halt_poll_ns(vcpu->halt_poll_ns,
>>> +                       halt_poll_ns_shrink, halt_poll_ns);
>>> +}
>>> +
>>>   static int kvm_vcpu_check_block(struct kvm_vcpu *vcpu)
>>>   {
>>>          if (kvm_arch_vcpu_runnable(vcpu)) {
>>> @@ -1954,6 +2013,10 @@ void kvm_vcpu_block(struct kvm_vcpu *vcpu)
>>>                          break;
>>>
>>>                  waited = true;
>>> +               if (vcpu->halt_poll_ns > halt_poll_ns_max)
>>> +                       shrink_halt_poll_ns(vcpu);
>>> +               else
>>> +                       grow_halt_poll_ns(vcpu);
>>
>> Shouldn't this go after the loop, and before "out:", in case we schedule
>> more than once? You can gate it on "if (waited)" so it only runs if we
>> actually scheduled.
>>
>>>                  schedule();
>>>          }
>>>
>>> --
>>> 1.9.1
>>>
>

Re: [PATCH v2 2/3] KVM: dynamic halt_poll_ns adjustment

[Wanpeng Li]

On 8/28/15 12:25 AM, David Matlack wrote:
> On Thu, Aug 27, 2015 at 2:59 AM, Wanpeng Li <wanpeng.li@hotmail.com> wrote:
>> Hi David,
>> On 8/26/15 1:19 AM, David Matlack wrote:
>>> Thanks for writing v2, Wanpeng.
>>>
>>> On Mon, Aug 24, 2015 at 11:35 PM, Wanpeng Li <wanpeng.li@hotmail.com>
>>> wrote:
>>>> There is a downside of halt_poll_ns since poll is still happen for idle
>>>> VCPU which can waste cpu usage. This patch adds the ability to adjust
>>>> halt_poll_ns dynamically.
>>> What testing have you done with these patches? Do you know if this removes
>>> the overhead of polling in idle VCPUs? Do we lose any of the performance
>>> from always polling?
>>>
>>>> There are two new kernel parameters for changing the halt_poll_ns:
>>>> halt_poll_ns_grow and halt_poll_ns_shrink. A third new parameter,
>>>> halt_poll_ns_max, controls the maximal halt_poll_ns; it is internally
>>>> rounded down to a closest multiple of halt_poll_ns_grow. The shrink/grow
>>>> matrix is suggested by David:
>>>>
>>>> if (poll successfully for interrupt): stay the same
>>>>     else if (length of kvm_vcpu_block is longer than halt_poll_ns_max):
>>>> shrink
>>>>     else if (length of kvm_vcpu_block is less than halt_poll_ns_max): grow
>>> The way you implemented this wasn't what I expected. I thought you would
>>> time
>>> the whole function (kvm_vcpu_block). But I like your approach better. It's
>>> simpler and [by inspection] does what we want.
>>
>> I see there is more idle vCPUs overhead w/ this method even more than always
>> halt-poll, so I bring back grow vcpu->halt_poll_ns when interrupt arrives
>> and shrinks when idle VCPU is detected. The perfomance looks good in v4.
> Why did this patch have a worse idle overhead than always poll?

I’m not sure. I make a mistake when I report the kernelbuild test, the 
perfomance is also worse than always poll w/ your method. I think your 
method didn't grow halt_poll_ns according to if interrupt arrives.

Regards,
Wanpeng Li

>
>> Regards,
>> Wanpeng Li
>>
>>
>>>>     halt_poll_ns_shrink/ |
>>>>     halt_poll_ns_grow    | grow halt_poll_ns    | shrink halt_poll_ns
>>>>     ---------------------+----------------------+-------------------
>>>>     < 1                  |  = halt_poll_ns      |  = 0
>>>>     < halt_poll_ns       | *= halt_poll_ns_grow | /= halt_poll_ns_shrink
>>>>     otherwise            | += halt_poll_ns_grow | -= halt_poll_ns_shrink
>>> I was curious why you went with this approach rather than just the
>>> middle row, or just the last row. Do you think we'll want the extra
>>> flexibility?
>>>
>>>> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
>>>> ---
>>>>    virt/kvm/kvm_main.c | 65
>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>>>    1 file changed, 64 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>>>> index 93db833..2a4962b 100644
>>>> --- a/virt/kvm/kvm_main.c
>>>> +++ b/virt/kvm/kvm_main.c
>>>> @@ -66,9 +66,26 @@
>>>>    MODULE_AUTHOR("Qumranet");
>>>>    MODULE_LICENSE("GPL");
>>>>
>>>> -static unsigned int halt_poll_ns;
>>>> +#define KVM_HALT_POLL_NS  500000
>>>> +#define KVM_HALT_POLL_NS_GROW   2
>>>> +#define KVM_HALT_POLL_NS_SHRINK 0
>>>> +#define KVM_HALT_POLL_NS_MAX 2000000
>>> The macros are not necessary. Also, hard coding the numbers in the param
>>> definitions will make reading the comments above them easier.
>>>
>>>> +
>>>> +static unsigned int halt_poll_ns = KVM_HALT_POLL_NS;
>>>>    module_param(halt_poll_ns, uint, S_IRUGO | S_IWUSR);
>>>>
>>>> +/* Default doubles per-vcpu halt_poll_ns. */
>>>> +static unsigned int halt_poll_ns_grow = KVM_HALT_POLL_NS_GROW;
>>>> +module_param(halt_poll_ns_grow, int, S_IRUGO);
>>>> +
>>>> +/* Default resets per-vcpu halt_poll_ns . */
>>>> +static unsigned int halt_poll_ns_shrink = KVM_HALT_POLL_NS_SHRINK;
>>>> +module_param(halt_poll_ns_shrink, int, S_IRUGO);
>>>> +
>>>> +/* halt polling only reduces halt latency by 10-15 us, 2ms is enough */
>>> Ah, I misspoke before. I was thinking about round-trip latency. The
>>> latency
>>> of a single halt is reduced by about 5-7 us.
>>>
>>>> +static unsigned int halt_poll_ns_max = KVM_HALT_POLL_NS_MAX;
>>>> +module_param(halt_poll_ns_max, int, S_IRUGO);
>>> We can remove halt_poll_ns_max. vcpu->halt_poll_ns can always start at
>>> zero
>>> and grow from there. Then we just need one module param to keep
>>> vcpu->halt_poll_ns from growing too large.
>>>
>>> [ It would make more sense to remove halt_poll_ns and keep
>>> halt_poll_ns_max,
>>>     but since halt_poll_ns already exists in upstream kernels, we probably
>>> can't
>>>     remove it. ]
>>>
>>>> +
>>>>    /*
>>>>     * Ordering of locks:
>>>>     *
>>>> @@ -1907,6 +1924,48 @@ void kvm_vcpu_mark_page_dirty(struct kvm_vcpu
>>>> *vcpu, gfn_t gfn)
>>>>    }
>>>>    EXPORT_SYMBOL_GPL(kvm_vcpu_mark_page_dirty);
>>>>
>>>> +static unsigned int __grow_halt_poll_ns(unsigned int val)
>>>> +{
>>>> +       if (halt_poll_ns_grow < 1)
>>>> +               return halt_poll_ns;
>>>> +
>>>> +       val = min(val, halt_poll_ns_max);
>>>> +
>>>> +       if (val == 0)
>>>> +               return halt_poll_ns;
>>>> +
>>>> +       if (halt_poll_ns_grow < halt_poll_ns)
>>>> +               val *= halt_poll_ns_grow;
>>>> +       else
>>>> +               val += halt_poll_ns_grow;
>>>> +
>>>> +       return val;
>>>> +}
>>>> +
>>>> +static unsigned int __shrink_halt_poll_ns(int val, int modifier, int
>>>> minimum)
>>> minimum never gets used.
>>>
>>>> +{
>>>> +       if (modifier < 1)
>>>> +               return 0;
>>>> +
>>>> +       if (modifier < halt_poll_ns)
>>>> +               val /= modifier;
>>>> +       else
>>>> +               val -= modifier;
>>>> +
>>>> +       return val;
>>>> +}
>>>> +
>>>> +static void grow_halt_poll_ns(struct kvm_vcpu *vcpu)
>>> These wrappers aren't necessary.
>>>
>>>> +{
>>>> +       vcpu->halt_poll_ns = __grow_halt_poll_ns(vcpu->halt_poll_ns);
>>>> +}
>>>> +
>>>> +static void shrink_halt_poll_ns(struct kvm_vcpu *vcpu)
>>>> +{
>>>> +       vcpu->halt_poll_ns = __shrink_halt_poll_ns(vcpu->halt_poll_ns,
>>>> +                       halt_poll_ns_shrink, halt_poll_ns);
>>>> +}
>>>> +
>>>>    static int kvm_vcpu_check_block(struct kvm_vcpu *vcpu)
>>>>    {
>>>>           if (kvm_arch_vcpu_runnable(vcpu)) {
>>>> @@ -1954,6 +2013,10 @@ void kvm_vcpu_block(struct kvm_vcpu *vcpu)
>>>>                           break;
>>>>
>>>>                   waited = true;
>>>> +               if (vcpu->halt_poll_ns > halt_poll_ns_max)
>>>> +                       shrink_halt_poll_ns(vcpu);
>>>> +               else
>>>> +                       grow_halt_poll_ns(vcpu);
>>> Shouldn't this go after the loop, and before "out:", in case we schedule
>>> more than once? You can gate it on "if (waited)" so it only runs if we
>>> actually scheduled.
>>>
>>>>                   schedule();
>>>>           }
>>>>
>>>> --
>>>> 1.9.1
>>>>