* [PATCH 0/5] misc fixes on halt-poll code both KVM and guest
@ 2019-10-26 3:23 Zhenzhong Duan
2019-10-26 3:23 ` [PATCH 1/5] KVM: simplify branch check in host poll code Zhenzhong Duan
` (4 more replies)
0 siblings, 5 replies; 19+ messages in thread
From: Zhenzhong Duan @ 2019-10-26 3:23 UTC (permalink / raw)
To: linux-kernel
Cc: kvm, mtosatti, joao.m.martins, rafael.j.wysocki, rkrcmar,
pbonzini, Zhenzhong Duan
The last two patches are similar with 2nd and 3rd, but counterpart for guest.
Zhenzhong Duan (5):
KVM: simplify branch check in host poll code
KVM: add a check to ensure grow start value is nonzero
KVM: ensure pool time is longer than block_ns
cpuidle-haltpoll: add a check to ensure grow start value is nonzero
cpuidle-haltpoll: fix up the branch check
drivers/cpuidle/governors/haltpoll.c | 21 +++++++++++++++------
virt/kvm/kvm_main.c | 18 ++++++++++++------
2 files changed, 27 insertions(+), 12 deletions(-)
--
1.8.3.1
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 1/5] KVM: simplify branch check in host poll code
2019-10-26 3:23 [PATCH 0/5] misc fixes on halt-poll code both KVM and guest Zhenzhong Duan
@ 2019-10-26 3:23 ` Zhenzhong Duan
2019-11-01 21:03 ` Marcelo Tosatti
2019-10-26 3:23 ` [PATCH 2/5] KVM: add a check to ensure grow start value is nonzero Zhenzhong Duan
` (3 subsequent siblings)
4 siblings, 1 reply; 19+ messages in thread
From: Zhenzhong Duan @ 2019-10-26 3:23 UTC (permalink / raw)
To: linux-kernel
Cc: kvm, mtosatti, joao.m.martins, rafael.j.wysocki, rkrcmar,
pbonzini, Zhenzhong Duan
Remove redundant check.
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
---
virt/kvm/kvm_main.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 67ef3f2..2ca2979 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2366,13 +2366,12 @@ void kvm_vcpu_block(struct kvm_vcpu *vcpu)
} else if (halt_poll_ns) {
if (block_ns <= vcpu->halt_poll_ns)
;
- /* we had a long block, shrink polling */
- else if (vcpu->halt_poll_ns && block_ns > halt_poll_ns)
- shrink_halt_poll_ns(vcpu);
/* we had a short halt and our poll time is too small */
- else if (vcpu->halt_poll_ns < halt_poll_ns &&
- block_ns < halt_poll_ns)
+ else if (block_ns < halt_poll_ns)
grow_halt_poll_ns(vcpu);
+ /* we had a long block, shrink polling */
+ else if (vcpu->halt_poll_ns)
+ shrink_halt_poll_ns(vcpu);
} else {
vcpu->halt_poll_ns = 0;
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 2/5] KVM: add a check to ensure grow start value is nonzero
2019-10-26 3:23 [PATCH 0/5] misc fixes on halt-poll code both KVM and guest Zhenzhong Duan
2019-10-26 3:23 ` [PATCH 1/5] KVM: simplify branch check in host poll code Zhenzhong Duan
@ 2019-10-26 3:23 ` Zhenzhong Duan
2019-11-11 13:49 ` Paolo Bonzini
2019-10-26 3:23 ` [PATCH 3/5] KVM: ensure pool time is longer than block_ns Zhenzhong Duan
` (2 subsequent siblings)
4 siblings, 1 reply; 19+ messages in thread
From: Zhenzhong Duan @ 2019-10-26 3:23 UTC (permalink / raw)
To: linux-kernel
Cc: kvm, mtosatti, joao.m.martins, rafael.j.wysocki, rkrcmar,
pbonzini, Zhenzhong Duan
vcpu->halt_poll_ns could be zeroed in certain cases (e.g. by
halt_poll_ns_shrink). If halt_poll_ns_grow_start is zero,
vcpu->halt_poll_ns will never be larger than zero.
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
---
virt/kvm/kvm_main.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 2ca2979..1b6fe3b 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2266,6 +2266,13 @@ static void grow_halt_poll_ns(struct kvm_vcpu *vcpu)
goto out;
val *= grow;
+
+ /*
+ * vcpu->halt_poll_ns needs a nonzero start point to grow if it's zero.
+ */
+ if (!grow_start)
+ grow_start = 1;
+
if (val < grow_start)
val = grow_start;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 3/5] KVM: ensure pool time is longer than block_ns
2019-10-26 3:23 [PATCH 0/5] misc fixes on halt-poll code both KVM and guest Zhenzhong Duan
2019-10-26 3:23 ` [PATCH 1/5] KVM: simplify branch check in host poll code Zhenzhong Duan
2019-10-26 3:23 ` [PATCH 2/5] KVM: add a check to ensure grow start value is nonzero Zhenzhong Duan
@ 2019-10-26 3:23 ` Zhenzhong Duan
2019-11-01 21:16 ` Marcelo Tosatti
2019-10-26 3:23 ` [PATCH 4/5] cpuidle-haltpoll: add a check to ensure grow start value is nonzero Zhenzhong Duan
2019-10-26 3:23 ` [PATCH 5/5] cpuidle-haltpoll: fix up the branch check Zhenzhong Duan
4 siblings, 1 reply; 19+ messages in thread
From: Zhenzhong Duan @ 2019-10-26 3:23 UTC (permalink / raw)
To: linux-kernel
Cc: kvm, mtosatti, joao.m.martins, rafael.j.wysocki, rkrcmar,
pbonzini, Zhenzhong Duan
When (block_ns == vcpu->halt_poll_ns), there is not a margin so that
vCPU may still get into block state unnecessorily.
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
---
virt/kvm/kvm_main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 1b6fe3b..48a1f1a 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2371,7 +2371,7 @@ void kvm_vcpu_block(struct kvm_vcpu *vcpu)
if (!vcpu_valid_wakeup(vcpu)) {
shrink_halt_poll_ns(vcpu);
} else if (halt_poll_ns) {
- if (block_ns <= vcpu->halt_poll_ns)
+ if (block_ns < vcpu->halt_poll_ns)
;
/* we had a short halt and our poll time is too small */
else if (block_ns < halt_poll_ns)
--
1.8.3.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 4/5] cpuidle-haltpoll: add a check to ensure grow start value is nonzero
2019-10-26 3:23 [PATCH 0/5] misc fixes on halt-poll code both KVM and guest Zhenzhong Duan
` (2 preceding siblings ...)
2019-10-26 3:23 ` [PATCH 3/5] KVM: ensure pool time is longer than block_ns Zhenzhong Duan
@ 2019-10-26 3:23 ` Zhenzhong Duan
2019-11-01 21:19 ` Marcelo Tosatti
2019-10-26 3:23 ` [PATCH 5/5] cpuidle-haltpoll: fix up the branch check Zhenzhong Duan
4 siblings, 1 reply; 19+ messages in thread
From: Zhenzhong Duan @ 2019-10-26 3:23 UTC (permalink / raw)
To: linux-kernel
Cc: kvm, mtosatti, joao.m.martins, rafael.j.wysocki, rkrcmar,
pbonzini, Zhenzhong Duan
dev->poll_limit_ns could be zeroed in certain cases (e.g. by
guest_halt_poll_shrink). If guest_halt_poll_grow_start is zero,
dev->poll_limit_ns will never be larger than zero.
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
---
drivers/cpuidle/governors/haltpoll.c | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/cpuidle/governors/haltpoll.c b/drivers/cpuidle/governors/haltpoll.c
index 7a703d2..4b00d7a 100644
--- a/drivers/cpuidle/governors/haltpoll.c
+++ b/drivers/cpuidle/governors/haltpoll.c
@@ -77,7 +77,7 @@ static int haltpoll_select(struct cpuidle_driver *drv,
static void adjust_poll_limit(struct cpuidle_device *dev, unsigned int block_us)
{
- unsigned int val;
+ unsigned int val, grow_start;
u64 block_ns = block_us*NSEC_PER_USEC;
/* Grow cpu_halt_poll_us if
@@ -86,8 +86,17 @@ static void adjust_poll_limit(struct cpuidle_device *dev, unsigned int block_us)
if (block_ns > dev->poll_limit_ns && block_ns <= guest_halt_poll_ns) {
val = dev->poll_limit_ns * guest_halt_poll_grow;
- if (val < guest_halt_poll_grow_start)
- val = guest_halt_poll_grow_start;
+ /*
+ * vcpu->halt_poll_ns needs a nonzero start point to grow if
+ * it's zero.
+ */
+ grow_start = guest_halt_poll_grow_start;
+ if (!grow_start)
+ grow_start = 1;
+
+ if (val < grow_start)
+ val = grow_start;
+
if (val > guest_halt_poll_ns)
val = guest_halt_poll_ns;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 5/5] cpuidle-haltpoll: fix up the branch check
2019-10-26 3:23 [PATCH 0/5] misc fixes on halt-poll code both KVM and guest Zhenzhong Duan
` (3 preceding siblings ...)
2019-10-26 3:23 ` [PATCH 4/5] cpuidle-haltpoll: add a check to ensure grow start value is nonzero Zhenzhong Duan
@ 2019-10-26 3:23 ` Zhenzhong Duan
2019-11-01 21:26 ` Marcelo Tosatti
4 siblings, 1 reply; 19+ messages in thread
From: Zhenzhong Duan @ 2019-10-26 3:23 UTC (permalink / raw)
To: linux-kernel
Cc: kvm, mtosatti, joao.m.martins, rafael.j.wysocki, rkrcmar,
pbonzini, Zhenzhong Duan
Ensure pool time is longer than block_ns, so there is a margin to
avoid vCPU get into block state unnecessorily.
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
---
drivers/cpuidle/governors/haltpoll.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/cpuidle/governors/haltpoll.c b/drivers/cpuidle/governors/haltpoll.c
index 4b00d7a..59eadaf 100644
--- a/drivers/cpuidle/governors/haltpoll.c
+++ b/drivers/cpuidle/governors/haltpoll.c
@@ -81,9 +81,9 @@ static void adjust_poll_limit(struct cpuidle_device *dev, unsigned int block_us)
u64 block_ns = block_us*NSEC_PER_USEC;
/* Grow cpu_halt_poll_us if
- * cpu_halt_poll_us < block_ns < guest_halt_poll_us
+ * cpu_halt_poll_us <= block_ns < guest_halt_poll_us
*/
- if (block_ns > dev->poll_limit_ns && block_ns <= guest_halt_poll_ns) {
+ if (block_ns >= dev->poll_limit_ns && block_ns < guest_halt_poll_ns) {
val = dev->poll_limit_ns * guest_halt_poll_grow;
/*
@@ -101,7 +101,7 @@ static void adjust_poll_limit(struct cpuidle_device *dev, unsigned int block_us)
val = guest_halt_poll_ns;
dev->poll_limit_ns = val;
- } else if (block_ns > guest_halt_poll_ns &&
+ } else if (block_ns >= guest_halt_poll_ns &&
guest_halt_poll_allow_shrink) {
unsigned int shrink = guest_halt_poll_shrink;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 1/5] KVM: simplify branch check in host poll code
2019-10-26 3:23 ` [PATCH 1/5] KVM: simplify branch check in host poll code Zhenzhong Duan
@ 2019-11-01 21:03 ` Marcelo Tosatti
2019-11-04 3:49 ` Zhenzhong Duan
0 siblings, 1 reply; 19+ messages in thread
From: Marcelo Tosatti @ 2019-11-01 21:03 UTC (permalink / raw)
To: Zhenzhong Duan
Cc: linux-kernel, kvm, joao.m.martins, rafael.j.wysocki, rkrcmar, pbonzini
On Sat, Oct 26, 2019 at 11:23:55AM +0800, Zhenzhong Duan wrote:
> Remove redundant check.
>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
> ---
> virt/kvm/kvm_main.c | 9 ++++-----
> 1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 67ef3f2..2ca2979 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -2366,13 +2366,12 @@ void kvm_vcpu_block(struct kvm_vcpu *vcpu)
> } else if (halt_poll_ns) {
> if (block_ns <= vcpu->halt_poll_ns)
> ;
> - /* we had a long block, shrink polling */
> - else if (vcpu->halt_poll_ns && block_ns > halt_poll_ns)
> - shrink_halt_poll_ns(vcpu);
> /* we had a short halt and our poll time is too small */
> - else if (vcpu->halt_poll_ns < halt_poll_ns &&
This is not a redundant check: it avoids from calling
into grow_halt_poll_ns, which will do:
1) Multiplication
2) Cap that back to halt_poll_ns
3) Invoke the trace_kvm_halt_poll_ns_grow tracepoint
(when in fact vcpu->halt_poll_ns did not grow).
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/5] KVM: ensure pool time is longer than block_ns
2019-10-26 3:23 ` [PATCH 3/5] KVM: ensure pool time is longer than block_ns Zhenzhong Duan
@ 2019-11-01 21:16 ` Marcelo Tosatti
2019-11-11 13:53 ` Paolo Bonzini
0 siblings, 1 reply; 19+ messages in thread
From: Marcelo Tosatti @ 2019-11-01 21:16 UTC (permalink / raw)
To: Zhenzhong Duan
Cc: linux-kernel, kvm, joao.m.martins, rafael.j.wysocki, rkrcmar, pbonzini
On Sat, Oct 26, 2019 at 11:23:57AM +0800, Zhenzhong Duan wrote:
> When (block_ns == vcpu->halt_poll_ns), there is not a margin so that
> vCPU may still get into block state unnecessorily.
>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
> ---
> virt/kvm/kvm_main.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 1b6fe3b..48a1f1a 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -2371,7 +2371,7 @@ void kvm_vcpu_block(struct kvm_vcpu *vcpu)
> if (!vcpu_valid_wakeup(vcpu)) {
> shrink_halt_poll_ns(vcpu);
> } else if (halt_poll_ns) {
> - if (block_ns <= vcpu->halt_poll_ns)
> + if (block_ns < vcpu->halt_poll_ns)
> ;
> /* we had a short halt and our poll time is too small */
> else if (block_ns < halt_poll_ns)
> --
> 1.8.3.1
Makes sense.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 4/5] cpuidle-haltpoll: add a check to ensure grow start value is nonzero
2019-10-26 3:23 ` [PATCH 4/5] cpuidle-haltpoll: add a check to ensure grow start value is nonzero Zhenzhong Duan
@ 2019-11-01 21:19 ` Marcelo Tosatti
2019-11-04 2:56 ` Zhenzhong Duan
0 siblings, 1 reply; 19+ messages in thread
From: Marcelo Tosatti @ 2019-11-01 21:19 UTC (permalink / raw)
To: Zhenzhong Duan
Cc: linux-kernel, kvm, joao.m.martins, rafael.j.wysocki, rkrcmar, pbonzini
On Sat, Oct 26, 2019 at 11:23:58AM +0800, Zhenzhong Duan wrote:
> dev->poll_limit_ns could be zeroed in certain cases (e.g. by
> guest_halt_poll_shrink). If guest_halt_poll_grow_start is zero,
> dev->poll_limit_ns will never be larger than zero.
>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
> ---
> drivers/cpuidle/governors/haltpoll.c | 15 ++++++++++++---
> 1 file changed, 12 insertions(+), 3 deletions(-)
I would rather disallow setting grow_start to zero rather
than silently setting it to one on the back of the user.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 5/5] cpuidle-haltpoll: fix up the branch check
2019-10-26 3:23 ` [PATCH 5/5] cpuidle-haltpoll: fix up the branch check Zhenzhong Duan
@ 2019-11-01 21:26 ` Marcelo Tosatti
2019-11-04 3:10 ` Zhenzhong Duan
0 siblings, 1 reply; 19+ messages in thread
From: Marcelo Tosatti @ 2019-11-01 21:26 UTC (permalink / raw)
To: Zhenzhong Duan
Cc: linux-kernel, kvm, joao.m.martins, rafael.j.wysocki, rkrcmar, pbonzini
On Sat, Oct 26, 2019 at 11:23:59AM +0800, Zhenzhong Duan wrote:
> Ensure pool time is longer than block_ns, so there is a margin to
> avoid vCPU get into block state unnecessorily.
>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
> ---
> drivers/cpuidle/governors/haltpoll.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/cpuidle/governors/haltpoll.c b/drivers/cpuidle/governors/haltpoll.c
> index 4b00d7a..59eadaf 100644
> --- a/drivers/cpuidle/governors/haltpoll.c
> +++ b/drivers/cpuidle/governors/haltpoll.c
> @@ -81,9 +81,9 @@ static void adjust_poll_limit(struct cpuidle_device *dev, unsigned int block_us)
> u64 block_ns = block_us*NSEC_PER_USEC;
>
> /* Grow cpu_halt_poll_us if
> - * cpu_halt_poll_us < block_ns < guest_halt_poll_us
> + * cpu_halt_poll_us <= block_ns < guest_halt_poll_us
> */
> - if (block_ns > dev->poll_limit_ns && block_ns <= guest_halt_poll_ns) {
> + if (block_ns >= dev->poll_limit_ns && block_ns < guest_halt_poll_ns) {
^^^^^^^^^^^^^^^^^^^^^^^^^^^^
If block_ns == guest_halt_poll_ns, you won't allow dev->poll_limit_ns to
grow. Why is that?
> @@ -101,7 +101,7 @@ static void adjust_poll_limit(struct cpuidle_device *dev, unsigned int block_us)
> val = guest_halt_poll_ns;
>
> dev->poll_limit_ns = val;
> - } else if (block_ns > guest_halt_poll_ns &&
> + } else if (block_ns >= guest_halt_poll_ns &&
> guest_halt_poll_allow_shrink) {
> unsigned int shrink = guest_halt_poll_shrink;
And here you shrink if block_ns == guest_halt_poll_ns. Not sure
why that makes sense either.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 4/5] cpuidle-haltpoll: add a check to ensure grow start value is nonzero
2019-11-01 21:19 ` Marcelo Tosatti
@ 2019-11-04 2:56 ` Zhenzhong Duan
2019-11-11 13:54 ` Paolo Bonzini
0 siblings, 1 reply; 19+ messages in thread
From: Zhenzhong Duan @ 2019-11-04 2:56 UTC (permalink / raw)
To: Marcelo Tosatti
Cc: linux-kernel, kvm, joao.m.martins, rafael.j.wysocki, rkrcmar, pbonzini
On 2019/11/2 5:19, Marcelo Tosatti wrote:
> On Sat, Oct 26, 2019 at 11:23:58AM +0800, Zhenzhong Duan wrote:
>> dev->poll_limit_ns could be zeroed in certain cases (e.g. by
>> guest_halt_poll_shrink). If guest_halt_poll_grow_start is zero,
>> dev->poll_limit_ns will never be larger than zero.
>>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
>> ---
>> drivers/cpuidle/governors/haltpoll.c | 15 ++++++++++++---
>> 1 file changed, 12 insertions(+), 3 deletions(-)
> I would rather disallow setting grow_start to zero rather
> than silently setting it to one on the back of the user.
Ok, will do.
Thanks
Zhenzhong
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 5/5] cpuidle-haltpoll: fix up the branch check
2019-11-01 21:26 ` Marcelo Tosatti
@ 2019-11-04 3:10 ` Zhenzhong Duan
2019-11-04 15:01 ` Marcelo Tosatti
0 siblings, 1 reply; 19+ messages in thread
From: Zhenzhong Duan @ 2019-11-04 3:10 UTC (permalink / raw)
To: Marcelo Tosatti
Cc: linux-kernel, kvm, joao.m.martins, rafael.j.wysocki, rkrcmar, pbonzini
On 2019/11/2 5:26, Marcelo Tosatti wrote:
> On Sat, Oct 26, 2019 at 11:23:59AM +0800, Zhenzhong Duan wrote:
>> Ensure pool time is longer than block_ns, so there is a margin to
>> avoid vCPU get into block state unnecessorily.
>>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
>> ---
>> drivers/cpuidle/governors/haltpoll.c | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/cpuidle/governors/haltpoll.c b/drivers/cpuidle/governors/haltpoll.c
>> index 4b00d7a..59eadaf 100644
>> --- a/drivers/cpuidle/governors/haltpoll.c
>> +++ b/drivers/cpuidle/governors/haltpoll.c
>> @@ -81,9 +81,9 @@ static void adjust_poll_limit(struct cpuidle_device *dev, unsigned int block_us)
>> u64 block_ns = block_us*NSEC_PER_USEC;
>>
>> /* Grow cpu_halt_poll_us if
>> - * cpu_halt_poll_us < block_ns < guest_halt_poll_us
>> + * cpu_halt_poll_us <= block_ns < guest_halt_poll_us
>> */
>> - if (block_ns > dev->poll_limit_ns && block_ns <= guest_halt_poll_ns) {
>> + if (block_ns >= dev->poll_limit_ns && block_ns < guest_halt_poll_ns) {
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> If block_ns == guest_halt_poll_ns, you won't allow dev->poll_limit_ns to
> grow. Why is that?
Maybe I'm too strict here. My understanding is: if block_ns = guest_halt_poll_ns,
dev->poll_limit_ns will grow to guest_halt_poll_ns, then block_ns = dev->poll_limit_ns,
there is not a margin to ensure poll time is enough to cover the equal block time.
In this case, shrinking may be a better choice?
>
>> @@ -101,7 +101,7 @@ static void adjust_poll_limit(struct cpuidle_device *dev, unsigned int block_us)
>> val = guest_halt_poll_ns;
>>
>> dev->poll_limit_ns = val;
>> - } else if (block_ns > guest_halt_poll_ns &&
>> + } else if (block_ns >= guest_halt_poll_ns &&
>> guest_halt_poll_allow_shrink) {
>> unsigned int shrink = guest_halt_poll_shrink;
> And here you shrink if block_ns == guest_halt_poll_ns. Not sure
> why that makes sense either.
See above explanation.
Zhenzhong
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/5] KVM: simplify branch check in host poll code
2019-11-01 21:03 ` Marcelo Tosatti
@ 2019-11-04 3:49 ` Zhenzhong Duan
0 siblings, 0 replies; 19+ messages in thread
From: Zhenzhong Duan @ 2019-11-04 3:49 UTC (permalink / raw)
To: Marcelo Tosatti
Cc: linux-kernel, kvm, joao.m.martins, rafael.j.wysocki, rkrcmar, pbonzini
On 2019/11/2 5:03, Marcelo Tosatti wrote:
> On Sat, Oct 26, 2019 at 11:23:55AM +0800, Zhenzhong Duan wrote:
>> Remove redundant check.
>>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
>> ---
>> virt/kvm/kvm_main.c | 9 ++++-----
>> 1 file changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>> index 67ef3f2..2ca2979 100644
>> --- a/virt/kvm/kvm_main.c
>> +++ b/virt/kvm/kvm_main.c
>> @@ -2366,13 +2366,12 @@ void kvm_vcpu_block(struct kvm_vcpu *vcpu)
>> } else if (halt_poll_ns) {
>> if (block_ns <= vcpu->halt_poll_ns)
>> ;
>> - /* we had a long block, shrink polling */
>> - else if (vcpu->halt_poll_ns && block_ns > halt_poll_ns)
>> - shrink_halt_poll_ns(vcpu);
>> /* we had a short halt and our poll time is too small */
>> - else if (vcpu->halt_poll_ns < halt_poll_ns &&
> This is not a redundant check: it avoids from calling
> into grow_halt_poll_ns, which will do:
>
> 1) Multiplication
> 2) Cap that back to halt_poll_ns
> 3) Invoke the trace_kvm_halt_poll_ns_grow tracepoint
> (when in fact vcpu->halt_poll_ns did not grow).
In this branch, vcpu->halt_poll_ns < block_ns is true, and if block_ns <
halt_poll_ns,
then vcpu->halt_poll_ns < halt_poll_ns is always true, isn't it?
I realized I ignored the situation that halt_poll_ns and
halt_poll_ns_grow could be
updated at runtime, so pls ignore this patch, I'll fix it by following
the guest haltpoll code.
Thanks
Zhenzhong
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 5/5] cpuidle-haltpoll: fix up the branch check
2019-11-04 3:10 ` Zhenzhong Duan
@ 2019-11-04 15:01 ` Marcelo Tosatti
2019-11-05 6:49 ` Zhenzhong Duan
0 siblings, 1 reply; 19+ messages in thread
From: Marcelo Tosatti @ 2019-11-04 15:01 UTC (permalink / raw)
To: Zhenzhong Duan
Cc: linux-kernel, kvm, joao.m.martins, rafael.j.wysocki, rkrcmar, pbonzini
On Mon, Nov 04, 2019 at 11:10:25AM +0800, Zhenzhong Duan wrote:
>
> On 2019/11/2 5:26, Marcelo Tosatti wrote:
> >On Sat, Oct 26, 2019 at 11:23:59AM +0800, Zhenzhong Duan wrote:
> >>Ensure pool time is longer than block_ns, so there is a margin to
> >>avoid vCPU get into block state unnecessorily.
> >>
> >>Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
> >>---
> >> drivers/cpuidle/governors/haltpoll.c | 6 +++---
> >> 1 file changed, 3 insertions(+), 3 deletions(-)
> >>
> >>diff --git a/drivers/cpuidle/governors/haltpoll.c b/drivers/cpuidle/governors/haltpoll.c
> >>index 4b00d7a..59eadaf 100644
> >>--- a/drivers/cpuidle/governors/haltpoll.c
> >>+++ b/drivers/cpuidle/governors/haltpoll.c
> >>@@ -81,9 +81,9 @@ static void adjust_poll_limit(struct cpuidle_device *dev, unsigned int block_us)
> >> u64 block_ns = block_us*NSEC_PER_USEC;
> >> /* Grow cpu_halt_poll_us if
> >>- * cpu_halt_poll_us < block_ns < guest_halt_poll_us
> >>+ * cpu_halt_poll_us <= block_ns < guest_halt_poll_us
> >> */
> >>- if (block_ns > dev->poll_limit_ns && block_ns <= guest_halt_poll_ns) {
> >>+ if (block_ns >= dev->poll_limit_ns && block_ns < guest_halt_poll_ns) {
> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >
> >If block_ns == guest_halt_poll_ns, you won't allow dev->poll_limit_ns to
> >grow. Why is that?
>
> Maybe I'm too strict here. My understanding is: if block_ns = guest_halt_poll_ns,
> dev->poll_limit_ns will grow to guest_halt_poll_ns,
OK.
> then block_ns = dev->poll_limit_ns,
block_ns = dev->poll_limit_ns = guest_halt_poll_ns. OK.
> there is not a margin to ensure poll time is enough to cover the equal block time.
> In this case, shrinking may be a better choice?
Ok, so you are considering _on the next_ halt instance, if block_ns =
guest_halt_poll_ns again?
Then without the suggested modification: we don't shrink, poll for
guest_halt_poll_ns again.
With your modification: we shrink, because block_ns ==
guest_halt_poll_ns.
IMO what really clarifies things here is either the real sleep pattern
or a synthetic sleep pattern similar to the real thing.
Do you have a scenario where the current algorithm is maintaining
a low dev->poll_limit_ns and performance is hurt?
If you could come up with examples, such as the client/server pair at
https://lore.kernel.org/lkml/20190514135022.GD4392@amt.cnet/T/
or just a sequence of delays:
block_ns, block_ns, block_ns-1,...
It would be easier to visualize this.
> >>@@ -101,7 +101,7 @@ static void adjust_poll_limit(struct cpuidle_device *dev, unsigned int block_us)
> >> val = guest_halt_poll_ns;
> >> dev->poll_limit_ns = val;
> >>- } else if (block_ns > guest_halt_poll_ns &&
> >>+ } else if (block_ns >= guest_halt_poll_ns &&
> >> guest_halt_poll_allow_shrink) {
> >> unsigned int shrink = guest_halt_poll_shrink;
> >And here you shrink if block_ns == guest_halt_poll_ns. Not sure
> >why that makes sense either.
>
> See above explanation.
>
> Zhenzhong
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 5/5] cpuidle-haltpoll: fix up the branch check
2019-11-04 15:01 ` Marcelo Tosatti
@ 2019-11-05 6:49 ` Zhenzhong Duan
0 siblings, 0 replies; 19+ messages in thread
From: Zhenzhong Duan @ 2019-11-05 6:49 UTC (permalink / raw)
To: Marcelo Tosatti
Cc: linux-kernel, kvm, joao.m.martins, rafael.j.wysocki, rkrcmar, pbonzini
On 2019/11/4 23:01, Marcelo Tosatti wrote:
> On Mon, Nov 04, 2019 at 11:10:25AM +0800, Zhenzhong Duan wrote:
>> On 2019/11/2 5:26, Marcelo Tosatti wrote:
>>> On Sat, Oct 26, 2019 at 11:23:59AM +0800, Zhenzhong Duan wrote:
>>>> Ensure pool time is longer than block_ns, so there is a margin to
>>>> avoid vCPU get into block state unnecessorily.
>>>>
>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
>>>> ---
>>>> drivers/cpuidle/governors/haltpoll.c | 6 +++---
>>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/cpuidle/governors/haltpoll.c b/drivers/cpuidle/governors/haltpoll.c
>>>> index 4b00d7a..59eadaf 100644
>>>> --- a/drivers/cpuidle/governors/haltpoll.c
>>>> +++ b/drivers/cpuidle/governors/haltpoll.c
>>>> @@ -81,9 +81,9 @@ static void adjust_poll_limit(struct cpuidle_device *dev, unsigned int block_us)
>>>> u64 block_ns = block_us*NSEC_PER_USEC;
>>>> /* Grow cpu_halt_poll_us if
>>>> - * cpu_halt_poll_us < block_ns < guest_halt_poll_us
>>>> + * cpu_halt_poll_us <= block_ns < guest_halt_poll_us
>>>> */
>>>> - if (block_ns > dev->poll_limit_ns && block_ns <= guest_halt_poll_ns) {
>>>> + if (block_ns >= dev->poll_limit_ns && block_ns < guest_halt_poll_ns) {
>>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>>
>>> If block_ns == guest_halt_poll_ns, you won't allow dev->poll_limit_ns to
>>> grow. Why is that?
>> Maybe I'm too strict here. My understanding is: if block_ns = guest_halt_poll_ns,
>> dev->poll_limit_ns will grow to guest_halt_poll_ns,
> OK.
>
>> then block_ns = dev->poll_limit_ns,
> block_ns = dev->poll_limit_ns = guest_halt_poll_ns. OK.
>
>> there is not a margin to ensure poll time is enough to cover the equal block time.
>> In this case, shrinking may be a better choice?
> Ok, so you are considering _on the next_ halt instance, if block_ns =
> guest_halt_poll_ns again?
Yes, I realized it's rarely to happen in nanosecond granularity.
>
> Then without the suggested modification: we don't shrink, poll for
> guest_halt_poll_ns again.
>
> With your modification: we shrink, because block_ns ==
> guest_halt_poll_ns.
>
> IMO what really clarifies things here is either the real sleep pattern
> or a synthetic sleep pattern similar to the real thing.
Agree
>
> Do you have a scenario where the current algorithm is maintaining
> a low dev->poll_limit_ns and performance is hurt?
>
> If you could come up with examples, such as the client/server pair at
> https://lore.kernel.org/lkml/20190514135022.GD4392@amt.cnet/T/
>
> or just a sequence of delays:
> block_ns, block_ns, block_ns-1,...
>
> It would be easier to visualize this.
Looks hard to generate a sequence of delays of same value in nanoseconds
which is also CPU cycle granularity.
I think this patch doesn't help much for a real scenario, so pls ignore it.
Thanks
Zhenzhong
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/5] KVM: add a check to ensure grow start value is nonzero
2019-10-26 3:23 ` [PATCH 2/5] KVM: add a check to ensure grow start value is nonzero Zhenzhong Duan
@ 2019-11-11 13:49 ` Paolo Bonzini
0 siblings, 0 replies; 19+ messages in thread
From: Paolo Bonzini @ 2019-11-11 13:49 UTC (permalink / raw)
To: Zhenzhong Duan, linux-kernel
Cc: kvm, mtosatti, joao.m.martins, rafael.j.wysocki, rkrcmar
On 26/10/19 05:23, Zhenzhong Duan wrote:
> vcpu->halt_poll_ns could be zeroed in certain cases (e.g. by
> halt_poll_ns_shrink). If halt_poll_ns_grow_start is zero,
> vcpu->halt_poll_ns will never be larger than zero.
>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
> ---
> virt/kvm/kvm_main.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 2ca2979..1b6fe3b 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -2266,6 +2266,13 @@ static void grow_halt_poll_ns(struct kvm_vcpu *vcpu)
> goto out;
>
> val *= grow;
> +
> + /*
> + * vcpu->halt_poll_ns needs a nonzero start point to grow if it's zero.
> + */
> + if (!grow_start)
> + grow_start = 1;
> +
> if (val < grow_start)
> val = grow_start;
>
>
Zeroing grow_start will simply disable halt polling. Is that a problem?
Paolo
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/5] KVM: ensure pool time is longer than block_ns
2019-11-01 21:16 ` Marcelo Tosatti
@ 2019-11-11 13:53 ` Paolo Bonzini
2019-11-12 12:14 ` Zhenzhong Duan
0 siblings, 1 reply; 19+ messages in thread
From: Paolo Bonzini @ 2019-11-11 13:53 UTC (permalink / raw)
To: Marcelo Tosatti, Zhenzhong Duan
Cc: linux-kernel, kvm, joao.m.martins, rafael.j.wysocki, rkrcmar
On 01/11/19 22:16, Marcelo Tosatti wrote:
> if (!vcpu_valid_wakeup(vcpu)) {
> shrink_halt_poll_ns(vcpu);
> } else if (halt_poll_ns) {
> - if (block_ns <= vcpu->halt_poll_ns)
> + if (block_ns < vcpu->halt_poll_ns)
> ;
> /* we had a short halt and our poll time is too small */
> else if (block_ns < halt_poll_ns)
What about making this "if (!waited)"? The result would be very readable:
if (!waited)
;
/* we had a long block, shrink polling */
else if (block_ns > halt_poll_ns && vcpu->halt_poll_ns)
shrink_halt_poll_ns(vcpu);
/* we had a short halt and our poll time is too small */
else if (block_ns < halt_poll_ns && vcpu->halt_poll_ns < halt_poll_ns)
grow_halt_poll_ns(vcpu);
Paolo
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 4/5] cpuidle-haltpoll: add a check to ensure grow start value is nonzero
2019-11-04 2:56 ` Zhenzhong Duan
@ 2019-11-11 13:54 ` Paolo Bonzini
0 siblings, 0 replies; 19+ messages in thread
From: Paolo Bonzini @ 2019-11-11 13:54 UTC (permalink / raw)
To: Zhenzhong Duan, Marcelo Tosatti
Cc: linux-kernel, kvm, joao.m.martins, rafael.j.wysocki, rkrcmar
On 04/11/19 03:56, Zhenzhong Duan wrote:
>
> On 2019/11/2 5:19, Marcelo Tosatti wrote:
>> On Sat, Oct 26, 2019 at 11:23:58AM +0800, Zhenzhong Duan wrote:
>>> dev->poll_limit_ns could be zeroed in certain cases (e.g. by
>>> guest_halt_poll_shrink). If guest_halt_poll_grow_start is zero,
>>> dev->poll_limit_ns will never be larger than zero.
>>>
>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
>>> ---
>>> drivers/cpuidle/governors/haltpoll.c | 15 ++++++++++++---
>>> 1 file changed, 12 insertions(+), 3 deletions(-)
>> I would rather disallow setting grow_start to zero rather
>> than silently setting it to one on the back of the user.
>
> Ok, will do.
Similar to patch 2, I think disabling halt polling is a good behavior if
grow_start = 0.
Paolo
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/5] KVM: ensure pool time is longer than block_ns
2019-11-11 13:53 ` Paolo Bonzini
@ 2019-11-12 12:14 ` Zhenzhong Duan
0 siblings, 0 replies; 19+ messages in thread
From: Zhenzhong Duan @ 2019-11-12 12:14 UTC (permalink / raw)
To: Paolo Bonzini, Marcelo Tosatti
Cc: linux-kernel, kvm, joao.m.martins, rafael.j.wysocki, rkrcmar
On 2019/11/11 21:53, Paolo Bonzini wrote:
> On 01/11/19 22:16, Marcelo Tosatti wrote:
>> if (!vcpu_valid_wakeup(vcpu)) {
>> shrink_halt_poll_ns(vcpu);
>> } else if (halt_poll_ns) {
>> - if (block_ns <= vcpu->halt_poll_ns)
>> + if (block_ns < vcpu->halt_poll_ns)
>> ;
>> /* we had a short halt and our poll time is too small */
>> else if (block_ns < halt_poll_ns)
> What about making this "if (!waited)"? The result would be very readable:
>
> if (!waited)
> ;
> /* we had a long block, shrink polling */
> else if (block_ns > halt_poll_ns && vcpu->halt_poll_ns)
> shrink_halt_poll_ns(vcpu);
> /* we had a short halt and our poll time is too small */
> else if (block_ns < halt_poll_ns && vcpu->halt_poll_ns < halt_poll_ns)
> grow_halt_poll_ns(vcpu);
This patch is dropped in v2 as it rarely happen in real scenario.
Appreciate you reviewing v2 in https://lkml.org/lkml/2019/11/6/447
Thanks
Zhenzhong
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2019-11-12 12:14 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-26 3:23 [PATCH 0/5] misc fixes on halt-poll code both KVM and guest Zhenzhong Duan
2019-10-26 3:23 ` [PATCH 1/5] KVM: simplify branch check in host poll code Zhenzhong Duan
2019-11-01 21:03 ` Marcelo Tosatti
2019-11-04 3:49 ` Zhenzhong Duan
2019-10-26 3:23 ` [PATCH 2/5] KVM: add a check to ensure grow start value is nonzero Zhenzhong Duan
2019-11-11 13:49 ` Paolo Bonzini
2019-10-26 3:23 ` [PATCH 3/5] KVM: ensure pool time is longer than block_ns Zhenzhong Duan
2019-11-01 21:16 ` Marcelo Tosatti
2019-11-11 13:53 ` Paolo Bonzini
2019-11-12 12:14 ` Zhenzhong Duan
2019-10-26 3:23 ` [PATCH 4/5] cpuidle-haltpoll: add a check to ensure grow start value is nonzero Zhenzhong Duan
2019-11-01 21:19 ` Marcelo Tosatti
2019-11-04 2:56 ` Zhenzhong Duan
2019-11-11 13:54 ` Paolo Bonzini
2019-10-26 3:23 ` [PATCH 5/5] cpuidle-haltpoll: fix up the branch check Zhenzhong Duan
2019-11-01 21:26 ` Marcelo Tosatti
2019-11-04 3:10 ` Zhenzhong Duan
2019-11-04 15:01 ` Marcelo Tosatti
2019-11-05 6:49 ` Zhenzhong Duan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).