| Message ID | 20250403092852.1072015-1-quic_zhonhan@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | cpuidle: menu: Optimize bucket assignment when next_timer_ns equals KTIME_MAX | expand |
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) ||
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.
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) ||
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(-)