| Message ID | 1573041302-4904-2-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 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.
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.
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 >
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 >>
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;
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(-)