| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAGETcx_aAynvykDSL4aue3zf5Pv7+hELUHQ=MWOzBbyZBPySDA@mail.gmail.com>
Date: Tue, 15 Nov 2022 14:35:51 -0800
From: Saravana Kannan <saravanak@...gle.com>
To: Sam Wu <wusamuel@...gle.com>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>,
Viresh Kumar <viresh.kumar@...aro.org>,
Ingo Molnar <mingo@...hat.com>,
Peter Zijlstra <peterz@...radead.org>,
Juri Lelli <juri.lelli@...hat.com>,
Vincent Guittot <vincent.guittot@...aro.org>,
Dietmar Eggemann <dietmar.eggemann@....com>,
Steven Rostedt <rostedt@...dmis.org>,
Ben Segall <bsegall@...gle.com>, Mel Gorman <mgorman@...e.de>,
Daniel Bristot de Oliveira <bristot@...hat.com>,
Valentin Schneider <vschneid@...hat.com>,
Lukasz Luba <lukasz.luba@....com>,
"Isaac J . Manjarres" <isaacmanjarres@...gle.com>,
kernel-team@...roid.com,
"Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1] Revert "cpufreq: schedutil: Move max CPU capacity to sugov_policy"
On Thu, Nov 10, 2022 at 11:57 AM Sam Wu <wusamuel@...gle.com> wrote:
>
> This reverts commit 6d5afdc97ea71958287364a1f1d07e59ef151b11.
>
> On a Pixel 6 device, it is observed that this commit increases
> latency by approximately 50ms, or 20%, in migrating a task
> that requires full CPU utilization from a LITTLE CPU to Fmax
> on a big CPU. Reverting this change restores the latency back
> to its original baseline value.
>
> Fixes: 6d5afdc97ea7 ("cpufreq: schedutil: Move max CPU capacity to sugov_policy")
> Cc: Lukasz Luba <lukasz.luba@....com>
> Cc: Saravana Kannan <saravanak@...gle.com>
> Cc: Isaac J. Manjarres <isaacmanjarres@...gle.com>
> Signed-off-by: Sam Wu <wusamuel@...gle.com>
Rafael, can we pick this up please?
Lukasz, no objections to the idea itself, but it's causing regression
and we'd like to revert and then fix it.
-Saravana
> ---
> kernel/sched/cpufreq_schedutil.c | 30 +++++++++++++++---------------
> 1 file changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> index 9161d1136d01..1207c78f85c1 100644
> --- a/kernel/sched/cpufreq_schedutil.c
> +++ b/kernel/sched/cpufreq_schedutil.c
> @@ -25,9 +25,6 @@ struct sugov_policy {
> unsigned int next_freq;
> unsigned int cached_raw_freq;
>
> - /* max CPU capacity, which is equal for all CPUs in freq. domain */
> - unsigned long max;
> -
> /* The next fields are only needed if fast switch cannot be used: */
> struct irq_work irq_work;
> struct kthread_work work;
> @@ -51,6 +48,7 @@ struct sugov_cpu {
>
> unsigned long util;
> unsigned long bw_dl;
> + unsigned long max;
>
> /* The field below is for single-CPU policies only: */
> #ifdef CONFIG_NO_HZ_COMMON
> @@ -160,6 +158,7 @@ static void sugov_get_util(struct sugov_cpu *sg_cpu)
> {
> struct rq *rq = cpu_rq(sg_cpu->cpu);
>
> + sg_cpu->max = arch_scale_cpu_capacity(sg_cpu->cpu);
> sg_cpu->bw_dl = cpu_bw_dl(rq);
> sg_cpu->util = effective_cpu_util(sg_cpu->cpu, cpu_util_cfs(sg_cpu->cpu),
> FREQUENCY_UTIL, NULL);
> @@ -254,7 +253,6 @@ static void sugov_iowait_boost(struct sugov_cpu *sg_cpu, u64 time,
> */
> static void sugov_iowait_apply(struct sugov_cpu *sg_cpu, u64 time)
> {
> - struct sugov_policy *sg_policy = sg_cpu->sg_policy;
> unsigned long boost;
>
> /* No boost currently required */
> @@ -282,8 +280,7 @@ static void sugov_iowait_apply(struct sugov_cpu *sg_cpu, u64 time)
> * sg_cpu->util is already in capacity scale; convert iowait_boost
> * into the same scale so we can compare.
> */
> - boost = sg_cpu->iowait_boost * sg_policy->max;
> - boost >>= SCHED_CAPACITY_SHIFT;
> + boost = (sg_cpu->iowait_boost * sg_cpu->max) >> SCHED_CAPACITY_SHIFT;
> boost = uclamp_rq_util_with(cpu_rq(sg_cpu->cpu), boost, NULL);
> if (sg_cpu->util < boost)
> sg_cpu->util = boost;
> @@ -340,7 +337,7 @@ static void sugov_update_single_freq(struct update_util_data *hook, u64 time,
> if (!sugov_update_single_common(sg_cpu, time, flags))
> return;
>
> - next_f = get_next_freq(sg_policy, sg_cpu->util, sg_policy->max);
> + next_f = get_next_freq(sg_policy, sg_cpu->util, sg_cpu->max);
> /*
> * Do not reduce the frequency if the CPU has not been idle
> * recently, as the reduction is likely to be premature then.
> @@ -376,7 +373,6 @@ static void sugov_update_single_perf(struct update_util_data *hook, u64 time,
> unsigned int flags)
> {
> struct sugov_cpu *sg_cpu = container_of(hook, struct sugov_cpu, update_util);
> - struct sugov_policy *sg_policy = sg_cpu->sg_policy;
> unsigned long prev_util = sg_cpu->util;
>
> /*
> @@ -403,8 +399,7 @@ static void sugov_update_single_perf(struct update_util_data *hook, u64 time,
> sg_cpu->util = prev_util;
>
> cpufreq_driver_adjust_perf(sg_cpu->cpu, map_util_perf(sg_cpu->bw_dl),
> - map_util_perf(sg_cpu->util),
> - sg_policy->max);
> + map_util_perf(sg_cpu->util), sg_cpu->max);
>
> sg_cpu->sg_policy->last_freq_update_time = time;
> }
> @@ -413,19 +408,25 @@ static unsigned int sugov_next_freq_shared(struct sugov_cpu *sg_cpu, u64 time)
> {
> struct sugov_policy *sg_policy = sg_cpu->sg_policy;
> struct cpufreq_policy *policy = sg_policy->policy;
> - unsigned long util = 0;
> + unsigned long util = 0, max = 1;
> unsigned int j;
>
> for_each_cpu(j, policy->cpus) {
> struct sugov_cpu *j_sg_cpu = &per_cpu(sugov_cpu, j);
> + unsigned long j_util, j_max;
>
> sugov_get_util(j_sg_cpu);
> sugov_iowait_apply(j_sg_cpu, time);
> + j_util = j_sg_cpu->util;
> + j_max = j_sg_cpu->max;
>
> - util = max(j_sg_cpu->util, util);
> + if (j_util * max > j_max * util) {
> + util = j_util;
> + max = j_max;
> + }
> }
>
> - return get_next_freq(sg_policy, util, sg_policy->max);
> + return get_next_freq(sg_policy, util, max);
> }
>
> static void
> @@ -751,7 +752,7 @@ static int sugov_start(struct cpufreq_policy *policy)
> {
> struct sugov_policy *sg_policy = policy->governor_data;
> void (*uu)(struct update_util_data *data, u64 time, unsigned int flags);
> - unsigned int cpu = cpumask_first(policy->cpus);
> + unsigned int cpu;
>
> sg_policy->freq_update_delay_ns = sg_policy->tunables->rate_limit_us * NSEC_PER_USEC;
> sg_policy->last_freq_update_time = 0;
> @@ -759,7 +760,6 @@ static int sugov_start(struct cpufreq_policy *policy)
> sg_policy->work_in_progress = false;
> sg_policy->limits_changed = false;
> sg_policy->cached_raw_freq = 0;
> - sg_policy->max = arch_scale_cpu_capacity(cpu);
>
> sg_policy->need_freq_update = cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS);
>
> --
> 2.38.1.431.g37b22c650d-goog
>
Powered by blists - more mailing lists