| Message ID | 1573041302-4904-3-git-send-email-zhenzhong.duan@oracle.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | misc fixes on halt-poll code for both KVM and guest | expand |
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 >
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
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 . */
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(-)