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

[Zhenzhong Duan <zhenzhong.duan@oracle.com>]

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