| Message ID | 20221105174225.28673-1-rui.zhang@intel.com (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
| Series | [RFC,1/3] cpuidle: ladder: Fix bogus comparison between s64 and u64 | expand |
On Sat, Nov 5, 2022 at 6:40 PM Zhang Rui <rui.zhang@intel.com> wrote: > > ladder_device_state.threshold.promotion_time_ns/demotion_time_ns > are u64 type. > > In ladder_select_state(), variable 'last_residency', as calculated by > > last_residency = dev->last_residency_ns - drv->states[last_idx].exit_latency_ns > > are s64 type, and it can be negative value. The code changes are fine AFAICS, but the description below could be more precise. > When this happens, comparing between 'last_residency' and > 'promotion_time_ns/demotion_time_ns' become bogus. IIUC, what happens is that last_residency is converted to u64 in the comparison expression and that conversion causes it to become a large positive number if it is negative. > As a result, the ladder governor promotes or stays with current state errornously. "promotes or retains the current state erroneously". > > <idle>-0 [001] d..1. 151.893396: ladder_select_state: last_idx 7, last_residency -373033 > <idle>-0 [001] d..1. 151.893399: ladder_select_state: dev->last_residency_ns 106967, drv->states[last_idx].exit_latency_ns 480000 > <idle>-0 [001] d..1. 151.893402: ladder_select_state: promote, last_state->threshold.promotion_time_ns 480000 > <idle>-0 [001] d..1. 151.893404: ladder_select_state: ---> new state 7 > <idle>-0 [001] d..1. 151.893465: ladder_select_state: last_idx 7, last_residency -463800 > <idle>-0 [001] d..1. 151.893467: ladder_select_state: dev->last_residency_ns 16200, drv->states[last_idx].exit_latency_ns 480000 > <idle>-0 [001] d..1. 151.893468: ladder_select_state: promote, last_state->threshold.promotion_time_ns 480000 > <idle>-0 [001] dn.1. 151.893470: ladder_select_state: ---> new state 8 > > Given that promotion_time_ns/demotion_time_ns are initialized with > cpuidle_state.exit_latency_ns, which is s64 type, and they are used to > compare with 'last_residency', which is also s64 type, there is no "they are compared with" > reason to use u64 for promotion_time_ns/demotion_time_ns. "so change them both to be s64". > With this patch, > <idle>-0 [001] d..1. 523.578531: ladder_select_state: last_idx 8, last_residency -879453 > <idle>-0 [001] d..1. 523.578531: ladder_select_state: dev->last_residency_ns 10547, drv->states[last_idx].exit_latency_ns 890000 > <idle>-0 [001] d..1. 523.578532: ladder_select_state: demote , last_state->threshold.demotion_time_ns 890000 > <idle>-0 [001] d..1. 523.578532: ladder_select_state: ---> new state 7 > <idle>-0 [001] d..1. 523.580220: ladder_select_state: last_idx 7, last_residency -169629 > <idle>-0 [001] d..1. 523.580221: ladder_select_state: dev->last_residency_ns 310371, drv->states[last_idx].exit_latency_ns 480000 > <idle>-0 [001] d..1. 523.580221: ladder_select_state: demote , last_state->threshold.demotion_time_ns 480000 > <idle>-0 [001] d..1. 523.580222: ladder_select_state: ---> new state 6 > > Signed-off-by: Zhang Rui <rui.zhang@intel.com> > --- > drivers/cpuidle/governors/ladder.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/cpuidle/governors/ladder.c b/drivers/cpuidle/governors/ladder.c > index 8e9058c4ea63..fb61118aef37 100644 > --- a/drivers/cpuidle/governors/ladder.c > +++ b/drivers/cpuidle/governors/ladder.c > @@ -27,8 +27,8 @@ struct ladder_device_state { > struct { > u32 promotion_count; > u32 demotion_count; > - u64 promotion_time_ns; > - u64 demotion_time_ns; > + s64 promotion_time_ns; > + s64 demotion_time_ns; > } threshold; > struct { > int promotion_count; > --
diff --git a/drivers/cpuidle/governors/ladder.c b/drivers/cpuidle/governors/ladder.c index 8e9058c4ea63..fb61118aef37 100644 --- a/drivers/cpuidle/governors/ladder.c +++ b/drivers/cpuidle/governors/ladder.c @@ -27,8 +27,8 @@ struct ladder_device_state { struct { u32 promotion_count; u32 demotion_count; - u64 promotion_time_ns; - u64 demotion_time_ns; + s64 promotion_time_ns; + s64 demotion_time_ns; } threshold; struct { int promotion_count;
ladder_device_state.threshold.promotion_time_ns/demotion_time_ns are u64 type. In ladder_select_state(), variable 'last_residency', as calculated by last_residency = dev->last_residency_ns - drv->states[last_idx].exit_latency_ns are s64 type, and it can be negative value. When this happens, comparing between 'last_residency' and 'promotion_time_ns/demotion_time_ns' become bogus. As a result, the ladder governor promotes or stays with current state errornously. <idle>-0 [001] d..1. 151.893396: ladder_select_state: last_idx 7, last_residency -373033 <idle>-0 [001] d..1. 151.893399: ladder_select_state: dev->last_residency_ns 106967, drv->states[last_idx].exit_latency_ns 480000 <idle>-0 [001] d..1. 151.893402: ladder_select_state: promote, last_state->threshold.promotion_time_ns 480000 <idle>-0 [001] d..1. 151.893404: ladder_select_state: ---> new state 7 <idle>-0 [001] d..1. 151.893465: ladder_select_state: last_idx 7, last_residency -463800 <idle>-0 [001] d..1. 151.893467: ladder_select_state: dev->last_residency_ns 16200, drv->states[last_idx].exit_latency_ns 480000 <idle>-0 [001] d..1. 151.893468: ladder_select_state: promote, last_state->threshold.promotion_time_ns 480000 <idle>-0 [001] dn.1. 151.893470: ladder_select_state: ---> new state 8 Given that promotion_time_ns/demotion_time_ns are initialized with cpuidle_state.exit_latency_ns, which is s64 type, and they are used to compare with 'last_residency', which is also s64 type, there is no reason to use u64 for promotion_time_ns/demotion_time_ns. With this patch, <idle>-0 [001] d..1. 523.578531: ladder_select_state: last_idx 8, last_residency -879453 <idle>-0 [001] d..1. 523.578531: ladder_select_state: dev->last_residency_ns 10547, drv->states[last_idx].exit_latency_ns 890000 <idle>-0 [001] d..1. 523.578532: ladder_select_state: demote , last_state->threshold.demotion_time_ns 890000 <idle>-0 [001] d..1. 523.578532: ladder_select_state: ---> new state 7 <idle>-0 [001] d..1. 523.580220: ladder_select_state: last_idx 7, last_residency -169629 <idle>-0 [001] d..1. 523.580221: ladder_select_state: dev->last_residency_ns 310371, drv->states[last_idx].exit_latency_ns 480000 <idle>-0 [001] d..1. 523.580221: ladder_select_state: demote , last_state->threshold.demotion_time_ns 480000 <idle>-0 [001] d..1. 523.580222: ladder_select_state: ---> new state 6 Signed-off-by: Zhang Rui <rui.zhang@intel.com> --- drivers/cpuidle/governors/ladder.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)