| Message ID | 20240606090050.327614-5-christian.loehle@arm.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | cpuidle: teo: fixes and improvements | expand |
On 06/06/2024 11:00, Christian Loehle wrote: > Since stopping the tick isn't free, add at least some minor constant > (1ms) for the threshold to stop the tick. Sounds pretty arbitrary to me? 'duration_ns' is either based on target_residency_ns or tick_nohz_get_sleep_length() or even set to TICK_NSEC/2. Does adding 1ms makes sense to all these cases? But then why 1ms? > Signed-off-by: Christian Loehle <christian.loehle@arm.com> > --- > drivers/cpuidle/governors/teo.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/cpuidle/governors/teo.c b/drivers/cpuidle/governors/teo.c > index 216d34747e3b..ca9422bbd8db 100644 > --- a/drivers/cpuidle/governors/teo.c > +++ b/drivers/cpuidle/governors/teo.c > @@ -622,10 +622,10 @@ static int teo_select(struct cpuidle_driver *drv, struct cpuidle_device *dev, > /* > * Allow the tick to be stopped unless the selected state is a polling > * one or the expected idle duration is shorter than the tick period > - * length. > + * length plus some constant (1ms) to account for stopping it. > */ > if ((!(drv->states[idx].flags & CPUIDLE_FLAG_POLLING) && > - duration_ns >= TICK_NSEC) || tick_nohz_tick_stopped()) > + duration_ns > NSEC_PER_MSEC + TICK_NSEC) || tick_nohz_tick_stopped()) > return idx; > > out_tick_state:
On 6/7/24 09:14, Dietmar Eggemann wrote: > On 06/06/2024 11:00, Christian Loehle wrote: >> Since stopping the tick isn't free, add at least some minor constant >> (1ms) for the threshold to stop the tick. > > Sounds pretty arbitrary to me? 'duration_ns' is either based on > target_residency_ns or tick_nohz_get_sleep_length() or even set to > TICK_NSEC/2. Does adding 1ms makes sense to all these cases? But then > why 1ms? It definitely is arbitrary, you're correct. Feel free to treat this as RFC. I'll probably just drop this from the serie and issue separately (to get the actual fixes merged more quickly). Anyway I'd like to hear comments on this. We are only interested in the cost of stopping the tick, which doesn't really depend on the selected state residency nor the expected sleep length. 1ms works fine (for me!!), making it depend on TICK_NSEC would be natural, too, but using TICK_NSEC is far too long for CONFIG_HZ=100 (and TICK_NSEC/2 too short for CONFIG_HZ=1000). The cost of stopping the tick depends on a number of factors and knowing all of them is probably not on the table anytime soon and until then I'd consider this an improvement over 0. > >> Signed-off-by: Christian Loehle <christian.loehle@arm.com> >> --- >> drivers/cpuidle/governors/teo.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/cpuidle/governors/teo.c b/drivers/cpuidle/governors/teo.c >> index 216d34747e3b..ca9422bbd8db 100644 >> --- a/drivers/cpuidle/governors/teo.c >> +++ b/drivers/cpuidle/governors/teo.c >> @@ -622,10 +622,10 @@ static int teo_select(struct cpuidle_driver *drv, struct cpuidle_device *dev, >> /* >> * Allow the tick to be stopped unless the selected state is a polling >> * one or the expected idle duration is shorter than the tick period >> - * length. >> + * length plus some constant (1ms) to account for stopping it. >> */ >> if ((!(drv->states[idx].flags & CPUIDLE_FLAG_POLLING) && >> - duration_ns >= TICK_NSEC) || tick_nohz_tick_stopped()) >> + duration_ns > NSEC_PER_MSEC + TICK_NSEC) || tick_nohz_tick_stopped()) >> return idx; >> >> out_tick_state: >
diff --git a/drivers/cpuidle/governors/teo.c b/drivers/cpuidle/governors/teo.c index 216d34747e3b..ca9422bbd8db 100644 --- a/drivers/cpuidle/governors/teo.c +++ b/drivers/cpuidle/governors/teo.c @@ -622,10 +622,10 @@ static int teo_select(struct cpuidle_driver *drv, struct cpuidle_device *dev, /* * Allow the tick to be stopped unless the selected state is a polling * one or the expected idle duration is shorter than the tick period - * length. + * length plus some constant (1ms) to account for stopping it. */ if ((!(drv->states[idx].flags & CPUIDLE_FLAG_POLLING) && - duration_ns >= TICK_NSEC) || tick_nohz_tick_stopped()) + duration_ns > NSEC_PER_MSEC + TICK_NSEC) || tick_nohz_tick_stopped()) return idx; out_tick_state:
Since stopping the tick isn't free, add at least some minor constant (1ms) for the threshold to stop the tick. Signed-off-by: Christian Loehle <christian.loehle@arm.com> --- drivers/cpuidle/governors/teo.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)