[PATCH 0/5] misc fixes on halt-poll code both KVM and guest

[PATCH 0/5] misc fixes on halt-poll code both KVM and guest

[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

[PATCH 1/5] KVM: simplify branch check in host poll code

[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

Re: [PATCH 1/5] KVM: simplify branch check in host poll code

[Marcelo Tosatti]

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).

Re: [PATCH 1/5] KVM: simplify branch check in host poll code

[Zhenzhong Duan]

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

[PATCH 2/5] KVM: add a check to ensure grow start value is nonzero

[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

Re: [PATCH 2/5] KVM: add a check to ensure grow start value is nonzero

[Paolo Bonzini]

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

[PATCH 3/5] KVM: ensure pool time is longer than block_ns

[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

Re: [PATCH 3/5] KVM: ensure pool time is longer than block_ns

[Marcelo Tosatti]

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.

Re: [PATCH 3/5] KVM: ensure pool time is longer than block_ns

[Paolo Bonzini]

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

Re: [PATCH 3/5] KVM: ensure pool time is longer than block_ns

[Zhenzhong Duan]

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

[PATCH 4/5] cpuidle-haltpoll: add a check to ensure grow start value is nonzero

[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

Re: [PATCH 4/5] cpuidle-haltpoll: add a check to ensure grow start value is nonzero

[Marcelo Tosatti]

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.

Re: [PATCH 4/5] cpuidle-haltpoll: add a check to ensure grow start value is nonzero

[Zhenzhong Duan]

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

Re: [PATCH 4/5] cpuidle-haltpoll: add a check to ensure grow start value is nonzero

[Paolo Bonzini]

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

[PATCH 5/5] cpuidle-haltpoll: fix up the branch check

[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

Re: [PATCH 5/5] cpuidle-haltpoll: fix up the branch check

[Marcelo Tosatti]

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.

Re: [PATCH 5/5] cpuidle-haltpoll: fix up the branch check

[Zhenzhong Duan]

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

Re: [PATCH 5/5] cpuidle-haltpoll: fix up the branch check

[Marcelo Tosatti]

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

Re: [PATCH 5/5] cpuidle-haltpoll: fix up the branch check

[Zhenzhong Duan]

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