| Message ID | 20250403092852.1072015-1-quic_zhonhan@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| 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/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.
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.
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~
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(-)