Hi All,
I am about 1/2 way through testing Feng's "hacky debug patch", let me know if I am wasting my time, and I'll abort. So far, it works fine.
The compiler complains:
kernel/sched/idle.c: In function ‘do_idle’: ./include/linux/typecheck.h:12:18: warning: comparison of distinct pointer types lacks a cast 12 | (void)(&__dummy == &__dummy2); \ | ^~ ./include/linux/compiler.h:78:42: note: in definition of macro ‘unlikely’ 78 | # define unlikely(x) __builtin_expect(!!(x), 0) | ^ ./include/linux/jiffies.h:106:3: note: in expansion of macro ‘typecheck’ 106 | typecheck(unsigned long, b) && \ | ^~~~~~~~~ ./include/linux/jiffies.h:154:35: note: in expansion of macro ‘time_after’ 154 | #define time_is_before_jiffies(a) time_after(jiffies, a) | ^~~~~~~~~~ kernel/sched/idle.c:274:15: note: in expansion of macro ‘time_is_before_jiffies’ 274 | if (unlikely(time_is_before_jiffies(expire))) {
Test 1: 347 Hertz work/sleep frequency on one CPU while others do virtually no load, but at around 0.02 hertz aggregate. Processor package power (Watts):
Kernel 5.17-rc3 + Feng patch (6 samples at 300 sec per): Average: 3.2 Min: 3.1 Max: 3.3
Kernel 5.17-rc3 (Stock) re-stated:
Stock: 5 samples @ 300 seconds per sample: average: 4.2 watts +31% minimum: 3.5 watts maximum: 4.9 watts
Kernel 5.17-rc3 with with b50db7095fe0 reverted. (Revert) re-stated:
Revert: 5 samples @ 300 seconds per sample: average: 3.2 watts minimum: 3.1 watts maximum: 3.2 watts
I also ran intel_pstate_tracer.py for 5 hours 33 minutes (20,000 seconds) on an idle system looking for long durations. (just being thorough.) There were 12 occurrences of durations longer than 6.1 seconds. Max: 6.8 seconds. (O.K.) I had expected about 3 seconds max, based on my understanding of the patch code.
Old results restated, but corrected for a stupid mistake:
Stock: 1712 occurrences of durations longer than 6.1 seconds Max: 303.6 seconds. (Bad)
Revert: 3 occurrences of durations longer than 6.1 seconds Max: 6.5 seconds (O.K.)
On Tue, Feb 22, 2022 at 10:04 AM Rafael J. Wysocki rafael@kernel.org wrote:
On Tue, Feb 22, 2022 at 8:41 AM Feng Tang feng.tang@intel.com wrote:
On Mon, Feb 14, 2022 at 07:17:24AM -0800, srinivas pandruvada wrote:
Hi Doug,
I think you use CONFIG_NO_HZ_FULL. Here we are getting callback from scheduler. Can we check that if scheduler woke up on those CPUs? We can run "trace-cmd -e sched" and check in kernel shark if there is similar gaps in activity.
Srinivas analyzed the scheduler trace data from trace-cmd, and thought is related with the cpufreq callback is not called timeley from scheduling events:
" I mean we ignore the callback when the target CPU is not a local CPU as we have to do IPI to adjust MSRs. This will happen many times when sched_wake will wake up a new CPU for the thread (we will get a callack for the target) but once the remote thread start executing "sched_switch", we will get a callback on local CPU, so we will adjust frequencies (provided 10ms interval from the last call).
From the trace file I see the scenario where it took 72sec between two
updates: CPU 2 34412.597161 busy=78 freq=3232653 34484.450725 busy=63 freq=2606793
There is periodic activity in between, related to active load balancing in scheduler (since last frequency was higher these small work will also run at higher frequency). But those threads are not CFS class, so scheduler callback will not be called for them.
So removing the patch removed a trigger which would have caused a sched_switch to a CFS task and call a cpufreq/intel_pstate callback.
And so this behavior needs to be restored for the time being which means reverting the problematic commit for 5.17 if possible.
It is unlikely that we'll get a proper fix before -rc7 and we still need to test it properly.
But calling for every class, will be too many callbacks and not sure we can even call for "stop" class, which these migration threads are using. "
Calling it for RT/deadline may not be a bad idea.
schedutil takes these classes into account when computing the utilization now (see effective_cpu_util()), so doing callbacks only for CFS seems insufficient.
Another way to avoid the issue at hand may be to prevent entering deep idle via PM QoS if the CPUs are running at high frequencies.
Following this direction, I made a hacky debug patch which should help to restore the previous behavior.
Doug, could you help to try it? thanks
It basically tries to make sure the cpufreq-update-util be called timely even for a silent system with very few interrupts (even from tick).
Thanks, Feng
From 6be5f5da66a847860b0b9924fbb09f93b2e2d6e6 Mon Sep 17 00:00:00 2001 From: Feng Tang feng.tang@intel.com Date: Tue, 22 Feb 2022 22:59:00 +0800 Subject: [PATCH] idle/intel-pstate: hacky debug patch to make sure the cpufreq_update_util callback being called timely in silent system
kernel/sched/idle.c | 10 ++++++++++ kernel/sched/sched.h | 13 +++++++++++++ 2 files changed, 23 insertions(+)
diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c index d17b0a5ce6ac..cc538acb3f1a 100644 --- a/kernel/sched/idle.c +++ b/kernel/sched/idle.c @@ -258,15 +258,25 @@ static void cpuidle_idle_call(void)
- Called with polling cleared.
*/ +DEFINE_PER_CPU(u64, last_util_update_time); /* in jiffies */ static void do_idle(void) { int cpu = smp_processor_id();
u64 expire; /* * Check if we need to update blocked load */ nohz_run_idle_balance(cpu);
+#ifdef CONFIG_X86_INTEL_PSTATE
Why? Doesn't this affect the other ccpufreq governors?
Yes, I have the same question.
expire = __this_cpu_read(last_util_update_time) + HZ * 3;
if (unlikely(time_is_before_jiffies(expire))) {
idle_update_util();
__this_cpu_write(last_util_update_time, get_jiffies_64());
}
+#endif
/* * If the arch has a polling bit, we maintain an invariant: *
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h index 0e66749486e7..2a8d87988d1f 100644 --- a/kernel/sched/sched.h +++ b/kernel/sched/sched.h @@ -2809,6 +2809,19 @@ static inline void cpufreq_update_util(struct rq *rq, unsigned int flags) if (data) data->func(data, rq_clock(rq), flags); }
+static inline void idle_update_util(void) +{
struct update_util_data *data;
struct rq *rq = cpu_rq(raw_smp_processor_id());
data = rcu_dereference_sched(*per_cpu_ptr(&cpufreq_update_util_data,
cpu_of(rq)));
if (data)
data->func(data, rq_clock(rq), 0);
+}
#else static inline void cpufreq_update_util(struct rq *rq, unsigned int flags) {}
#endif /* CONFIG_CPU_FREQ */