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@xxxxxxxxxx>
> > ---
> >   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