cpuidle: menu: Optimize bucket assignment when next_timer_ns equals KTIME_MAX

[PATCH] cpuidle: menu: Optimize bucket assignment when next_timer_ns equals KTIME_MAX

Posted by Zhongqiu Han 1 day, 1 hour ago

Directly assign the last bucket value instead of calling which_bucket()
when next_timer_ns equals KTIME_MAX, the largest possible value that
always falls into the last bucket. This avoids unnecessary calculations
and enhances performance.

Signed-off-by: Zhongqiu Han <quic_zhonhan@quicinc.com>
---
 drivers/cpuidle/governors/menu.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c
index 39aa0aea61c6..8fc7fbed0052 100644
--- a/drivers/cpuidle/governors/menu.c
+++ b/drivers/cpuidle/governors/menu.c
@@ -255,7 +255,12 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev,
 		 */
 		data->next_timer_ns = KTIME_MAX;
 		delta_tick = TICK_NSEC / 2;
-		data->bucket = which_bucket(KTIME_MAX);
+		/*
+		 * Assign the last bucket value directly instead of calling
+		 * which_bucket(), since KTIME_MAX is the largest possible
+		 * value that always falls into the last bucket.
+		 */
+		data->bucket = BUCKETS - 1;
 	}
 
 	if (unlikely(drv->state_count <= 1 || latency_req == 0) ||
-- 
2.25.1

Re: [PATCH] cpuidle: menu: Optimize bucket assignment when next_timer_ns equals KTIME_MAX

Posted by Christian Loehle 1 day, 1 hour ago

On 4/3/25 10:28, Zhongqiu Han wrote:
> Directly assign the last bucket value instead of calling which_bucket()
> when next_timer_ns equals KTIME_MAX, the largest possible value that
> always falls into the last bucket. This avoids unnecessary calculations
> and enhances performance.
> 
> Signed-off-by: Zhongqiu Han <quic_zhonhan@quicinc.com>

Reviewed-by: Christian Loehle <christian.loehle@arm.com>

> ---
>  drivers/cpuidle/governors/menu.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c
> index 39aa0aea61c6..8fc7fbed0052 100644
> --- a/drivers/cpuidle/governors/menu.c
> +++ b/drivers/cpuidle/governors/menu.c
> @@ -255,7 +255,12 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev,
>  		 */
>  		data->next_timer_ns = KTIME_MAX;
>  		delta_tick = TICK_NSEC / 2;
> -		data->bucket = which_bucket(KTIME_MAX);
> +		/*
> +		 * Assign the last bucket value directly instead of calling
> +		 * which_bucket(), since KTIME_MAX is the largest possible
> +		 * value that always falls into the last bucket.
> +		 */

comment almost seems overkill.

> +		data->bucket = BUCKETS - 1;
>  	}
>  
>  	if (unlikely(drv->state_count <= 1 || latency_req == 0) ||

Re: [PATCH] cpuidle: menu: Optimize bucket assignment when next_timer_ns equals KTIME_MAX

Posted by Zhongqiu Han 1 day, 1 hour ago

On 4/3/2025 5:34 PM, Christian Loehle wrote:
> On 4/3/25 10:28, Zhongqiu Han wrote:
>> Directly assign the last bucket value instead of calling which_bucket()
>> when next_timer_ns equals KTIME_MAX, the largest possible value that
>> always falls into the last bucket. This avoids unnecessary calculations
>> and enhances performance.
>>
>> Signed-off-by: Zhongqiu Han <quic_zhonhan@quicinc.com>
> 
> Reviewed-by: Christian Loehle <christian.loehle@arm.com>
> 
>> ---
>>   drivers/cpuidle/governors/menu.c | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c
>> index 39aa0aea61c6..8fc7fbed0052 100644
>> --- a/drivers/cpuidle/governors/menu.c
>> +++ b/drivers/cpuidle/governors/menu.c
>> @@ -255,7 +255,12 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev,
>>   		 */
>>   		data->next_timer_ns = KTIME_MAX;
>>   		delta_tick = TICK_NSEC / 2;
>> -		data->bucket = which_bucket(KTIME_MAX);
>> +		/*
>> +		 * Assign the last bucket value directly instead of calling
>> +		 * which_bucket(), since KTIME_MAX is the largest possible
>> +		 * value that always falls into the last bucket.
>> +		 */
> 
> comment almost seems overkill.
> 
>> +		data->bucket = BUCKETS - 1;
>>   	}
>>   
>>   	if (unlikely(drv->state_count <= 1 || latency_req == 0) ||
> 
Thanks Christian for the review~

Actually I just want to add a comment to indicate that which_bucket()
was once called here, in case which_bucket() changes in the future,
and however, we stayed with the original approach, leading to the
inconsistency.

Could you please review the comment below and let me know if it's okay
or if I should not add any log? Thanks a lot~

/* KTIME_MAX falls into the last bucket, skip which_bucket(). */



I will collect review comments before arise patch V2.


-- 
Thx and BRs,
Zhongqiu Han

Re: [PATCH] cpuidle: menu: Optimize bucket assignment when next_timer_ns equals KTIME_MAX

Posted by Christian Loehle 1 day, 1 hour ago

On 4/3/25 11:01, Zhongqiu Han wrote:
> On 4/3/2025 5:34 PM, Christian Loehle wrote:
>> On 4/3/25 10:28, Zhongqiu Han wrote:
>>> Directly assign the last bucket value instead of calling which_bucket()
>>> when next_timer_ns equals KTIME_MAX, the largest possible value that
>>> always falls into the last bucket. This avoids unnecessary calculations
>>> and enhances performance.
>>>
>>> Signed-off-by: Zhongqiu Han <quic_zhonhan@quicinc.com>
>>
>> Reviewed-by: Christian Loehle <christian.loehle@arm.com>
>>
>>> ---
>>>   drivers/cpuidle/governors/menu.c | 7 ++++++-
>>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c
>>> index 39aa0aea61c6..8fc7fbed0052 100644
>>> --- a/drivers/cpuidle/governors/menu.c
>>> +++ b/drivers/cpuidle/governors/menu.c
>>> @@ -255,7 +255,12 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev,
>>>            */
>>>           data->next_timer_ns = KTIME_MAX;
>>>           delta_tick = TICK_NSEC / 2;
>>> -        data->bucket = which_bucket(KTIME_MAX);
>>> +        /*
>>> +         * Assign the last bucket value directly instead of calling
>>> +         * which_bucket(), since KTIME_MAX is the largest possible
>>> +         * value that always falls into the last bucket.
>>> +         */
>>
>> comment almost seems overkill.
>>
>>> +        data->bucket = BUCKETS - 1;
>>>       }
>>>         if (unlikely(drv->state_count <= 1 || latency_req == 0) ||
>>
> Thanks Christian for the review~
> 
> Actually I just want to add a comment to indicate that which_bucket()
> was once called here, in case which_bucket() changes in the future,
> and however, we stayed with the original approach, leading to the
> inconsistency.
> 
> Could you please review the comment below and let me know if it's okay
> or if I should not add any log? Thanks a lot~
> 
> /* KTIME_MAX falls into the last bucket, skip which_bucket(). */
> 
> 
> 
> I will collect review comments before arise patch V2.

Honestly I'd be fine without a comment, it's pretty obvious that
everything containing "bucket =" needs to be changed if the bucket
logic ever changes.

Re: [PATCH] cpuidle: menu: Optimize bucket assignment when next_timer_ns equals KTIME_MAX

Posted by Zhongqiu Han 1 day ago

On 4/3/2025 6:06 PM, Christian Loehle wrote:
> On 4/3/25 11:01, Zhongqiu Han wrote:
>> On 4/3/2025 5:34 PM, Christian Loehle wrote:
>>> On 4/3/25 10:28, Zhongqiu Han wrote:
>>>> Directly assign the last bucket value instead of calling which_bucket()
>>>> when next_timer_ns equals KTIME_MAX, the largest possible value that
>>>> always falls into the last bucket. This avoids unnecessary calculations
>>>> and enhances performance.
>>>>
>>>> Signed-off-by: Zhongqiu Han <quic_zhonhan@quicinc.com>
>>>
>>> Reviewed-by: Christian Loehle <christian.loehle@arm.com>
>>>
>>>> ---
>>>>    drivers/cpuidle/governors/menu.c | 7 ++++++-
>>>>    1 file changed, 6 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c
>>>> index 39aa0aea61c6..8fc7fbed0052 100644
>>>> --- a/drivers/cpuidle/governors/menu.c
>>>> +++ b/drivers/cpuidle/governors/menu.c
>>>> @@ -255,7 +255,12 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev,
>>>>             */
>>>>            data->next_timer_ns = KTIME_MAX;
>>>>            delta_tick = TICK_NSEC / 2;
>>>> -        data->bucket = which_bucket(KTIME_MAX);
>>>> +        /*
>>>> +         * Assign the last bucket value directly instead of calling
>>>> +         * which_bucket(), since KTIME_MAX is the largest possible
>>>> +         * value that always falls into the last bucket.
>>>> +         */
>>>
>>> comment almost seems overkill.
>>>
>>>> +        data->bucket = BUCKETS - 1;
>>>>        }
>>>>          if (unlikely(drv->state_count <= 1 || latency_req == 0) ||
>>>
>> Thanks Christian for the review~
>>
>> Actually I just want to add a comment to indicate that which_bucket()
>> was once called here, in case which_bucket() changes in the future,
>> and however, we stayed with the original approach, leading to the
>> inconsistency.
>>
>> Could you please review the comment below and let me know if it's okay
>> or if I should not add any log? Thanks a lot~
>>
>> /* KTIME_MAX falls into the last bucket, skip which_bucket(). */
>>
>>
>>
>> I will collect review comments before arise patch V2.
> 
> Honestly I'd be fine without a comment, it's pretty obvious that
> everything containing "bucket =" needs to be changed if the bucket
> logic ever changes.
Thanks Christian, let me wait more other aspects comments~

-- 
Thx and BRs,
Zhongqiu Han