* [PATCH RESEND v2 0/4] misc fixes on halt-poll code for both KVM and guest
@ 2019-11-06 11:54 Zhenzhong Duan
2019-11-06 11:54 ` [PATCH RESEND v2 1/4] cpuidle-haltpoll: ensure grow start value is nonzero Zhenzhong Duan
` (3 more replies)
0 siblings, 4 replies; 13+ messages in thread
From: Zhenzhong Duan @ 2019-11-06 11:54 UTC (permalink / raw)
To: linux-kernel
Cc: pbonzini, rkrcmar, rafael.j.wysocki, joao.m.martins, mtosatti,
kvm, linux-pm, 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH RESEND v2 1/4] cpuidle-haltpoll: ensure grow start value is nonzero
2019-11-06 11:54 [PATCH RESEND v2 0/4] misc fixes on halt-poll code for both KVM and guest Zhenzhong Duan
@ 2019-11-06 11:54 ` Zhenzhong Duan
2019-11-15 10:06 ` Rafael J. Wysocki
2019-11-15 10:26 ` Rafael J. Wysocki
2019-11-06 11:55 ` [PATCH RESEND v2 2/4] KVM: " Zhenzhong Duan
` (2 subsequent siblings)
3 siblings, 2 replies; 13+ messages in thread
From: Zhenzhong Duan @ 2019-11-06 11:54 UTC (permalink / raw)
To: linux-kernel
Cc: pbonzini, rkrcmar, rafael.j.wysocki, joao.m.martins, mtosatti,
kvm, linux-pm, 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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH RESEND v2 2/4] KVM: ensure grow start value is nonzero
2019-11-06 11:54 [PATCH RESEND v2 0/4] misc fixes on halt-poll code for both KVM and guest Zhenzhong Duan
2019-11-06 11:54 ` [PATCH RESEND v2 1/4] cpuidle-haltpoll: ensure grow start value is nonzero Zhenzhong Duan
@ 2019-11-06 11:55 ` Zhenzhong Duan
2019-11-11 20:13 ` Sean Christopherson
2019-11-06 11:55 ` [PATCH RESEND v2 3/4] cpuidle-haltpoll: ensure cpu_halt_poll_us in right scope Zhenzhong Duan
2019-11-06 11:55 ` [PATCH RESEND v2 4/4] KVM: ensure vCPU halt_poll_us " Zhenzhong Duan
3 siblings, 1 reply; 13+ messages in thread
From: Zhenzhong Duan @ 2019-11-06 11:55 UTC (permalink / raw)
To: linux-kernel
Cc: pbonzini, rkrcmar, rafael.j.wysocki, joao.m.martins, mtosatti,
kvm, linux-pm, 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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH RESEND v2 3/4] cpuidle-haltpoll: ensure cpu_halt_poll_us in right scope
2019-11-06 11:54 [PATCH RESEND v2 0/4] misc fixes on halt-poll code for both KVM and guest Zhenzhong Duan
2019-11-06 11:54 ` [PATCH RESEND v2 1/4] cpuidle-haltpoll: ensure grow start value is nonzero Zhenzhong Duan
2019-11-06 11:55 ` [PATCH RESEND v2 2/4] KVM: " Zhenzhong Duan
@ 2019-11-06 11:55 ` Zhenzhong Duan
2019-11-15 10:45 ` Rafael J. Wysocki
2019-11-06 11:55 ` [PATCH RESEND v2 4/4] KVM: ensure vCPU halt_poll_us " Zhenzhong Duan
3 siblings, 1 reply; 13+ messages in thread
From: Zhenzhong Duan @ 2019-11-06 11:55 UTC (permalink / raw)
To: linux-kernel
Cc: pbonzini, rkrcmar, rafael.j.wysocki, joao.m.martins, mtosatti,
kvm, linux-pm, 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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH RESEND v2 4/4] KVM: ensure vCPU halt_poll_us in right scope
2019-11-06 11:54 [PATCH RESEND v2 0/4] misc fixes on halt-poll code for both KVM and guest Zhenzhong Duan
` (2 preceding siblings ...)
2019-11-06 11:55 ` [PATCH RESEND v2 3/4] cpuidle-haltpoll: ensure cpu_halt_poll_us in right scope Zhenzhong Duan
@ 2019-11-06 11:55 ` Zhenzhong Duan
3 siblings, 0 replies; 13+ messages in thread
From: Zhenzhong Duan @ 2019-11-06 11:55 UTC (permalink / raw)
To: linux-kernel
Cc: pbonzini, rkrcmar, rafael.j.wysocki, joao.m.martins, mtosatti,
kvm, linux-pm, 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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH RESEND v2 2/4] KVM: ensure grow start value is nonzero
2019-11-06 11:55 ` [PATCH RESEND v2 2/4] KVM: " Zhenzhong Duan
@ 2019-11-11 20:13 ` Sean Christopherson
2019-11-12 12:19 ` Zhenzhong Duan
0 siblings, 1 reply; 13+ messages in thread
From: Sean Christopherson @ 2019-11-11 20:13 UTC (permalink / raw)
To: Zhenzhong Duan
Cc: linux-kernel, pbonzini, rkrcmar, rafael.j.wysocki,
joao.m.martins, mtosatti, kvm, linux-pm
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
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH RESEND v2 2/4] KVM: ensure grow start value is nonzero
2019-11-11 20:13 ` Sean Christopherson
@ 2019-11-12 12:19 ` Zhenzhong Duan
0 siblings, 0 replies; 13+ messages in thread
From: Zhenzhong Duan @ 2019-11-12 12:19 UTC (permalink / raw)
To: Sean Christopherson
Cc: linux-kernel, pbonzini, rkrcmar, rafael.j.wysocki,
joao.m.martins, mtosatti, kvm, linux-pm
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH RESEND v2 1/4] cpuidle-haltpoll: ensure grow start value is nonzero
2019-11-06 11:54 ` [PATCH RESEND v2 1/4] cpuidle-haltpoll: ensure grow start value is nonzero Zhenzhong Duan
@ 2019-11-15 10:06 ` Rafael J. Wysocki
2019-11-15 10:16 ` Rafael J. Wysocki
2019-11-15 10:26 ` Rafael J. Wysocki
1 sibling, 1 reply; 13+ messages in thread
From: Rafael J. Wysocki @ 2019-11-15 10:06 UTC (permalink / raw)
To: Zhenzhong Duan
Cc: linux-kernel, pbonzini, rkrcmar, rafael.j.wysocki,
joao.m.martins, mtosatti, kvm, linux-pm
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH RESEND v2 1/4] cpuidle-haltpoll: ensure grow start value is nonzero
2019-11-15 10:06 ` Rafael J. Wysocki
@ 2019-11-15 10:16 ` Rafael J. Wysocki
0 siblings, 0 replies; 13+ messages in thread
From: Rafael J. Wysocki @ 2019-11-15 10:16 UTC (permalink / raw)
To: Zhenzhong Duan
Cc: Linux Kernel Mailing List, Paolo Bonzini,
Radim Krčmář,
Rafael Wysocki, Joao Martins, Marcelo Tosatti, kvm-devel,
Linux PM
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH RESEND v2 1/4] cpuidle-haltpoll: ensure grow start value is nonzero
2019-11-06 11:54 ` [PATCH RESEND v2 1/4] cpuidle-haltpoll: ensure grow start value is nonzero Zhenzhong Duan
2019-11-15 10:06 ` Rafael J. Wysocki
@ 2019-11-15 10:26 ` Rafael J. Wysocki
2019-11-17 9:02 ` Zhenzhong Duan
1 sibling, 1 reply; 13+ messages in thread
From: Rafael J. Wysocki @ 2019-11-15 10:26 UTC (permalink / raw)
To: Zhenzhong Duan
Cc: Linux Kernel Mailing List, Paolo Bonzini,
Radim Krčmář,
Rafael Wysocki, Joao Martins, Marcelo Tosatti, kvm-devel,
Linux PM
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
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH RESEND v2 3/4] cpuidle-haltpoll: ensure cpu_halt_poll_us in right scope
2019-11-06 11:55 ` [PATCH RESEND v2 3/4] cpuidle-haltpoll: ensure cpu_halt_poll_us in right scope Zhenzhong Duan
@ 2019-11-15 10:45 ` Rafael J. Wysocki
2019-11-17 8:57 ` Zhenzhong Duan
0 siblings, 1 reply; 13+ messages in thread
From: Rafael J. Wysocki @ 2019-11-15 10:45 UTC (permalink / raw)
To: Zhenzhong Duan
Cc: linux-kernel, pbonzini, rkrcmar, rafael.j.wysocki,
joao.m.martins, mtosatti, kvm, linux-pm
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;
> }
>
> /**
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH RESEND v2 3/4] cpuidle-haltpoll: ensure cpu_halt_poll_us in right scope
2019-11-15 10:45 ` Rafael J. Wysocki
@ 2019-11-17 8:57 ` Zhenzhong Duan
0 siblings, 0 replies; 13+ messages in thread
From: Zhenzhong Duan @ 2019-11-17 8:57 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: linux-kernel, pbonzini, rkrcmar, rafael.j.wysocki,
joao.m.martins, mtosatti, kvm, linux-pm
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH RESEND v2 1/4] cpuidle-haltpoll: ensure grow start value is nonzero
2019-11-15 10:26 ` Rafael J. Wysocki
@ 2019-11-17 9:02 ` Zhenzhong Duan
0 siblings, 0 replies; 13+ messages in thread
From: Zhenzhong Duan @ 2019-11-17 9:02 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Linux Kernel Mailing List, Paolo Bonzini,
Radim Krčmář,
Rafael Wysocki, Joao Martins, Marcelo Tosatti, kvm-devel,
Linux PM
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
>>
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2019-11-17 9:02 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-06 11:54 [PATCH RESEND v2 0/4] misc fixes on halt-poll code for both KVM and guest Zhenzhong Duan
2019-11-06 11:54 ` [PATCH RESEND v2 1/4] cpuidle-haltpoll: ensure grow start value is nonzero Zhenzhong Duan
2019-11-15 10:06 ` Rafael J. Wysocki
2019-11-15 10:16 ` Rafael J. Wysocki
2019-11-15 10:26 ` Rafael J. Wysocki
2019-11-17 9:02 ` Zhenzhong Duan
2019-11-06 11:55 ` [PATCH RESEND v2 2/4] KVM: " Zhenzhong Duan
2019-11-11 20:13 ` Sean Christopherson
2019-11-12 12:19 ` Zhenzhong Duan
2019-11-06 11:55 ` [PATCH RESEND v2 3/4] cpuidle-haltpoll: ensure cpu_halt_poll_us in right scope Zhenzhong Duan
2019-11-15 10:45 ` Rafael J. Wysocki
2019-11-17 8:57 ` Zhenzhong Duan
2019-11-06 11:55 ` [PATCH RESEND v2 4/4] KVM: ensure vCPU halt_poll_us " 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).