[PATCH RESEND v2 0/4] misc fixes on halt-poll code for both KVM and guest

[PATCH RESEND v2 0/4] misc fixes on halt-poll code for both KVM and guest

[Zhenzhong Duan]

This patchset tries to fix below issues:

1. Admin could set halt_poll_ns to 0 at runtime to disable poll and kernel
behave just like the generic halt driver. Then If guest_halt_poll_grow_start
is set to 0 and guest_halt_poll_ns set to nonzero later, cpu_halt_poll_us will
never grow beyond 0. The first two patches fix this issue from both kvm and
guest side.

2. guest_halt_poll_grow_start and guest_halt_poll_ns could be adjusted at
runtime by admin, this could make a window where cpu_halt_poll_us jump out
of the boundary. the window could be long in some cases(e.g. guest_halt_poll_grow_start
is bumped and cpu_halt_poll_us is shrinking) The last two patches fix this
issue from both kvm and guest side.

3. The 4th patch also simplifies branch check code.

v2:
Rewrite the patches and drop unnecessory changes

Zhenzhong Duan (4):
  cpuidle-haltpoll: ensure grow start value is nonzero
  KVM: ensure grow start value is nonzero
  cpuidle-haltpoll: ensure cpu_halt_poll_us in right scope
  KVM: ensure vCPU halt_poll_us in right scope

 drivers/cpuidle/governors/haltpoll.c | 50 ++++++++++++++++++++++++-----------
 virt/kvm/kvm_main.c                  | 51 ++++++++++++++++++++++++------------
 2 files changed, 68 insertions(+), 33 deletions(-)

-- 
1.8.3.1

[PATCH RESEND v2 1/4] cpuidle-haltpoll: ensure grow start value is nonzero

[Zhenzhong Duan]

dev->poll_limit_ns could be zeroed in certain cases (e.g. by
guest_halt_poll_ns = 0). If guest_halt_poll_grow_start is zero,
dev->poll_limit_ns will never be bigger than zero.

Use param callback to avoid writing zero to guest_halt_poll_grow_start.

Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
---
 drivers/cpuidle/governors/haltpoll.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/cpuidle/governors/haltpoll.c b/drivers/cpuidle/governors/haltpoll.c
index 7a703d2..660859d 100644
--- a/drivers/cpuidle/governors/haltpoll.c
+++ b/drivers/cpuidle/governors/haltpoll.c
@@ -20,6 +20,26 @@
 #include <linux/module.h>
 #include <linux/kvm_para.h>
 
+static int grow_start_set(const char *val, const struct kernel_param *kp)
+{
+	int ret;
+	unsigned int n;
+
+	if (!val)
+		return -EINVAL;
+
+	ret = kstrtouint(val, 0, &n);
+	if (ret || !n)
+		return -EINVAL;
+
+	return param_set_uint(val, kp);
+}
+
+static const struct kernel_param_ops grow_start_ops = {
+	.set = grow_start_set,
+	.get = param_get_uint,
+};
+
 static unsigned int guest_halt_poll_ns __read_mostly = 200000;
 module_param(guest_halt_poll_ns, uint, 0644);
 
@@ -33,7 +53,7 @@
 
 /* value in us to start growing per-cpu halt_poll_ns */
 static unsigned int guest_halt_poll_grow_start __read_mostly = 50000;
-module_param(guest_halt_poll_grow_start, uint, 0644);
+module_param_cb(guest_halt_poll_grow_start, &grow_start_ops, &guest_halt_poll_grow_start, 0644);
 
 /* allow shrinking guest halt poll */
 static bool guest_halt_poll_allow_shrink __read_mostly = true;
-- 
1.8.3.1

[PATCH RESEND v2 2/4] KVM: ensure grow start value is nonzero

[Zhenzhong Duan]

vcpu->halt_poll_ns could be zeroed in certain cases (e.g. by
halt_poll_ns = 0). If halt_poll_grow_start is zero,
vcpu->halt_poll_ns will never be bigger than zero.

Use param callback to avoid writing zero to halt_poll_grow_start.

Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
---
 virt/kvm/kvm_main.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index d6f0696..359516b 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -69,6 +69,26 @@
 MODULE_AUTHOR("Qumranet");
 MODULE_LICENSE("GPL");
 
+static int grow_start_set(const char *val, const struct kernel_param *kp)
+{
+	int ret;
+	unsigned int n;
+
+	if (!val)
+		return -EINVAL;
+
+	ret = kstrtouint(val, 0, &n);
+	if (ret || !n)
+		return -EINVAL;
+
+	return param_set_uint(val, kp);
+}
+
+static const struct kernel_param_ops grow_start_ops = {
+	.set = grow_start_set,
+	.get = param_get_uint,
+};
+
 /* Architectures should define their poll value according to the halt latency */
 unsigned int halt_poll_ns = KVM_HALT_POLL_NS_DEFAULT;
 module_param(halt_poll_ns, uint, 0644);
@@ -81,7 +101,7 @@
 
 /* The start value to grow halt_poll_ns from */
 unsigned int halt_poll_ns_grow_start = 10000; /* 10us */
-module_param(halt_poll_ns_grow_start, uint, 0644);
+module_param_cb(halt_poll_ns_grow_start, &grow_start_ops, &halt_poll_ns_grow_start, 0644);
 EXPORT_SYMBOL_GPL(halt_poll_ns_grow_start);
 
 /* Default resets per-vcpu halt_poll_ns . */
-- 
1.8.3.1

[PATCH RESEND v2 3/4] cpuidle-haltpoll: ensure cpu_halt_poll_us in right scope

[Zhenzhong Duan]

As user can adjust guest_halt_poll_grow_start and guest_halt_poll_ns
which leads to cpu_halt_poll_us beyond the two boundaries. This patch
ensures cpu_halt_poll_us in that scope.

If guest_halt_poll_shrink is 0, shrink the cpu_halt_poll_us to
guest_halt_poll_grow_start instead of 0. To disable poll we can set
guest_halt_poll_ns to 0.

If user wrongly set guest_halt_poll_grow_start > guest_halt_poll_ns > 0,
guest_halt_poll_ns take precedency and poll time is a fixed value of
guest_halt_poll_ns.

Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
---
 drivers/cpuidle/governors/haltpoll.c | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/drivers/cpuidle/governors/haltpoll.c b/drivers/cpuidle/governors/haltpoll.c
index 660859d..4a39df4 100644
--- a/drivers/cpuidle/governors/haltpoll.c
+++ b/drivers/cpuidle/governors/haltpoll.c
@@ -97,32 +97,30 @@ 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 = dev->poll_limit_ns;
 	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 &&
+	    guest_halt_poll_grow)
 		val = dev->poll_limit_ns * guest_halt_poll_grow;
-
-		if (val < guest_halt_poll_grow_start)
-			val = guest_halt_poll_grow_start;
-		if (val > guest_halt_poll_ns)
-			val = guest_halt_poll_ns;
-
-		dev->poll_limit_ns = val;
-	} else if (block_ns > guest_halt_poll_ns &&
-		   guest_halt_poll_allow_shrink) {
+	else if (block_ns > guest_halt_poll_ns &&
+		 guest_halt_poll_allow_shrink) {
 		unsigned int shrink = guest_halt_poll_shrink;
 
-		val = dev->poll_limit_ns;
 		if (shrink == 0)
-			val = 0;
+			val = guest_halt_poll_grow_start;
 		else
 			val /= shrink;
-		dev->poll_limit_ns = val;
 	}
+	if (val < guest_halt_poll_grow_start)
+		val = guest_halt_poll_grow_start;
+	if (val > guest_halt_poll_ns)
+		val = guest_halt_poll_ns;
+
+	dev->poll_limit_ns = val;
 }
 
 /**
-- 
1.8.3.1

[PATCH RESEND v2 4/4] KVM: ensure vCPU halt_poll_us in right scope

[Zhenzhong Duan]

As user can adjust halt_poll_ns_grow_start and halt_poll_ns which
leads to vcpu->halt_poll_ns beyond the two boundaries. This patch
ensures vcpu->halt_poll_ns in that scope after growing or shrinking.

If halt_poll_ns_shrink is 0, shrink vcpu->halt_poll_ns to
halt_poll_ns_grow_start instead of 0. To disable poll we can set
halt_poll_ns to 0.

In case user wrongly set halt_poll_ns_grow_start > halt_poll_ns > 0,
halt_poll_ns take precedency and poll time is a fixed value of
halt_poll_ns.

This patch also simplifies branch check based on the guest haltpoll
code.

Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
---
 virt/kvm/kvm_main.c | 29 +++++++++++++----------------
 1 file changed, 13 insertions(+), 16 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 359516b..b4fca66 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2308,9 +2308,15 @@ static void shrink_halt_poll_ns(struct kvm_vcpu *vcpu)
 	old = val = vcpu->halt_poll_ns;
 	shrink = READ_ONCE(halt_poll_ns_shrink);
 	if (shrink == 0)
-		val = 0;
-	else
+		val = halt_poll_ns_grow_start;
+	else {
 		val /= shrink;
+		if (val < halt_poll_ns_grow_start)
+			val = halt_poll_ns_grow_start;
+	}
+
+	if (val > halt_poll_ns)
+		val = halt_poll_ns;
 
 	vcpu->halt_poll_ns = val;
 	trace_kvm_halt_poll_ns_shrink(vcpu->vcpu_id, val, old);
@@ -2385,21 +2391,12 @@ void kvm_vcpu_block(struct kvm_vcpu *vcpu)
 	block_ns = ktime_to_ns(cur) - ktime_to_ns(start);
 
 	if (!kvm_arch_no_poll(vcpu)) {
-		if (!vcpu_valid_wakeup(vcpu)) {
+		/* we had a long block, shrink polling */
+		if (!vcpu_valid_wakeup(vcpu) || block_ns > halt_poll_ns)
 			shrink_halt_poll_ns(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)
-				grow_halt_poll_ns(vcpu);
-		} else {
-			vcpu->halt_poll_ns = 0;
-		}
+		/* we had a short block and our poll time is too small */
+		else if (block_ns > vcpu->halt_poll_ns)
+			grow_halt_poll_ns(vcpu);
 	}
 
 	trace_kvm_vcpu_wakeup(block_ns, waited, vcpu_valid_wakeup(vcpu));
-- 
1.8.3.1

Re: [PATCH RESEND v2 2/4] KVM: ensure grow start value is nonzero

[Sean Christopherson]

On Wed, Nov 06, 2019 at 07:55:00PM +0800, Zhenzhong Duan wrote:
> vcpu->halt_poll_ns could be zeroed in certain cases (e.g. by
> halt_poll_ns = 0). If halt_poll_grow_start is zero,
> vcpu->halt_poll_ns will never be bigger than zero.
> 
> Use param callback to avoid writing zero to halt_poll_grow_start.

This doesn't explain why allowing an admin to disable halt polling by
writing halt_poll_grow_start=0 is a bad thing.  Paolo had the same
question in v1, here[1] and in the guest driver[2].

[1] https://lkml.kernel.org/r/57679389-6e4a-b7ad-559f-3128a608c28a@redhat.com
[2] https://lkml.kernel.org/r/391dd11b-ebbb-28ff-5e57-4a795cd16a1b@redhat.com

> 
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
> ---
>  virt/kvm/kvm_main.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index d6f0696..359516b 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -69,6 +69,26 @@
>  MODULE_AUTHOR("Qumranet");
>  MODULE_LICENSE("GPL");
>  
> +static int grow_start_set(const char *val, const struct kernel_param *kp)
> +{
> +	int ret;
> +	unsigned int n;
> +
> +	if (!val)
> +		return -EINVAL;
> +
> +	ret = kstrtouint(val, 0, &n);
> +	if (ret || !n)
> +		return -EINVAL;
> +
> +	return param_set_uint(val, kp);
> +}
> +
> +static const struct kernel_param_ops grow_start_ops = {
> +	.set = grow_start_set,
> +	.get = param_get_uint,
> +};
> +
>  /* Architectures should define their poll value according to the halt latency */
>  unsigned int halt_poll_ns = KVM_HALT_POLL_NS_DEFAULT;
>  module_param(halt_poll_ns, uint, 0644);
> @@ -81,7 +101,7 @@
>  
>  /* The start value to grow halt_poll_ns from */
>  unsigned int halt_poll_ns_grow_start = 10000; /* 10us */
> -module_param(halt_poll_ns_grow_start, uint, 0644);
> +module_param_cb(halt_poll_ns_grow_start, &grow_start_ops, &halt_poll_ns_grow_start, 0644);
>  EXPORT_SYMBOL_GPL(halt_poll_ns_grow_start);
>  
>  /* Default resets per-vcpu halt_poll_ns . */
> -- 
> 1.8.3.1
> 

Re: [PATCH RESEND v2 2/4] KVM: ensure grow start value is nonzero

[Zhenzhong Duan]

On 2019/11/12 4:13, Sean Christopherson wrote:
> On Wed, Nov 06, 2019 at 07:55:00PM +0800, Zhenzhong Duan wrote:
>> vcpu->halt_poll_ns could be zeroed in certain cases (e.g. by
>> halt_poll_ns = 0). If halt_poll_grow_start is zero,
>> vcpu->halt_poll_ns will never be bigger than zero.
>>
>> Use param callback to avoid writing zero to halt_poll_grow_start.
> This doesn't explain why allowing an admin to disable halt polling by
> writing halt_poll_grow_start=0 is a bad thing.  Paolo had the same
> question in v1, here[1] and in the guest driver[2].
>
> [1]https://lkml.kernel.org/r/57679389-6e4a-b7ad-559f-3128a608c28a@redhat.com
> [2]https://lkml.kernel.org/r/391dd11b-ebbb-28ff-5e57-4a795cd16a1b@redhat.com

Ok, answer all the same questions about grow_start=0 here.

VCPU halt polling time may be nonzero even if grow_start=0, such as in below situation:
0=grow_start< block_ns< (vcpu->halt_poll_ns)< halt_poll_ns

grow_start=0 has your mentioned effect only in below sequence:
1. set halt_poll_ns=0 to disable halt polling(this lead to vcpu->halt_poll_ns=0)
2. set grow_start=0
3. set halt_poll_ns to nonzero
4. Admin expect halt polling time auto adjust in range [0, nonzero], but polling time stick at 0.

So I think we should use halt_poll_ns=0 to disable halt polling instead of grow_start=0.

Zhenzhong

Re: [PATCH RESEND v2 1/4] cpuidle-haltpoll: ensure grow start value is nonzero

[Rafael J. Wysocki]

On Wednesday, November 6, 2019 12:54:59 PM CET Zhenzhong Duan wrote:
> dev->poll_limit_ns could be zeroed in certain cases (e.g. by
> guest_halt_poll_ns = 0). If guest_halt_poll_grow_start is zero,
> dev->poll_limit_ns will never be bigger than zero.

Given that haltpoll_enable_device() sets dev->poll_limit_ns = 0 to start with,
I don't think that the statement above is correct.

Re: [PATCH RESEND v2 1/4] cpuidle-haltpoll: ensure grow start value is nonzero

[Rafael J. Wysocki]

On Fri, Nov 15, 2019 at 11:06 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> On Wednesday, November 6, 2019 12:54:59 PM CET Zhenzhong Duan wrote:
> > dev->poll_limit_ns could be zeroed in certain cases (e.g. by
> > guest_halt_poll_ns = 0). If guest_halt_poll_grow_start is zero,
> > dev->poll_limit_ns will never be bigger than zero.
>
> Given that haltpoll_enable_device() sets dev->poll_limit_ns = 0 to start with,
> I don't think that the statement above is correct.

Scratch this, I misread it.

Re: [PATCH RESEND v2 1/4] cpuidle-haltpoll: ensure grow start value is nonzero

[Rafael J. Wysocki]

On Wed, Nov 6, 2019 at 12:56 PM Zhenzhong Duan
<zhenzhong.duan@oracle.com> wrote:
>
> dev->poll_limit_ns could be zeroed in certain cases (e.g. by
> guest_halt_poll_ns = 0). If guest_halt_poll_grow_start is zero,
> dev->poll_limit_ns will never be bigger than zero.

I would rephrase this in the following way:

"If guest_halt_poll_grow_start is zero and dev->poll_limit_ns becomes
zero for any reason, it will never be greater than zero again, so use
..."

The patch itself looks OK to me.

> Use param callback to avoid writing zero to guest_halt_poll_grow_start.
>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
> ---
>  drivers/cpuidle/governors/haltpoll.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/cpuidle/governors/haltpoll.c b/drivers/cpuidle/governors/haltpoll.c
> index 7a703d2..660859d 100644
> --- a/drivers/cpuidle/governors/haltpoll.c
> +++ b/drivers/cpuidle/governors/haltpoll.c
> @@ -20,6 +20,26 @@
>  #include <linux/module.h>
>  #include <linux/kvm_para.h>
>
> +static int grow_start_set(const char *val, const struct kernel_param *kp)
> +{
> +       int ret;
> +       unsigned int n;
> +
> +       if (!val)
> +               return -EINVAL;
> +
> +       ret = kstrtouint(val, 0, &n);
> +       if (ret || !n)
> +               return -EINVAL;
> +
> +       return param_set_uint(val, kp);
> +}
> +
> +static const struct kernel_param_ops grow_start_ops = {
> +       .set = grow_start_set,
> +       .get = param_get_uint,
> +};
> +
>  static unsigned int guest_halt_poll_ns __read_mostly = 200000;
>  module_param(guest_halt_poll_ns, uint, 0644);
>
> @@ -33,7 +53,7 @@
>
>  /* value in us to start growing per-cpu halt_poll_ns */
>  static unsigned int guest_halt_poll_grow_start __read_mostly = 50000;
> -module_param(guest_halt_poll_grow_start, uint, 0644);
> +module_param_cb(guest_halt_poll_grow_start, &grow_start_ops, &guest_halt_poll_grow_start, 0644);
>
>  /* allow shrinking guest halt poll */
>  static bool guest_halt_poll_allow_shrink __read_mostly = true;
> --
> 1.8.3.1
>

Re: [PATCH RESEND v2 3/4] cpuidle-haltpoll: ensure cpu_halt_poll_us in right scope

[Rafael J. Wysocki]

On Wednesday, November 6, 2019 12:55:01 PM CET Zhenzhong Duan wrote:
> As user can adjust guest_halt_poll_grow_start and guest_halt_poll_ns
> which leads to cpu_halt_poll_us beyond the two boundaries. This patch
> ensures cpu_halt_poll_us in that scope.
> 
> If guest_halt_poll_shrink is 0, shrink the cpu_halt_poll_us to
> guest_halt_poll_grow_start instead of 0. To disable poll we can set
> guest_halt_poll_ns to 0.
> 
> If user wrongly set guest_halt_poll_grow_start > guest_halt_poll_ns > 0,
> guest_halt_poll_ns take precedency and poll time is a fixed value of
> guest_halt_poll_ns.
> 
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
> ---
>  drivers/cpuidle/governors/haltpoll.c | 28 +++++++++++++---------------
>  1 file changed, 13 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/cpuidle/governors/haltpoll.c b/drivers/cpuidle/governors/haltpoll.c
> index 660859d..4a39df4 100644
> --- a/drivers/cpuidle/governors/haltpoll.c
> +++ b/drivers/cpuidle/governors/haltpoll.c
> @@ -97,32 +97,30 @@ 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 = dev->poll_limit_ns;

Not necessary to initialize it here.

>  	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

You could update the comment to say "dev->poll_limit_ns" instead of
"cpu_halt_poll_us" while at it.

>  	 */
> -	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 &&
> +	    guest_halt_poll_grow)

The "{" brace is still needed as per the coding style and I'm not sure why
to avoid guest_halt_poll_grow equal to zero here?

>  		val = dev->poll_limit_ns * guest_halt_poll_grow;
> -
> -		if (val < guest_halt_poll_grow_start)
> -			val = guest_halt_poll_grow_start;
> -		if (val > guest_halt_poll_ns)
> -			val = guest_halt_poll_ns;
> -
> -		dev->poll_limit_ns = val;
> -	} else if (block_ns > guest_halt_poll_ns &&
> -		   guest_halt_poll_allow_shrink) {
> +	else if (block_ns > guest_halt_poll_ns &&
> +		 guest_halt_poll_allow_shrink) {
>  		unsigned int shrink = guest_halt_poll_shrink;
>  
> -		val = dev->poll_limit_ns;
>  		if (shrink == 0)
> -			val = 0;
> +			val = guest_halt_poll_grow_start;

That's going to be corrected below, so the original code would be fine.

>  		else
>  			val /= shrink;

Here you can do

			val = dev->poll_limit_ns / shrink;

> -		dev->poll_limit_ns = val;
>  	}
> +	if (val < guest_halt_poll_grow_start)
> +		val = guest_halt_poll_grow_start;

Note that guest_halt_poll_grow_start is in us (as per the comment next to its
definition and the initial value).  That is a bug in the original code too,
but anyway.

> +	if (val > guest_halt_poll_ns)
> +		val = guest_halt_poll_ns;
> +
> +	dev->poll_limit_ns = val;
>  }
>  
>  /**
> 

Re: [PATCH RESEND v2 3/4] cpuidle-haltpoll: ensure cpu_halt_poll_us in right scope

[Zhenzhong Duan]

On 2019/11/15 18:45, Rafael J. Wysocki wrote:

> On Wednesday, November 6, 2019 12:55:01 PM CET Zhenzhong Duan wrote:
>> As user can adjust guest_halt_poll_grow_start and guest_halt_poll_ns
>> which leads to cpu_halt_poll_us beyond the two boundaries. This patch
>> ensures cpu_halt_poll_us in that scope.
>>
>> If guest_halt_poll_shrink is 0, shrink the cpu_halt_poll_us to
>> guest_halt_poll_grow_start instead of 0. To disable poll we can set
>> guest_halt_poll_ns to 0.
>>
>> If user wrongly set guest_halt_poll_grow_start > guest_halt_poll_ns > 0,
>> guest_halt_poll_ns take precedency and poll time is a fixed value of
>> guest_halt_poll_ns.
>>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
>> ---
>>   drivers/cpuidle/governors/haltpoll.c | 28 +++++++++++++---------------
>>   1 file changed, 13 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/cpuidle/governors/haltpoll.c b/drivers/cpuidle/governors/haltpoll.c
>> index 660859d..4a39df4 100644
>> --- a/drivers/cpuidle/governors/haltpoll.c
>> +++ b/drivers/cpuidle/governors/haltpoll.c
>> @@ -97,32 +97,30 @@ 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 = dev->poll_limit_ns;
> Not necessary to initialize it here.

Then an random val may bypass all the check and get assigned to dev->poll_limit_ns

if guest_halt_poll_grow_start< block_ns< uninitialized val< guest_halt_poll_ns

With my change, dev->poll_limit_ns will not be changed in that case, logic same as original code.

>
>>   	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
> You could update the comment to say "dev->poll_limit_ns" instead of
> "cpu_halt_poll_us" while at it.

Will do, also guest_halt_poll_us to guest_halt_poll_ns

>
>>   	 */
>> -	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 &&
>> +	    guest_halt_poll_grow)
> The "{" brace is still needed as per the coding style and I'm not sure why
> to avoid guest_halt_poll_grow equal to zero here?

Will add "{}" and remove guest_halt_poll_grow check. My inital thought was to prevent

dev->poll_limit_ns get shrinked with guest_halt_poll_grow=0.

>
>>   		val = dev->poll_limit_ns * guest_halt_poll_grow;
>> -
>> -		if (val < guest_halt_poll_grow_start)
>> -			val = guest_halt_poll_grow_start;
>> -		if (val > guest_halt_poll_ns)
>> -			val = guest_halt_poll_ns;
>> -
>> -		dev->poll_limit_ns = val;
>> -	} else if (block_ns > guest_halt_poll_ns &&
>> -		   guest_halt_poll_allow_shrink) {
>> +	else if (block_ns > guest_halt_poll_ns &&
>> +		 guest_halt_poll_allow_shrink) {
>>   		unsigned int shrink = guest_halt_poll_shrink;
>>   
>> -		val = dev->poll_limit_ns;
>>   		if (shrink == 0)
>> -			val = 0;
>> +			val = guest_halt_poll_grow_start;
> That's going to be corrected below, so the original code would be fine.

val was assigned twice using 'val = 0' while it's once with my change, optimal a bit?

>
>>   		else
>>   			val /= shrink;
> Here you can do
>
> 			val = dev->poll_limit_ns / shrink;

Any special reason？Looks no difference for me.

>
>> -		dev->poll_limit_ns = val;
>>   	}
>> +	if (val < guest_halt_poll_grow_start)
>> +		val = guest_halt_poll_grow_start;
> Note that guest_halt_poll_grow_start is in us (as per the comment next to its
> definition and the initial value).  That is a bug in the original code too,
> but anyway.

Good catch! will fix the comment. The default 50000ns vs 50000us, looks author means ns.
guest_halt_poll_ns defaults to 200000, also hints ns for guest_halt_poll_grow_start.

Thanks

Zhenzhong

Re: [PATCH RESEND v2 1/4] cpuidle-haltpoll: ensure grow start value is nonzero

[Zhenzhong Duan]

On 2019/11/15 18:26, Rafael J. Wysocki wrote:
> On Wed, Nov 6, 2019 at 12:56 PM Zhenzhong Duan
> <zhenzhong.duan@oracle.com> wrote:
>> dev->poll_limit_ns could be zeroed in certain cases (e.g. by
>> guest_halt_poll_ns = 0). If guest_halt_poll_grow_start is zero,
>> dev->poll_limit_ns will never be bigger than zero.
> I would rephrase this in the following way:
>
> "If guest_halt_poll_grow_start is zero and dev->poll_limit_ns becomes
> zero for any reason, it will never be greater than zero again, so use
> ..."

OK, will do, thanks for your suggestion.

Zhenzhong

>
> The patch itself looks OK to me.
>
>> Use param callback to avoid writing zero to guest_halt_poll_grow_start.
>>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
>> ---
>>   drivers/cpuidle/governors/haltpoll.c | 22 +++++++++++++++++++++-
>>   1 file changed, 21 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/cpuidle/governors/haltpoll.c b/drivers/cpuidle/governors/haltpoll.c
>> index 7a703d2..660859d 100644
>> --- a/drivers/cpuidle/governors/haltpoll.c
>> +++ b/drivers/cpuidle/governors/haltpoll.c
>> @@ -20,6 +20,26 @@
>>   #include <linux/module.h>
>>   #include <linux/kvm_para.h>
>>
>> +static int grow_start_set(const char *val, const struct kernel_param *kp)
>> +{
>> +       int ret;
>> +       unsigned int n;
>> +
>> +       if (!val)
>> +               return -EINVAL;
>> +
>> +       ret = kstrtouint(val, 0, &n);
>> +       if (ret || !n)
>> +               return -EINVAL;
>> +
>> +       return param_set_uint(val, kp);
>> +}
>> +
>> +static const struct kernel_param_ops grow_start_ops = {
>> +       .set = grow_start_set,
>> +       .get = param_get_uint,
>> +};
>> +
>>   static unsigned int guest_halt_poll_ns __read_mostly = 200000;
>>   module_param(guest_halt_poll_ns, uint, 0644);
>>
>> @@ -33,7 +53,7 @@
>>
>>   /* value in us to start growing per-cpu halt_poll_ns */
>>   static unsigned int guest_halt_poll_grow_start __read_mostly = 50000;
>> -module_param(guest_halt_poll_grow_start, uint, 0644);
>> +module_param_cb(guest_halt_poll_grow_start, &grow_start_ops, &guest_halt_poll_grow_start, 0644);
>>
>>   /* allow shrinking guest halt poll */
>>   static bool guest_halt_poll_allow_shrink __read_mostly = true;
>> --
>> 1.8.3.1
>>