Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755620AbdCTVwC (ORCPT ); Mon, 20 Mar 2017 17:52:02 -0400 Received: from cloudserver094114.home.net.pl ([79.96.170.134]:43738 "EHLO cloudserver094114.home.net.pl" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752673AbdCTVwA (ORCPT ); Mon, 20 Mar 2017 17:52:00 -0400 From: "Rafael J. Wysocki" To: Linux PM Cc: LKML , Peter Zijlstra , Srinivas Pandruvada , Viresh Kumar , Juri Lelli , Vincent Guittot , Patrick Bellasi , Joel Fernandes , Morten Rasmussen , Ingo Molnar Subject: [RFC][PATCH v2 2/2] cpufreq: schedutil: Avoid decreasing frequency of busy CPUs Date: Mon, 20 Mar 2017 22:46:27 +0100 Message-ID: <3300960.HE4b3sK4dn@aspire.rjw.lan> User-Agent: KMail/4.14.10 (Linux/4.10.0+; KDE/4.14.9; x86_64; ; ) In-Reply-To: <2185243.flNrap3qq1@aspire.rjw.lan> References: <4366682.tsferJN35u@aspire.rjw.lan> <2185243.flNrap3qq1@aspire.rjw.lan> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5590 Lines: 148 From: Rafael J. Wysocki The way the schedutil governor uses the PELT metric causes it to underestimate the CPU utilization in some cases. That can be easily demonstrated by running kernel compilation on a Sandy Bridge Intel processor, running turbostat in parallel with it and looking at the values written to the MSR_IA32_PERF_CTL register. Namely, the expected result would be that when all CPUs were 100% busy, all of them would be requested to run in the maximum P-state, but observation shows that this clearly isn't the case. The CPUs run in the maximum P-state for a while and then are requested to run slower and go back to the maximum P-state after a while again. That causes the actual frequency of the processor to visibly oscillate below the sustainable maximum in a jittery fashion which clearly is not desirable. To work around this issue use the observation that, from the schedutil governor's perspective, it does not make sense to decrease the frequency of a CPU that doesn't enter idle and avoid decreasing the frequency of busy CPUs. To that end, use the counter of idle calls in the timekeeping code. Namely, make the schedutil governor look at that counter for the current CPU every time before it is about to set a new frequency for that CPU's policy. If the counter has not changed since the previous iteration, the CPU has been busy for all that time and its frequency should not be decreased, so if the new frequency would be lower than the one set previously, the governor will skip the frequency update. Signed-off-by: Rafael J. Wysocki --- This is a slightly different approach (avoid decreasing frequency for busy CPUs instead of bumping if for them to the max upfront) and it works around the original problem too. I tried to address a few Peter's comments here and the result doesn't seem to be too heavy-wieght. Thanks, Rafael --- include/linux/tick.h | 1 + kernel/sched/cpufreq_schedutil.c | 28 ++++++++++++++++++++++++---- kernel/time/tick-sched.c | 12 ++++++++++++ 3 files changed, 37 insertions(+), 4 deletions(-) Index: linux-pm/kernel/sched/cpufreq_schedutil.c =================================================================== --- linux-pm.orig/kernel/sched/cpufreq_schedutil.c +++ linux-pm/kernel/sched/cpufreq_schedutil.c @@ -56,6 +56,9 @@ struct sugov_cpu { unsigned long iowait_boost; unsigned long iowait_boost_max; u64 last_update; +#ifdef CONFIG_NO_HZ_COMMON + unsigned long saved_idle_calls; +#endif /* The fields below are only needed when sharing a policy. */ unsigned long util; @@ -88,11 +91,28 @@ static bool sugov_should_update_freq(str return delta_ns >= sg_policy->freq_update_delay_ns; } -static void sugov_update_commit(struct sugov_policy *sg_policy, u64 time, - unsigned int next_freq) +#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(); + bool ret = idle_calls == sg_cpu->saved_idle_calls; + + sg_cpu->saved_idle_calls = idle_calls; + return ret; +} +#else +static inline bool sugov_cpu_is_busy(struct sugov_cpu *sg_cpu) { return false; } +#endif /* CONFIG_NO_HZ_COMMON */ + +static void sugov_update_commit(struct sugov_cpu *sg_cpu, + struct sugov_policy *sg_policy, + u64 time, unsigned int next_freq) { struct cpufreq_policy *policy = sg_policy->policy; + if (sugov_cpu_is_busy(sg_cpu) && next_freq < sg_policy->next_freq) + next_freq = sg_policy->next_freq; + if (policy->fast_switch_enabled) { if (sg_policy->next_freq == next_freq) { trace_cpu_frequency(policy->cur, smp_processor_id()); @@ -214,7 +234,7 @@ static void sugov_update_single(struct u sugov_iowait_boost(sg_cpu, &util, &max); next_f = get_next_freq(sg_policy, util, max); } - sugov_update_commit(sg_policy, time, next_f); + sugov_update_commit(sg_cpu, sg_policy, time, next_f); } static unsigned int sugov_next_freq_shared(struct sugov_cpu *sg_cpu) @@ -283,7 +303,7 @@ static void sugov_update_shared(struct u else next_f = sugov_next_freq_shared(sg_cpu); - sugov_update_commit(sg_policy, time, next_f); + sugov_update_commit(sg_cpu, sg_policy, time, next_f); } raw_spin_unlock(&sg_policy->update_lock); Index: linux-pm/include/linux/tick.h =================================================================== --- linux-pm.orig/include/linux/tick.h +++ linux-pm/include/linux/tick.h @@ -117,6 +117,7 @@ extern void tick_nohz_idle_enter(void); 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 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 */ Index: linux-pm/kernel/time/tick-sched.c =================================================================== --- linux-pm.orig/kernel/time/tick-sched.c +++ linux-pm/kernel/time/tick-sched.c @@ -993,6 +993,18 @@ ktime_t tick_nohz_get_sleep_length(void) return ts->sleep_length; } +/** + * tick_nohz_get_idle_calls - return the current idle calls counter value + * + * Called from the schedutil frequency scaling governor in scheduler context. + */ +unsigned long tick_nohz_get_idle_calls(void) +{ + struct tick_sched *ts = this_cpu_ptr(&tick_cpu_sched); + + return ts->idle_calls; +} + static void tick_nohz_account_idle_ticks(struct tick_sched *ts) { #ifndef CONFIG_VIRT_CPU_ACCOUNTING_NATIVE