cpufreq: schedutil: Use idle_calls counter of the remote CPU
Subsystems (3)
PATCH RESEND 1/1 cpufreq: schedutil: Use idle_calls counter of the remote CPU
Since the recent remote cpufreq callback work, its possible that a cpufreq update is triggered from a remote CPU. For single policies however, the current code uses the local CPU when trying to determine if the remote sg_cpu entered idle or is busy. This is incorrect. To remedy this, compare with the nohz tick idle_calls counter of the remote CPU.
---
Just resending this which is cpufreq-related as requested by Rafael rebased on linus/master.
The other 2 patches in my last series which can go in independent of this one are:
https://patchwork.kernel.org/patch/10115395/
https://patchwork.kernel.org/patch/10115401/
I'm still waiting on scheduler maintainers to comment on those. Unfortunately, I haven't heard back anything yet since the last repost of those.
include/linux/tick.h | 1 + kernel/sched/cpufreq_schedutil.c | 2 +- kernel/time/tick-sched.c | 13 +++++++++++++ 3 files changed, 15 insertions(+), 1 deletion(-)
include/linux/tick.h +1 -0
@@ -119,6 +119,7 @@ extern void tick_nohz_idle_exit(void);extern void tick_nohz_irq_exit (void );extern ktime_t tick_nohz_get_sleep_length (void );extern unsigned long tick_nohz_get_idle_calls (void ); +extern unsigned long tick_nohz_get_idle_calls_cpu (int cpu);extern u64 get_cpu_idle_time_us (int cpu,u64 *last_update_time);extern u64 get_cpu_iowait_time_us (int cpu,u64 *last_update_time);#else /* !CONFIG_NO_HZ_COMMON */
kernel/sched/cpufreq_schedutil.c +1 -1
@@ -244,7 +244,7 @@ static void sugov_iowait_boost(struct sugov_cpu *sg_cpu, unsigned long *util,#ifdef CONFIG_NO_HZ_COMMON static bool sugov_cpu_is_busy (struct sugov_cpu *sg_cpu) { -unsigned long idle_calls =tick_nohz_get_idle_calls (); +unsigned long idle_calls =tick_nohz_get_idle_calls_cpu (sg_cpu->cpu);bool ret = idle_calls == sg_cpu->saved_idle_calls; sg_cpu->saved_idle_calls = idle_calls;
kernel/time/tick-sched.c +13 -0
@@ -985,6 +985,19 @@ ktime_t tick_nohz_get_sleep_length(void)return ts->sleep_length; } +/** +* tick_nohz_get_idle_calls_cpu - return the current idle calls counter value +* for a particular CPU. +* +* Called from the schedutil frequency scaling governor in scheduler context. +*/ +unsigned long tick_nohz_get_idle_calls_cpu (int cpu) +{ +struct tick_sched *ts =tick_get_tick_sched (cpu); + +return ts->idle_calls; +} +/** * tick_nohz_get_idle_calls - return the current idle calls counter value *
Signature · 1 line
2.15.1.504.g5279b80103-goog
Quoted 8 lines · Joel Fernandes
Since the recent remote cpufreq callback work, its possible that a cpufreq update is triggered from a remote CPU. For single policies however, the current code uses the local CPU when trying to determine if the remote
sg_cpuentered idle or is busy. This is incorrect. To remedy this, compare with the nohz tickidle_callscounter of the remote CPU.Acked-by Viresh Kumar <[email protected]>Signed-off-by Joel Fernandes <[email protected]>
Quoted 9 lines
---
Just resending this which is cpufreq-related as requested by Rafael rebased on linus/master.The other 2 patches in my last series which can go in independent of this one are:
https://patchwork.kernel.org/patch/10115395/
https://patchwork.kernel.org/patch/10115401/
I'm still waiting on scheduler maintainers to comment on those. Unfortunately, I haven't heard back anything yet since the last repost of those.
Both of us have been somewhat preoccupied with that whole kaiser/pti thing the past few weeks.
I have an absolutely stupid backlog :/
Quoted 11 lines · Peter Zijlstra
Quoted 8 lines · Joel Fernandes
Since the recent remote cpufreq callback work, its possible that a cpufreq update is triggered from a remote CPU. For single policies however, the current code uses the local CPU when trying to determine if the remote
sg_cpuentered idle or is busy. This is incorrect. To remedy this, compare with the nohz tickidle_callscounter of the remote CPU.Acked-by Viresh Kumar <[email protected]>Signed-off-by Joel Fernandes <[email protected]>
Sweet!
Quoted 5 lines
Quoted 5 lines
---
Just resending this which is cpufreq-related as requested by Rafael rebased on linus/master.
Quoted 8 lines
Quoted 5 lines
The other 2 patches in my last series which can go in independent of this one are:
https://patchwork.kernel.org/patch/10115395/
https://patchwork.kernel.org/patch/10115401/
I'm still waiting on scheduler maintainers to comment on those. Unfortunately, I haven't heard back anything yet since the last repost of those.Both of us have been somewhat preoccupied with that whole kaiser/pti thing the past few weeks.
I understand, thanks for taking time to look at it! Hopefully you're Ok with the second one as well
(https://patchwork.kernel.org/patch/10115401). And this cap aware one's been pretty beaten to death too:
https://patchwork.kernel.org/patch/10113337/ but let me know your objections if any.
Quoted 2 lines
I have an absolutely stupid backlog :/
I see. :/
I am thinking of spending more time reviewing fwiw and hopefully helping relieve some of that burden. Happy to help in any other way as well so let me/us know how we can help.
thanks,
- Joel
Quoted 8 lines · CET Joel Fernandes
Quoted 7 lines · Peter Zijlstra
Quoted 6 lines · Joel Fernandes
Since the recent remote cpufreq callback work, its possible that a cpufreq update is triggered from a remote CPU. For single policies however, the current code uses the local CPU when trying to determine if the remote
sg_cpuentered idle or is busy. This is incorrect. To remedy this, compare with the nohz tickidle_callscounter of the remote CPU.
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
Acked-by Viresh Kumar <[email protected]>Signed-off-by Joel Fernandes <[email protected]>Sweet!
OK, so any chance to get a Fixes: tag for the patch?
Thanks,
Rafael
Hi Rafael,
Quoted 9 lines · Rafael J. Wysocki
Quoted 8 lines · CET Joel Fernandes
Quoted 7 lines · Peter Zijlstra
Quoted 6 lines · Joel Fernandes
Since the recent remote cpufreq callback work, its possible that a cpufreq update is triggered from a remote CPU. For single policies however, the current code uses the local CPU when trying to determine if the remote
sg_cpuentered idle or is busy. This is incorrect. To remedy this, compare with the nohz tickidle_callscounter of the remote CPU.
Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
Acked-by Viresh Kumar <[email protected]>Signed-off-by Joel Fernandes <[email protected]>Sweet!
OK, so any chance to get a Fixes: tag for the patch?
Sure. Please find an inline patch below with all acks and fixes tag. thanks!
---8<---
From: Joel Fernandes <[email protected]>
Subject: [PATCH] cpufreq: schedutil: Use idle_calls counter of the remote CPU
Since the recent remote cpufreq callback work, its possible that a cpufreq update is triggered from a remote CPU. For single policies however, the current code uses the local CPU when trying to determine if the remote sg_cpu entered idle or is busy. This is incorrect. To remedy this, compare with the nohz tick idle_calls counter of the remote CPU.
--- include/linux/tick.h | 1 + kernel/sched/cpufreq_schedutil.c | 2 +- kernel/time/tick-sched.c | 13 +++++++++++++ 3 files changed, 15 insertions(+), 1 deletion(-)
include/linux/tick.h +1 -0
@@ -119,6 +119,7 @@ extern void tick_nohz_idle_exit(void);extern void tick_nohz_irq_exit (void );extern ktime_t tick_nohz_get_sleep_length (void );extern unsigned long tick_nohz_get_idle_calls (void ); +extern unsigned long tick_nohz_get_idle_calls_cpu (int cpu);extern u64 get_cpu_idle_time_us (int cpu,u64 *last_update_time);extern u64 get_cpu_iowait_time_us (int cpu,u64 *last_update_time);#else /* !CONFIG_NO_HZ_COMMON */
kernel/sched/cpufreq_schedutil.c +1 -1
@@ -244,7 +244,7 @@ static void sugov_iowait_boost(struct sugov_cpu *sg_cpu, unsigned long *util,#ifdef CONFIG_NO_HZ_COMMON static bool sugov_cpu_is_busy (struct sugov_cpu *sg_cpu) { -unsigned long idle_calls =tick_nohz_get_idle_calls (); +unsigned long idle_calls =tick_nohz_get_idle_calls_cpu (sg_cpu->cpu);bool ret = idle_calls == sg_cpu->saved_idle_calls; sg_cpu->saved_idle_calls = idle_calls;
kernel/time/tick-sched.c +13 -0
@@ -985,6 +985,19 @@ ktime_t tick_nohz_get_sleep_length(void)return ts->sleep_length; } +/** +* tick_nohz_get_idle_calls_cpu - return the current idle calls counter value +* for a particular CPU. +* +* Called from the schedutil frequency scaling governor in scheduler context. +*/ +unsigned long tick_nohz_get_idle_calls_cpu (int cpu) +{ +struct tick_sched *ts =tick_get_tick_sched (cpu); + +return ts->idle_calls; +} +/** * tick_nohz_get_idle_calls - return the current idle calls counter value *