* Re: [PATCH v2] schedutil: Allow cpufreq requests to be made even when kthread kicked
From: Rafael J. Wysocki @ 2018-05-22 12:22 UTC (permalink / raw)
To: Viresh Kumar, Joel Fernandes (Google.)
Cc: Rafael J. Wysocki, Linux Kernel Mailing List,
Joel Fernandes (Google), Rafael J . Wysocki, Peter Zijlstra,
Ingo Molnar, Patrick Bellasi, Juri Lelli, Luca Abeni, Todd Kjos,
Claudio Scordino, kernel-team, Linux PM
In-Reply-To: <CAJZ5v0hP+gCiLa+6YvKHaj7u-DuRfmVq_vjFRMj6CnVktJRxVg@mail.gmail.com>
On Tuesday, May 22, 2018 1:42:05 PM CEST Rafael J. Wysocki wrote:
> On Tue, May 22, 2018 at 1:38 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > On 22-05-18, 13:31, Rafael J. Wysocki wrote:
> >> So below is my (compiled-only) version of the $subject patch, obviously based
> >> on the Joel's work.
> >>
> >> Roughly, what it does is to move the fast_switch_enabled path entirely to
> >> sugov_update_single() and take the spinlock around sugov_update_commit()
> >> in the one-CPU case too.
[cut]
> >
> > Why do you assume that fast switch isn't possible in shared policy
> > cases ? It infact is already enabled for few drivers.
I hope that fast_switch is not used with devfs_possible_from_any_cpu set in the
one-CPU policy case, as that looks racy even without any patching.
> OK, so the fast_switch thing needs to be left outside of the spinlock
> in the single case only. Fair enough.
That would be something like the patch below (again, compiled-only).
---
kernel/sched/cpufreq_schedutil.c | 67 +++++++++++++++++++++++++++------------
1 file changed, 47 insertions(+), 20 deletions(-)
Index: linux-pm/kernel/sched/cpufreq_schedutil.c
===================================================================
--- linux-pm.orig/kernel/sched/cpufreq_schedutil.c
+++ linux-pm/kernel/sched/cpufreq_schedutil.c
@@ -92,9 +92,6 @@ static bool sugov_should_update_freq(str
!cpufreq_can_do_remote_dvfs(sg_policy->policy))
return false;
- if (sg_policy->work_in_progress)
- return false;
-
if (unlikely(sg_policy->need_freq_update))
return true;
@@ -103,25 +100,41 @@ 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)
+static bool sugov_update_next_freq(struct sugov_policy *sg_policy, u64 time,
+ unsigned int next_freq)
{
- struct cpufreq_policy *policy = sg_policy->policy;
-
if (sg_policy->next_freq == next_freq)
- return;
+ return false;
sg_policy->next_freq = next_freq;
sg_policy->last_freq_update_time = time;
- if (policy->fast_switch_enabled) {
- next_freq = cpufreq_driver_fast_switch(policy, next_freq);
- if (!next_freq)
- return;
+ return true;
+}
- policy->cur = next_freq;
- trace_cpu_frequency(next_freq, smp_processor_id());
- } else {
+static void sugov_fast_switch(struct sugov_policy *sg_policy, u64 time,
+ unsigned int next_freq)
+{
+ struct cpufreq_policy *policy = sg_policy->policy;
+
+ if (!sugov_update_next_freq(sg_policy, time, next_freq))
+ return;
+
+ next_freq = cpufreq_driver_fast_switch(policy, next_freq);
+ if (!next_freq)
+ return;
+
+ policy->cur = next_freq;
+ trace_cpu_frequency(next_freq, smp_processor_id());
+}
+
+static void sugov_update_commit(struct sugov_policy *sg_policy, u64 time,
+ unsigned int next_freq)
+{
+ if (!sugov_update_next_freq(sg_policy, time, next_freq))
+ return;
+
+ if (!sg_policy->work_in_progress) {
sg_policy->work_in_progress = true;
irq_work_queue(&sg_policy->irq_work);
}
@@ -307,7 +320,13 @@ static void sugov_update_single(struct u
sg_policy->cached_raw_freq = 0;
}
- sugov_update_commit(sg_policy, time, next_f);
+ if (sg_policy->policy->fast_switch_enabled) {
+ sugov_fast_switch(sg_policy, time, next_f);
+ } else {
+ raw_spin_lock(&sg_policy->update_lock);
+ sugov_update_commit(sg_policy, time, next_f);
+ raw_spin_unlock(&sg_policy->update_lock);
+ }
}
static unsigned int sugov_next_freq_shared(struct sugov_cpu *sg_cpu, u64 time)
@@ -367,7 +386,10 @@ sugov_update_shared(struct update_util_d
if (sugov_should_update_freq(sg_policy, time)) {
next_f = sugov_next_freq_shared(sg_cpu, time);
- sugov_update_commit(sg_policy, time, next_f);
+ if (sg_policy->policy->fast_switch_enabled)
+ sugov_fast_switch(sg_policy, time, next_f);
+ else
+ sugov_update_commit(sg_policy, time, next_f);
}
raw_spin_unlock(&sg_policy->update_lock);
@@ -376,13 +398,18 @@ sugov_update_shared(struct update_util_d
static void sugov_work(struct kthread_work *work)
{
struct sugov_policy *sg_policy = container_of(work, struct sugov_policy, work);
+ unsigned int next_freq;
+ unsigned long flags;
+
+ raw_spin_lock_irqsave(&sg_policy->update_lock, flags);
+ next_freq = sg_policy->next_freq;
+ sg_policy->work_in_progress = false;
+ raw_spin_unlock_irqrestore(&sg_policy->update_lock, flags);
mutex_lock(&sg_policy->work_lock);
- __cpufreq_driver_target(sg_policy->policy, sg_policy->next_freq,
+ __cpufreq_driver_target(sg_policy->policy, next_freq,
CPUFREQ_RELATION_L);
mutex_unlock(&sg_policy->work_lock);
-
- sg_policy->work_in_progress = false;
}
static void sugov_irq_work(struct irq_work *irq_work)
^ permalink raw reply
* Re: [PATCH v2] schedutil: Allow cpufreq requests to be made even when kthread kicked
From: Rafael J. Wysocki @ 2018-05-22 11:42 UTC (permalink / raw)
To: Viresh Kumar
Cc: Rafael J. Wysocki, Joel Fernandes (Google.),
Linux Kernel Mailing List, Joel Fernandes (Google),
Rafael J . Wysocki, Peter Zijlstra, Ingo Molnar, Patrick Bellasi,
Juri Lelli, Luca Abeni, Todd Kjos, Claudio Scordino, kernel-team,
Linux PM
In-Reply-To: <20180522113844.5rz3skjeck57arft@vireshk-i7>
On Tue, May 22, 2018 at 1:38 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 22-05-18, 13:31, Rafael J. Wysocki wrote:
>> So below is my (compiled-only) version of the $subject patch, obviously based
>> on the Joel's work.
>>
>> Roughly, what it does is to move the fast_switch_enabled path entirely to
>> sugov_update_single() and take the spinlock around sugov_update_commit()
>> in the one-CPU case too.
>>
>> ---
>> kernel/sched/cpufreq_schedutil.c | 57 ++++++++++++++++++++++++++-------------
>> 1 file changed, 38 insertions(+), 19 deletions(-)
>>
>> Index: linux-pm/kernel/sched/cpufreq_schedutil.c
>> ===================================================================
>> --- linux-pm.orig/kernel/sched/cpufreq_schedutil.c
>> +++ linux-pm/kernel/sched/cpufreq_schedutil.c
>> @@ -92,9 +92,6 @@ static bool sugov_should_update_freq(str
>> !cpufreq_can_do_remote_dvfs(sg_policy->policy))
>> return false;
>>
>> - if (sg_policy->work_in_progress)
>> - return false;
>> -
>> if (unlikely(sg_policy->need_freq_update))
>> return true;
>>
>> @@ -103,25 +100,25 @@ 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)
>> +static bool sugov_update_next_freq(struct sugov_policy *sg_policy, u64 time,
>> + unsigned int next_freq)
>> {
>> - struct cpufreq_policy *policy = sg_policy->policy;
>> -
>> if (sg_policy->next_freq == next_freq)
>> - return;
>> + return false;
>>
>> sg_policy->next_freq = next_freq;
>> sg_policy->last_freq_update_time = time;
>>
>> - if (policy->fast_switch_enabled) {
>> - next_freq = cpufreq_driver_fast_switch(policy, next_freq);
>> - if (!next_freq)
>> - return;
>> + return true;
>> +}
>>
>> - policy->cur = next_freq;
>> - trace_cpu_frequency(next_freq, smp_processor_id());
>> - } else {
>> +static void sugov_update_commit(struct sugov_policy *sg_policy, u64 time,
>> + unsigned int next_freq)
>> +{
>> + if (!sugov_update_next_freq(sg_policy, time, next_freq))
>> + return;
>> +
>> + if (!sg_policy->work_in_progress) {
>> sg_policy->work_in_progress = true;
>> irq_work_queue(&sg_policy->irq_work);
>> }
>> @@ -277,6 +274,7 @@ static void sugov_update_single(struct u
>> {
>> struct sugov_cpu *sg_cpu = container_of(hook, struct sugov_cpu, update_util);
>> struct sugov_policy *sg_policy = sg_cpu->sg_policy;
>> + struct cpufreq_policy *policy = sg_policy->policy;
>> unsigned long util, max;
>> unsigned int next_f;
>> bool busy;
>> @@ -307,7 +305,23 @@ static void sugov_update_single(struct u
>> sg_policy->cached_raw_freq = 0;
>> }
>>
>> - sugov_update_commit(sg_policy, time, next_f);
>> + if (policy->fast_switch_enabled) {
>
> Why do you assume that fast switch isn't possible in shared policy
> cases ? It infact is already enabled for few drivers.
OK, so the fast_switch thing needs to be left outside of the spinlock
in the single case only. Fair enough.
^ permalink raw reply
* [PATCH] cpufreq / CPPC: Add cpuinfo_cur_freq support for CPPC
From: George Cherian @ 2018-05-22 11:42 UTC (permalink / raw)
To: linux-kernel, linux-pm; +Cc: rjw, viresh.kumar, George Cherian
Per Section 8.4.7.1.3 of ACPI 6.2, The platform provides performance
feedback via set of performance counters. To determine the actual
performance level delivered over time, OSPM may read a set of
performance counters from the Reference Performance Counter Register
and the Delivered Performance Counter Register.
OSPM calculates the delivered performance over a given time period by
taking a beginning and ending snapshot of both the reference and
delivered performance counters, and calculating:
delivered_perf = reference_perf X (delta of delivered_perf counter / delta of reference_perf counter).
Implement the above and hook this to the cpufreq->get method.
Signed-off-by: George Cherian <george.cherian@cavium.com>
---
drivers/cpufreq/cppc_cpufreq.c | 44 ++++++++++++++++++++++++++++++++++++++++++
1 file changed, 44 insertions(+)
diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
index b15115a..a046915 100644
--- a/drivers/cpufreq/cppc_cpufreq.c
+++ b/drivers/cpufreq/cppc_cpufreq.c
@@ -240,10 +240,54 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
return ret;
}
+static int cppc_get_rate_from_fbctrs(struct cppc_perf_fb_ctrs fb_ctrs_t0,
+ struct cppc_perf_fb_ctrs fb_ctrs_t1)
+{
+ u64 delta_reference, delta_delivered;
+ u64 reference_perf, ratio;
+
+ reference_perf = fb_ctrs_t0.reference_perf;
+ if (fb_ctrs_t1.reference > fb_ctrs_t0.reference)
+ delta_reference = fb_ctrs_t1.reference - fb_ctrs_t0.reference;
+ else /* Counters would have wrapped-around */
+ delta_reference = ((u64)(~((u64)0)) - fb_ctrs_t0.reference) +
+ fb_ctrs_t1.reference;
+
+ if (fb_ctrs_t1.delivered > fb_ctrs_t0.delivered)
+ delta_delivered = fb_ctrs_t1.delivered - fb_ctrs_t0.delivered;
+ else /* Counters would have wrapped-around */
+ delta_delivered = ((u64)(~((u64)0)) - fb_ctrs_t0.delivered) +
+ fb_ctrs_t1.delivered;
+
+ if (delta_reference) /* Check to avoid divide-by zero */
+ ratio = (delta_delivered * 1000) / delta_reference;
+ else
+ return -EINVAL;
+
+ return (reference_perf * ratio) / 1000;
+}
+
+static unsigned int cppc_cpufreq_get_rate(unsigned int cpunum)
+{
+ struct cppc_perf_fb_ctrs fb_ctrs_t0 = {0}, fb_ctrs_t1 = {0};
+ int ret;
+
+ ret = cppc_get_perf_ctrs(cpunum, &fb_ctrs_t0);
+ if (ret)
+ return ret;
+
+ ret = cppc_get_perf_ctrs(cpunum, &fb_ctrs_t1);
+ if (ret)
+ return ret;
+
+ return cppc_get_rate_from_fbctrs(fb_ctrs_t0, fb_ctrs_t1);
+}
+
static struct cpufreq_driver cppc_cpufreq_driver = {
.flags = CPUFREQ_CONST_LOOPS,
.verify = cppc_verify_policy,
.target = cppc_cpufreq_set_target,
+ .get = cppc_cpufreq_get_rate,
.init = cppc_cpufreq_cpu_init,
.stop_cpu = cppc_cpufreq_stop_cpu,
.name = "cppc_cpufreq",
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH] PM / core: Fix direct_complete handling for devices with no callbacks
From: Ulf Hansson @ 2018-05-22 11:41 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Linux PM, LKML, Thomas Martitz, Lukas Wunner, Greg Kroah-Hartman
In-Reply-To: <6238546.EnOBWIOf9o@aspire.rjw.lan>
On 22 May 2018 at 13:02, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Commit 08810a4119aa (PM / core: Add NEVER_SKIP and SMART_PREPARE
> driver flags) inadvertently prevented the power.direct_complete flag
> from being set for devices without PM callbacks and with disabled
> runtime PM which also prevents power.direct_complete from being set
> for their parents. That led to problems including a resume crash on
> HP ZBook 14u.
>
> Restore the previous behavior by causing power.direct_complete to be
> set for those devices again, but do that in a more direct way to
> avoid overlooking that case in the future.
>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=199693
> Fixes: 08810a4119aa (PM / core: Add NEVER_SKIP and SMART_PREPARE driver flags)
> Reported-by: Thomas Martitz <kugel@rockbox.org>
> Tested-by: Thomas Martitz <kugel@rockbox.org>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
It seems like the resume path of HP ZBook 14u is kind of fragile, in
case it *requires* dev->power.direct_complete to be set for devices
like these. And that makes me wonder, that perhaps we should try to
address that issue as well, no?
In either way:
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
Kind regards
Uffe
> ---
> drivers/base/power/main.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> Index: linux-pm/drivers/base/power/main.c
> ===================================================================
> --- linux-pm.orig/drivers/base/power/main.c
> +++ linux-pm/drivers/base/power/main.c
> @@ -1920,10 +1920,8 @@ static int device_prepare(struct device
>
> dev->power.wakeup_path = false;
>
> - if (dev->power.no_pm_callbacks) {
> - ret = 1; /* Let device go direct_complete */
> + if (dev->power.no_pm_callbacks)
> goto unlock;
> - }
>
> if (dev->pm_domain)
> callback = dev->pm_domain->ops.prepare;
> @@ -1957,7 +1955,8 @@ unlock:
> */
> spin_lock_irq(&dev->power.lock);
> dev->power.direct_complete = state.event == PM_EVENT_SUSPEND &&
> - pm_runtime_suspended(dev) && ret > 0 &&
> + ((pm_runtime_suspended(dev) && ret > 0) ||
> + dev->power.no_pm_callbacks) &&
> !dev_pm_test_driver_flags(dev, DPM_FLAG_NEVER_SKIP);
> spin_unlock_irq(&dev->power.lock);
> return 0;
>
^ permalink raw reply
* Re: [PATCH v2] schedutil: Allow cpufreq requests to be made even when kthread kicked
From: Viresh Kumar @ 2018-05-22 11:38 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Joel Fernandes (Google.), linux-kernel, Joel Fernandes (Google),
Rafael J . Wysocki, Peter Zijlstra, Ingo Molnar, Patrick Bellasi,
Juri Lelli, Luca Abeni, Todd Kjos, claudio, kernel-team, linux-pm
In-Reply-To: <6163159.YL3rkaEOau@aspire.rjw.lan>
On 22-05-18, 13:31, Rafael J. Wysocki wrote:
> So below is my (compiled-only) version of the $subject patch, obviously based
> on the Joel's work.
>
> Roughly, what it does is to move the fast_switch_enabled path entirely to
> sugov_update_single() and take the spinlock around sugov_update_commit()
> in the one-CPU case too.
>
> ---
> kernel/sched/cpufreq_schedutil.c | 57 ++++++++++++++++++++++++++-------------
> 1 file changed, 38 insertions(+), 19 deletions(-)
>
> Index: linux-pm/kernel/sched/cpufreq_schedutil.c
> ===================================================================
> --- linux-pm.orig/kernel/sched/cpufreq_schedutil.c
> +++ linux-pm/kernel/sched/cpufreq_schedutil.c
> @@ -92,9 +92,6 @@ static bool sugov_should_update_freq(str
> !cpufreq_can_do_remote_dvfs(sg_policy->policy))
> return false;
>
> - if (sg_policy->work_in_progress)
> - return false;
> -
> if (unlikely(sg_policy->need_freq_update))
> return true;
>
> @@ -103,25 +100,25 @@ 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)
> +static bool sugov_update_next_freq(struct sugov_policy *sg_policy, u64 time,
> + unsigned int next_freq)
> {
> - struct cpufreq_policy *policy = sg_policy->policy;
> -
> if (sg_policy->next_freq == next_freq)
> - return;
> + return false;
>
> sg_policy->next_freq = next_freq;
> sg_policy->last_freq_update_time = time;
>
> - if (policy->fast_switch_enabled) {
> - next_freq = cpufreq_driver_fast_switch(policy, next_freq);
> - if (!next_freq)
> - return;
> + return true;
> +}
>
> - policy->cur = next_freq;
> - trace_cpu_frequency(next_freq, smp_processor_id());
> - } else {
> +static void sugov_update_commit(struct sugov_policy *sg_policy, u64 time,
> + unsigned int next_freq)
> +{
> + if (!sugov_update_next_freq(sg_policy, time, next_freq))
> + return;
> +
> + if (!sg_policy->work_in_progress) {
> sg_policy->work_in_progress = true;
> irq_work_queue(&sg_policy->irq_work);
> }
> @@ -277,6 +274,7 @@ static void sugov_update_single(struct u
> {
> struct sugov_cpu *sg_cpu = container_of(hook, struct sugov_cpu, update_util);
> struct sugov_policy *sg_policy = sg_cpu->sg_policy;
> + struct cpufreq_policy *policy = sg_policy->policy;
> unsigned long util, max;
> unsigned int next_f;
> bool busy;
> @@ -307,7 +305,23 @@ static void sugov_update_single(struct u
> sg_policy->cached_raw_freq = 0;
> }
>
> - sugov_update_commit(sg_policy, time, next_f);
> + if (policy->fast_switch_enabled) {
Why do you assume that fast switch isn't possible in shared policy
cases ? It infact is already enabled for few drivers.
--
viresh
^ permalink raw reply
* Re: [PATCH v2] schedutil: Allow cpufreq requests to be made even when kthread kicked
From: Rafael J. Wysocki @ 2018-05-22 11:31 UTC (permalink / raw)
To: Viresh Kumar, Joel Fernandes (Google.)
Cc: linux-kernel, Joel Fernandes (Google), Rafael J . Wysocki,
Peter Zijlstra, Ingo Molnar, Patrick Bellasi, Juri Lelli,
Luca Abeni, Todd Kjos, claudio, kernel-team, linux-pm
In-Reply-To: <20180522105429.yn2omuz63oa2w3ez@vireshk-i7>
On Tuesday, May 22, 2018 12:54:29 PM CEST Viresh Kumar wrote:
> On 22-05-18, 12:50, Rafael J. Wysocki wrote:
> > Ugly indeed.
>
> Hehe. I was thinking, maybe we can write wrapper helpers around lock/unlock
> which are stored as pointers in sg_policy. So that those are only set to
> non-NULL values (or non-Noop routines) for slow-switching single policy or
> any-switching shared policy systems. Then we can get rid of such conditional
> locking attempts :)
>
>
So below is my (compiled-only) version of the $subject patch, obviously based
on the Joel's work.
Roughly, what it does is to move the fast_switch_enabled path entirely to
sugov_update_single() and take the spinlock around sugov_update_commit()
in the one-CPU case too.
---
kernel/sched/cpufreq_schedutil.c | 57 ++++++++++++++++++++++++++-------------
1 file changed, 38 insertions(+), 19 deletions(-)
Index: linux-pm/kernel/sched/cpufreq_schedutil.c
===================================================================
--- linux-pm.orig/kernel/sched/cpufreq_schedutil.c
+++ linux-pm/kernel/sched/cpufreq_schedutil.c
@@ -92,9 +92,6 @@ static bool sugov_should_update_freq(str
!cpufreq_can_do_remote_dvfs(sg_policy->policy))
return false;
- if (sg_policy->work_in_progress)
- return false;
-
if (unlikely(sg_policy->need_freq_update))
return true;
@@ -103,25 +100,25 @@ 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)
+static bool sugov_update_next_freq(struct sugov_policy *sg_policy, u64 time,
+ unsigned int next_freq)
{
- struct cpufreq_policy *policy = sg_policy->policy;
-
if (sg_policy->next_freq == next_freq)
- return;
+ return false;
sg_policy->next_freq = next_freq;
sg_policy->last_freq_update_time = time;
- if (policy->fast_switch_enabled) {
- next_freq = cpufreq_driver_fast_switch(policy, next_freq);
- if (!next_freq)
- return;
+ return true;
+}
- policy->cur = next_freq;
- trace_cpu_frequency(next_freq, smp_processor_id());
- } else {
+static void sugov_update_commit(struct sugov_policy *sg_policy, u64 time,
+ unsigned int next_freq)
+{
+ if (!sugov_update_next_freq(sg_policy, time, next_freq))
+ return;
+
+ if (!sg_policy->work_in_progress) {
sg_policy->work_in_progress = true;
irq_work_queue(&sg_policy->irq_work);
}
@@ -277,6 +274,7 @@ static void sugov_update_single(struct u
{
struct sugov_cpu *sg_cpu = container_of(hook, struct sugov_cpu, update_util);
struct sugov_policy *sg_policy = sg_cpu->sg_policy;
+ struct cpufreq_policy *policy = sg_policy->policy;
unsigned long util, max;
unsigned int next_f;
bool busy;
@@ -307,7 +305,23 @@ static void sugov_update_single(struct u
sg_policy->cached_raw_freq = 0;
}
- sugov_update_commit(sg_policy, time, next_f);
+ if (policy->fast_switch_enabled) {
+ if (!sugov_update_next_freq(sg_policy, time, next_f))
+ return;
+
+ next_f = cpufreq_driver_fast_switch(policy, next_f);
+ if (!next_f)
+ return;
+
+ policy->cur = next_f;
+ trace_cpu_frequency(next_f, smp_processor_id());
+ } else {
+ raw_spin_lock(&sg_policy->update_lock);
+
+ sugov_update_commit(sg_policy, time, next_f);
+
+ raw_spin_unlock(&sg_policy->update_lock);
+ }
}
static unsigned int sugov_next_freq_shared(struct sugov_cpu *sg_cpu, u64 time)
@@ -376,13 +390,18 @@ sugov_update_shared(struct update_util_d
static void sugov_work(struct kthread_work *work)
{
struct sugov_policy *sg_policy = container_of(work, struct sugov_policy, work);
+ unsigned int next_freq;
+ unsigned long flags;
+
+ raw_spin_lock_irqsave(&sg_policy->update_lock, flags);
+ next_freq = sg_policy->next_freq;
+ sg_policy->work_in_progress = false;
+ raw_spin_unlock_irqrestore(&sg_policy->update_lock, flags);
mutex_lock(&sg_policy->work_lock);
- __cpufreq_driver_target(sg_policy->policy, sg_policy->next_freq,
+ __cpufreq_driver_target(sg_policy->policy, next_freq,
CPUFREQ_RELATION_L);
mutex_unlock(&sg_policy->work_lock);
-
- sg_policy->work_in_progress = false;
}
static void sugov_irq_work(struct irq_work *irq_work)
^ permalink raw reply
* [PATCH] cpufreq: Add Kryo CPU scaling driver
From: Ilia Lin @ 2018-05-22 11:29 UTC (permalink / raw)
To: viresh.kumar
Cc: linux-clk, devicetree, linux-kernel, linux-pm, linux-arm-msm,
linux-soc, linux-arm-kernel
In-Reply-To: <1526729701-8589-1-git-send-email-ilialin@codeaurora.org>
In Certain QCOM SoCs like apq8096 and msm8996 that have KRYO processors,
the CPU frequency subset and voltage value of each OPP varies
based on the silicon variant in use. Qualcomm Process Voltage Scaling Tables
defines the voltage and frequency value based on the msm-id in SMEM
and speedbin blown in the efuse combination.
The qcom-cpufreq-kryo driver reads the msm-id and efuse value from the SoC
to provide the OPP framework with required information.
This is used to determine the voltage and frequency value for each OPP of
operating-points-v2 table when it is parsed by the OPP framework.
Signed-off-by: Ilia Lin <ilialin@codeaurora.org>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/cpufreq/Kconfig.arm | 10 +++
drivers/cpufreq/Makefile | 1 +
drivers/cpufreq/cpufreq-dt-platdev.c | 3 +
drivers/cpufreq/qcom-cpufreq-kryo.c | 163 +++++++++++++++++++++++++++++++++++
4 files changed, 177 insertions(+)
create mode 100644 drivers/cpufreq/qcom-cpufreq-kryo.c
diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
index de55c7d..0bfd40e 100644
--- a/drivers/cpufreq/Kconfig.arm
+++ b/drivers/cpufreq/Kconfig.arm
@@ -124,6 +124,16 @@ config ARM_OMAP2PLUS_CPUFREQ
depends on ARCH_OMAP2PLUS
default ARCH_OMAP2PLUS
+config ARM_QCOM_CPUFREQ_KRYO
+ bool "Qualcomm Kryo based CPUFreq"
+ depends on QCOM_QFPROM
+ depends on QCOM_SMEM
+ select PM_OPP
+ help
+ This adds the CPUFreq driver for Qualcomm Kryo SoC based boards.
+
+ If in doubt, say N.
+
config ARM_S3C_CPUFREQ
bool
help
diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
index 8d24ade..fb4a2ec 100644
--- a/drivers/cpufreq/Makefile
+++ b/drivers/cpufreq/Makefile
@@ -65,6 +65,7 @@ obj-$(CONFIG_MACH_MVEBU_V7) += mvebu-cpufreq.o
obj-$(CONFIG_ARM_OMAP2PLUS_CPUFREQ) += omap-cpufreq.o
obj-$(CONFIG_ARM_PXA2xx_CPUFREQ) += pxa2xx-cpufreq.o
obj-$(CONFIG_PXA3xx) += pxa3xx-cpufreq.o
+obj-$(CONFIG_ARM_QCOM_CPUFREQ_KRYO) += qcom-cpufreq-kryo.o
obj-$(CONFIG_ARM_S3C2410_CPUFREQ) += s3c2410-cpufreq.o
obj-$(CONFIG_ARM_S3C2412_CPUFREQ) += s3c2412-cpufreq.o
obj-$(CONFIG_ARM_S3C2416_CPUFREQ) += s3c2416-cpufreq.o
diff --git a/drivers/cpufreq/cpufreq-dt-platdev.c b/drivers/cpufreq/cpufreq-dt-platdev.c
index 3b585e4..77d6ab8 100644
--- a/drivers/cpufreq/cpufreq-dt-platdev.c
+++ b/drivers/cpufreq/cpufreq-dt-platdev.c
@@ -118,6 +118,9 @@
{ .compatible = "nvidia,tegra124", },
+ { .compatible = "qcom,apq8096", },
+ { .compatible = "qcom,msm8996", },
+
{ .compatible = "st,stih407", },
{ .compatible = "st,stih410", },
diff --git a/drivers/cpufreq/qcom-cpufreq-kryo.c b/drivers/cpufreq/qcom-cpufreq-kryo.c
new file mode 100644
index 0000000..c5bb070
--- /dev/null
+++ b/drivers/cpufreq/qcom-cpufreq-kryo.c
@@ -0,0 +1,163 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018, The Linux Foundation. All rights reserved.
+ */
+
+/*
+ * In Certain QCOM SoCs like apq8096 and msm8996 that have KRYO processors,
+ * the CPU frequency subset and voltage value of each OPP varies
+ * based on the silicon variant in use. Qualcomm Process Voltage Scaling Tables
+ * defines the voltage and frequency value based on the msm-id in SMEM
+ * and speedbin blown in the efuse combination.
+ * The qcom-cpufreq-kryo driver reads the msm-id and efuse value from the SoC
+ * to provide the OPP framework with required information.
+ * This is used to determine the voltage and frequency value for each OPP of
+ * operating-points-v2 table when it is parsed by the OPP framework.
+ */
+
+#include <linux/cpu.h>
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/nvmem-consumer.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pm_opp.h>
+#include <linux/slab.h>
+#include <linux/soc/qcom/smem.h>
+
+#define MSM_ID_SMEM 137
+
+enum _msm_id {
+ MSM8996V3 = 0xF6ul,
+ APQ8096V3 = 0x123ul,
+ MSM8996SG = 0x131ul,
+ APQ8096SG = 0x138ul,
+};
+
+enum _msm8996_version {
+ MSM8996_V3,
+ MSM8996_SG,
+ NUM_OF_MSM8996_VERSIONS,
+};
+
+static enum _msm8996_version __init qcom_cpufreq_kryo_get_msm_id(void)
+{
+ size_t len;
+ u32 *msm_id;
+ enum _msm8996_version version;
+
+ msm_id = qcom_smem_get(QCOM_SMEM_HOST_ANY, MSM_ID_SMEM, &len);
+ /* The first 4 bytes are format, next to them is the actual msm-id */
+ msm_id++;
+
+ switch ((enum _msm_id)*msm_id) {
+ case MSM8996V3:
+ case APQ8096V3:
+ version = MSM8996_V3;
+ break;
+ case MSM8996SG:
+ case APQ8096SG:
+ version = MSM8996_SG;
+ break;
+ default:
+ version = NUM_OF_MSM8996_VERSIONS;
+ }
+
+ return version;
+}
+
+static int __init qcom_cpufreq_kryo_driver_init(void)
+{
+ struct opp_table *opp_tables[NR_CPUS] = {0};
+ enum _msm8996_version msm8996_version;
+ struct nvmem_cell *speedbin_nvmem;
+ struct platform_device *pdev;
+ struct device_node *np;
+ struct device *cpu_dev;
+ unsigned cpu;
+ u8 *speedbin;
+ u32 versions;
+ size_t len;
+ int ret;
+
+ cpu_dev = get_cpu_device(0);
+ if (NULL == cpu_dev)
+ return -ENODEV;
+
+ msm8996_version = qcom_cpufreq_kryo_get_msm_id();
+ if (NUM_OF_MSM8996_VERSIONS == msm8996_version) {
+ dev_err(cpu_dev, "Not Snapdragon 820/821!");
+ return -ENODEV;
+ }
+
+ np = dev_pm_opp_of_get_opp_desc_node(cpu_dev);
+ if (IS_ERR(np))
+ return PTR_ERR(np);
+
+ if (!of_device_is_compatible(np, "operating-points-v2-kryo-cpu")) {
+ ret = -ENOENT;
+ goto free_np;
+ }
+
+ speedbin_nvmem = of_nvmem_cell_get(np, NULL);
+ if (IS_ERR(speedbin_nvmem)) {
+ ret = PTR_ERR(speedbin_nvmem);
+ dev_err(cpu_dev, "Could not get nvmem cell: %d\n", ret);
+ goto free_np;
+ }
+
+ speedbin = nvmem_cell_read(speedbin_nvmem, &len);
+ nvmem_cell_put(speedbin_nvmem);
+
+ switch (msm8996_version) {
+ case MSM8996_V3:
+ versions = 1 << (unsigned int)(*speedbin);
+ break;
+ case MSM8996_SG:
+ versions = 1 << ((unsigned int)(*speedbin) + 4);
+ break;
+ default:
+ BUG();
+ break;
+ }
+
+ for_each_possible_cpu(cpu) {
+ cpu_dev = get_cpu_device(cpu);
+ if (NULL == cpu_dev) {
+ ret = -ENODEV;
+ goto free_opp;
+ }
+
+ opp_tables[cpu] = dev_pm_opp_set_supported_hw(cpu_dev,
+ &versions, 1);
+ ret = PTR_ERR(opp_tables[cpu]);
+ if (ret && ret != -EBUSY) {
+ dev_err(cpu_dev, "Failed to set supported hardware\n");
+ goto free_opp;
+ }
+ }
+
+ pdev = platform_device_register_simple("cpufreq-dt", -1, NULL, 0);
+ if (!IS_ERR(pdev))
+ return 0;
+
+ ret = PTR_ERR(pdev);
+ dev_err(cpu_dev, "Failed to register platform device\n");
+
+free_opp:
+ for_each_possible_cpu(cpu) {
+ if (IS_ERR_OR_NULL(opp_tables[cpu]))
+ break;
+ dev_pm_opp_put_supported_hw(opp_tables[cpu]);
+ }
+free_np:
+ of_node_put(np);
+
+ return ret;
+}
+late_initcall(qcom_cpufreq_kryo_driver_init);
+
+MODULE_DESCRIPTION("Qualcomm Technologies, Inc. Kryo CPUfreq driver");
+MODULE_LICENSE("GPL v2");
--
1.9.1
^ permalink raw reply related
* Re: [PATCH v2] schedutil: Allow cpufreq requests to be made even when kthread kicked
From: Rafael J. Wysocki @ 2018-05-22 11:26 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Joel Fernandes, Viresh Kumar, Joel Fernandes (Google.),
Linux Kernel Mailing List, Rafael J . Wysocki, Peter Zijlstra,
Ingo Molnar, Patrick Bellasi, Juri Lelli, Luca Abeni, Todd Kjos,
Claudio Scordino, kernel-team, Linux PM
In-Reply-To: <CAJZ5v0hucyBEyWoz8nS=6pfwt8Yd5Wi+39=2WGFgfJobv7=qPQ@mail.gmail.com>
On Tuesday, May 22, 2018 12:02:24 PM CEST Rafael J. Wysocki wrote:
> On Mon, May 21, 2018 at 6:13 PM, Joel Fernandes <joel@joelfernandes.org> wrote:
> > On Mon, May 21, 2018 at 10:29:52AM +0200, Rafael J. Wysocki wrote:
> >> On Mon, May 21, 2018 at 7:14 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >> > On 18-05-18, 11:55, Joel Fernandes (Google.) wrote:
> >> >> From: "Joel Fernandes (Google)" <joel@joelfernandes.org>
> >> >>
> >> >> Currently there is a chance of a schedutil cpufreq update request to be
> >> >> dropped if there is a pending update request. This pending request can
> >> >> be delayed if there is a scheduling delay of the irq_work and the wake
> >> >> up of the schedutil governor kthread.
> >> >>
> >> >> A very bad scenario is when a schedutil request was already just made,
> >> >> such as to reduce the CPU frequency, then a newer request to increase
> >> >> CPU frequency (even sched deadline urgent frequency increase requests)
> >> >> can be dropped, even though the rate limits suggest that its Ok to
> >> >> process a request. This is because of the way the work_in_progress flag
> >> >> is used.
> >> >>
> >> >> This patch improves the situation by allowing new requests to happen
> >> >> even though the old one is still being processed. Note that in this
> >> >> approach, if an irq_work was already issued, we just update next_freq
> >> >> and don't bother to queue another request so there's no extra work being
> >> >> done to make this happen.
> >> >
> >> > Now that this isn't an RFC anymore, you shouldn't have added below
> >> > paragraph here. It could go to the comments section though.
> >> >
> >> >> I had brought up this issue at the OSPM conference and Claudio had a
> >> >> discussion RFC with an alternate approach [1]. I prefer the approach as
> >> >> done in the patch below since it doesn't need any new flags and doesn't
> >> >> cause any other extra overhead.
> >> >>
> >> >> [1] https://patchwork.kernel.org/patch/10384261/
> >> >>
> >> >> LGTMed-by: Viresh Kumar <viresh.kumar@linaro.org>
> >> >> LGTMed-by: Juri Lelli <juri.lelli@redhat.com>
> >> >
> >> > Looks like a Tag you just invented ? :)
> >>
> >> Yeah.
> >>
> >> The LGTM from Juri can be converned into an ACK silently IMO. That
> >> said I have added Looks-good-to: tags to a couple of commits. :-)
> >
> > Cool, I'll covert them to Acks :-)
>
> So it looks like I should expect an update of this patch, right?
>
> Or do you prefer the current one to be applied and work on top of it?
Well, sorry, I can't apply this one as it is racy.
^ permalink raw reply
* Re: [PATCH] PM / core: Fix direct_complete handling for devices with no callbacks
From: Rafael J. Wysocki @ 2018-05-22 11:23 UTC (permalink / raw)
To: Thomas Martitz
Cc: Linux PM, LKML, Ulf Hansson, Lukas Wunner, Greg Kroah-Hartman
In-Reply-To: <229515f7-d812-b92e-6918-886b62718991@rockbox.org>
On Tuesday, May 22, 2018 1:12:40 PM CEST Thomas Martitz wrote:
> Hello,
>
> thanks for for your effort and the patch.
>
> Is this eligible for stable?
Yes, it is.
Thanks,
Rafael
> Am 22.05.2018 um 13:02 schrieb Rafael J. Wysocki:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Commit 08810a4119aa (PM / core: Add NEVER_SKIP and SMART_PREPARE
> > driver flags) inadvertently prevented the power.direct_complete flag
> > from being set for devices without PM callbacks and with disabled
> > runtime PM which also prevents power.direct_complete from being set
> > for their parents. That led to problems including a resume crash on
> > HP ZBook 14u.
> >
> > Restore the previous behavior by causing power.direct_complete to be
> > set for those devices again, but do that in a more direct way to
> > avoid overlooking that case in the future.
> >
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=199693
> > Fixes: 08810a4119aa (PM / core: Add NEVER_SKIP and SMART_PREPARE driver flags)
> > Reported-by: Thomas Martitz <kugel@rockbox.org>
> > Tested-by: Thomas Martitz <kugel@rockbox.org>
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> > drivers/base/power/main.c | 7 +++----
> > 1 file changed, 3 insertions(+), 4 deletions(-)
> >
> > Index: linux-pm/drivers/base/power/main.c
> > ===================================================================
> > --- linux-pm.orig/drivers/base/power/main.c
> > +++ linux-pm/drivers/base/power/main.c
> > @@ -1920,10 +1920,8 @@ static int device_prepare(struct device
> >
> > dev->power.wakeup_path = false;
> >
> > - if (dev->power.no_pm_callbacks) {
> > - ret = 1; /* Let device go direct_complete */
> > + if (dev->power.no_pm_callbacks)
> > goto unlock;
> > - }
> >
> > if (dev->pm_domain)
> > callback = dev->pm_domain->ops.prepare;
> > @@ -1957,7 +1955,8 @@ unlock:
> > */
> > spin_lock_irq(&dev->power.lock);
> > dev->power.direct_complete = state.event == PM_EVENT_SUSPEND &&
> > - pm_runtime_suspended(dev) && ret > 0 &&
> > + ((pm_runtime_suspended(dev) && ret > 0) ||
> > + dev->power.no_pm_callbacks) &&
> > !dev_pm_test_driver_flags(dev, DPM_FLAG_NEVER_SKIP);
> > spin_unlock_irq(&dev->power.lock);
> > return 0;
> >
>
>
^ permalink raw reply
* Re: [PATCH] PM / core: Fix direct_complete handling for devices with no callbacks
From: Thomas Martitz @ 2018-05-22 11:12 UTC (permalink / raw)
To: Rafael J. Wysocki, Linux PM
Cc: LKML, Ulf Hansson, Lukas Wunner, Greg Kroah-Hartman, stable
In-Reply-To: <6238546.EnOBWIOf9o@aspire.rjw.lan>
Hello,
thanks for for your effort and the patch.
Is this eligible for stable?
Best regards
Am 22.05.2018 um 13:02 schrieb Rafael J. Wysocki:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Commit 08810a4119aa (PM / core: Add NEVER_SKIP and SMART_PREPARE
> driver flags) inadvertently prevented the power.direct_complete flag
> from being set for devices without PM callbacks and with disabled
> runtime PM which also prevents power.direct_complete from being set
> for their parents. That led to problems including a resume crash on
> HP ZBook 14u.
>
> Restore the previous behavior by causing power.direct_complete to be
> set for those devices again, but do that in a more direct way to
> avoid overlooking that case in the future.
>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=199693
> Fixes: 08810a4119aa (PM / core: Add NEVER_SKIP and SMART_PREPARE driver flags)
> Reported-by: Thomas Martitz <kugel@rockbox.org>
> Tested-by: Thomas Martitz <kugel@rockbox.org>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> drivers/base/power/main.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> Index: linux-pm/drivers/base/power/main.c
> ===================================================================
> --- linux-pm.orig/drivers/base/power/main.c
> +++ linux-pm/drivers/base/power/main.c
> @@ -1920,10 +1920,8 @@ static int device_prepare(struct device
>
> dev->power.wakeup_path = false;
>
> - if (dev->power.no_pm_callbacks) {
> - ret = 1; /* Let device go direct_complete */
> + if (dev->power.no_pm_callbacks)
> goto unlock;
> - }
>
> if (dev->pm_domain)
> callback = dev->pm_domain->ops.prepare;
> @@ -1957,7 +1955,8 @@ unlock:
> */
> spin_lock_irq(&dev->power.lock);
> dev->power.direct_complete = state.event == PM_EVENT_SUSPEND &&
> - pm_runtime_suspended(dev) && ret > 0 &&
> + ((pm_runtime_suspended(dev) && ret > 0) ||
> + dev->power.no_pm_callbacks) &&
> !dev_pm_test_driver_flags(dev, DPM_FLAG_NEVER_SKIP);
> spin_unlock_irq(&dev->power.lock);
> return 0;
>
^ permalink raw reply
* [PATCH v4 2/2] cpufreq: schedutil: Cleanup and document iowait boost
From: Patrick Bellasi @ 2018-05-22 11:07 UTC (permalink / raw)
To: linux-kernel, linux-pm
Cc: Ingo Molnar, Peter Zijlstra, Rafael J . Wysocki, Viresh Kumar,
Vincent Guittot, Dietmar Eggemann, Juri Lelli, Joel Fernandes
In-Reply-To: <20180522110754.20832-1-patrick.bellasi@arm.com>
The iowait boosting code has been recently updated to add a progressive
boosting behavior which allows to be less aggressive in boosting tasks
doing only sporadic IO operations, thus being more energy efficient for
example on mobile platforms.
The current code is now however a bit convoluted. Some functionalities
(e.g. iowait boost reset) are replicated in different paths and their
documentation is slightly misaligned.
Let's cleanup the code by consolidating all the IO wait boosting related
functionality within within few dedicated functions and better define
their role:
- sugov_iowait_boost: set/increase the IO wait boost of a CPU
- sugov_iowait_apply: apply/reduce the IO wait boost of a CPU
Both these two function are used at every sugov update and they make
use of a unified IO wait boost reset policy provided by:
- sugov_iowait_reset: reset/disable the IO wait boost of a CPU
if a CPU is not updated for more then one tick
This makes possible a cleaner and more self-contained design for the IO
wait boosting code since the rest of the sugov update routines, both for
single and shared frequency domains, follow the same template:
/* Configure IO boost, if required */
sugov_iowait_boost()
/* Return here if freq change is in progress or throttled */
/* Collect and aggregate utilization information */
sugov_get_util()
sugov_aggregate_util()
/*
* Add IO boost, if currently enabled, on top of the aggregated
* utilization value
*/
sugov_iowait_apply()
As a extra bonus, let's also add the documentation for the new
functions and better align the in-code documentation.
Signed-off-by: Patrick Bellasi <patrick.bellasi@arm.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Cc: Joel Fernandes <joelaf@google.com>
Cc: Juri Lelli <juri.lelli@redhat.com>
Cc: Dietmar Eggemann <dietmar.eggemann@arm.com>
Cc: linux-kernel@vger.kernel.org
Cc: linux-pm@vger.kernel.org
Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
Changes in v4:
- fixed typos in changelog (Joel)
- added Reviewed-by: Joel
- added Acked-by: Viresh and Peter
Changes in v3:
- renamed the iowait boost functions (Peter)
- moved boost reset into a dedicated function (Peter)
- split the fix into a separated patch (Viresh)
---
kernel/sched/cpufreq_schedutil.c | 152 +++++++++++++++++++++++++++------------
1 file changed, 107 insertions(+), 45 deletions(-)
diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
index c4063e578e4d..1a50e1e95f4b 100644
--- a/kernel/sched/cpufreq_schedutil.c
+++ b/kernel/sched/cpufreq_schedutil.c
@@ -51,7 +51,7 @@ struct sugov_cpu {
bool iowait_boost_pending;
unsigned int iowait_boost;
unsigned int iowait_boost_max;
- u64 last_update;
+ u64 last_update;
/* The fields below are only needed when sharing a policy: */
unsigned long util_cfs;
@@ -201,45 +201,120 @@ static unsigned long sugov_aggregate_util(struct sugov_cpu *sg_cpu)
return min(util, sg_cpu->max);
}
-static void sugov_set_iowait_boost(struct sugov_cpu *sg_cpu, u64 time, unsigned int flags)
+/**
+ * sugov_iowait_reset() - Reset the IO boost status of a CPU.
+ * @sg_cpu: the sugov data for the CPU to boost
+ * @time: the update time from the caller
+ * @set_iowait_boost: true if an IO boost has been requested
+ *
+ * The IO wait boost of a task is disabled after a tick since the last update
+ * of a CPU. If a new IO wait boost is requested after more then a tick, then
+ * we enable the boost starting from the minimum frequency, which improves
+ * energy efficiency by ignoring sporadic wakeups from IO.
+ */
+static bool sugov_iowait_reset(struct sugov_cpu *sg_cpu, u64 time,
+ bool set_iowait_boost)
{
- /* Clear iowait_boost if the CPU apprears to have been idle. */
- if (sg_cpu->iowait_boost) {
- s64 delta_ns = time - sg_cpu->last_update;
+ s64 delta_ns = time - sg_cpu->last_update;
- if (delta_ns > TICK_NSEC) {
- sg_cpu->iowait_boost = 0;
- sg_cpu->iowait_boost_pending = false;
- }
- }
+ /* Reset boost only if a tick has elapsed since last request */
+ if (delta_ns <= TICK_NSEC)
+ return false;
- if (flags & SCHED_CPUFREQ_IOWAIT) {
- if (sg_cpu->iowait_boost_pending)
- return;
+ sg_cpu->iowait_boost = set_iowait_boost
+ ? sg_cpu->sg_policy->policy->min : 0;
+ sg_cpu->iowait_boost_pending = set_iowait_boost;
- sg_cpu->iowait_boost_pending = true;
+ return true;
+}
- if (sg_cpu->iowait_boost) {
- sg_cpu->iowait_boost <<= 1;
- if (sg_cpu->iowait_boost > sg_cpu->iowait_boost_max)
- sg_cpu->iowait_boost = sg_cpu->iowait_boost_max;
- } else {
- sg_cpu->iowait_boost = sg_cpu->sg_policy->policy->min;
- }
+/**
+ * sugov_iowait_boost() - Updates the IO boost status of a CPU.
+ * @sg_cpu: the sugov data for the CPU to boost
+ * @time: the update time from the caller
+ * @flags: SCHED_CPUFREQ_IOWAIT if the task is waking up after an IO wait
+ *
+ * Each time a task wakes up after an IO operation, the CPU utilization can be
+ * boosted to a certain utilization which doubles at each "frequent and
+ * successive" wakeup from IO, ranging from the utilization of the minimum
+ * OPP to the utilization of the maximum OPP.
+ * To keep doubling, an IO boost has to be requested at least once per tick,
+ * otherwise we restart from the utilization of the minimum OPP.
+ */
+static void sugov_iowait_boost(struct sugov_cpu *sg_cpu, u64 time,
+ unsigned int flags)
+{
+ bool set_iowait_boost = flags & SCHED_CPUFREQ_IOWAIT;
+
+ /* Reset boost if the CPU appears to have been idle enough */
+ if (sg_cpu->iowait_boost &&
+ sugov_iowait_reset(sg_cpu, time, set_iowait_boost))
+ return;
+
+ /* Boost only tasks waking up after IO */
+ if (!set_iowait_boost)
+ return;
+
+ /* Ensure boost doubles only one time at each request */
+ if (sg_cpu->iowait_boost_pending)
+ return;
+ sg_cpu->iowait_boost_pending = true;
+
+ /* Double the boost at each request */
+ if (sg_cpu->iowait_boost) {
+ sg_cpu->iowait_boost <<= 1;
+ if (sg_cpu->iowait_boost > sg_cpu->iowait_boost_max)
+ sg_cpu->iowait_boost = sg_cpu->iowait_boost_max;
+ return;
}
+
+ /* First wakeup after IO: start with minimum boost */
+ sg_cpu->iowait_boost = sg_cpu->sg_policy->policy->min;
}
-static void sugov_iowait_boost(struct sugov_cpu *sg_cpu, unsigned long *util,
- unsigned long *max)
+/**
+ * sugov_iowait_apply() - Apply the IO boost to a CPU.
+ * @sg_cpu: the sugov data for the cpu to boost
+ * @time: the update time from the caller
+ * @util: the utilization to (eventually) boost
+ * @max: the maximum value the utilization can be boosted to
+ *
+ * A CPU running a task which woken up after an IO operation can have its
+ * utilization boosted to speed up the completion of those IO operations.
+ * The IO boost value is increased each time a task wakes up from IO, in
+ * sugov_iowait_apply(), and it's instead decreased by this function,
+ * each time an increase has not been requested (!iowait_boost_pending).
+ *
+ * A CPU which also appears to have been idle for at least one tick has also
+ * its IO boost utilization reset.
+ *
+ * This mechanism is designed to boost high frequently IO waiting tasks, while
+ * being more conservative on tasks which does sporadic IO operations.
+ */
+static void sugov_iowait_apply(struct sugov_cpu *sg_cpu, u64 time,
+ unsigned long *util, unsigned long *max)
{
unsigned int boost_util, boost_max;
+ /* No boost currently required */
if (!sg_cpu->iowait_boost)
return;
+ /* Reset boost if the CPU appears to have been idle enough */
+ if (sugov_iowait_reset(sg_cpu, time, false))
+ return;
+
+ /*
+ * An IO waiting task has just woken up:
+ * allow to further double the boost value
+ */
if (sg_cpu->iowait_boost_pending) {
sg_cpu->iowait_boost_pending = false;
} else {
+ /*
+ * Otherwise: reduce the boost value and disable it when we
+ * reach the minimum.
+ */
sg_cpu->iowait_boost >>= 1;
if (sg_cpu->iowait_boost < sg_cpu->sg_policy->policy->min) {
sg_cpu->iowait_boost = 0;
@@ -247,9 +322,12 @@ static void sugov_iowait_boost(struct sugov_cpu *sg_cpu, unsigned long *util,
}
}
+ /*
+ * Apply the current boost value: a CPU is boosted only if its current
+ * utilization is smaller then the current IO boost level.
+ */
boost_util = sg_cpu->iowait_boost;
boost_max = sg_cpu->iowait_boost_max;
-
if (*util * boost_max < *max * boost_util) {
*util = boost_util;
*max = boost_max;
@@ -288,7 +366,7 @@ static void sugov_update_single(struct update_util_data *hook, u64 time,
unsigned int next_f;
bool busy;
- sugov_set_iowait_boost(sg_cpu, time, flags);
+ sugov_iowait_boost(sg_cpu, time, flags);
sg_cpu->last_update = time;
ignore_dl_rate_limit(sg_cpu, sg_policy);
@@ -301,7 +379,7 @@ static void sugov_update_single(struct update_util_data *hook, u64 time,
sugov_get_util(sg_cpu);
max = sg_cpu->max;
util = sugov_aggregate_util(sg_cpu);
- sugov_iowait_boost(sg_cpu, &util, &max);
+ sugov_iowait_apply(sg_cpu, time, &util, &max);
next_f = get_next_freq(sg_policy, util, max);
/*
* Do not reduce the frequency if the CPU has not been idle
@@ -328,28 +406,12 @@ static unsigned int sugov_next_freq_shared(struct sugov_cpu *sg_cpu, u64 time)
for_each_cpu(j, policy->cpus) {
struct sugov_cpu *j_sg_cpu = &per_cpu(sugov_cpu, j);
unsigned long j_util, j_max;
- s64 delta_ns;
sugov_get_util(j_sg_cpu);
-
- /*
- * If the CFS CPU utilization was last updated before the
- * previous frequency update and the time elapsed between the
- * last update of the CPU utilization and the last frequency
- * update is long enough, reset iowait_boost and util_cfs, as
- * they are now probably stale. However, still consider the
- * CPU contribution if it has some DEADLINE utilization
- * (util_dl).
- */
- delta_ns = time - j_sg_cpu->last_update;
- if (delta_ns > TICK_NSEC) {
- j_sg_cpu->iowait_boost = 0;
- j_sg_cpu->iowait_boost_pending = false;
- }
-
j_max = j_sg_cpu->max;
j_util = sugov_aggregate_util(j_sg_cpu);
- sugov_iowait_boost(j_sg_cpu, &j_util, &j_max);
+ sugov_iowait_apply(j_sg_cpu, time, &j_util, &j_max);
+
if (j_util * max > j_max * util) {
util = j_util;
max = j_max;
@@ -368,7 +430,7 @@ sugov_update_shared(struct update_util_data *hook, u64 time, unsigned int flags)
raw_spin_lock(&sg_policy->update_lock);
- sugov_set_iowait_boost(sg_cpu, time, flags);
+ sugov_iowait_boost(sg_cpu, time, flags);
sg_cpu->last_update = time;
ignore_dl_rate_limit(sg_cpu, sg_policy);
--
2.15.1
^ permalink raw reply related
* [PATCH v4 1/2] cpufreq: schedutil: Fix iowait boost reset
From: Patrick Bellasi @ 2018-05-22 11:07 UTC (permalink / raw)
To: linux-kernel, linux-pm
Cc: Ingo Molnar, Peter Zijlstra, Rafael J . Wysocki, Viresh Kumar,
Vincent Guittot, Dietmar Eggemann, Juri Lelli, Joel Fernandes
In-Reply-To: <20180522110754.20832-1-patrick.bellasi@arm.com>
A more energy efficient update of the IO wait boosting mechanism has
been introduced in:
commit a5a0809bc58e ("cpufreq: schedutil: Make iowait boost more energy efficient")
where the boost value is expected to be:
- doubled at each successive wakeup from IO
staring from the minimum frequency supported by a CPU
- reset when a CPU is not updated for more then one tick
by either disabling the IO wait boost or resetting its value to the
minimum frequency if this new update requires an IO boost.
This approach is supposed to "ignore" boosting for sporadic wakeups from
IO, while still getting the frequency boosted to the maximum to benefit
long sequence of wakeup from IO operations.
However, these assumptions are not always satisfied.
For example, when an IO boosted CPU enters idle for more the one tick
and then wakes up after an IO wait, since in sugov_set_iowait_boost() we
first check the IOWAIT flag, we keep doubling the iowait boost instead
of restarting from the minimum frequency value.
This misbehavior could happen mainly on non-shared frequency domains,
thus defeating the energy efficiency optimization, but it can also
happen on shared frequency domain systems.
Let fix this issue in sugov_set_iowait_boost() by:
- first check the IO wait boost reset conditions
to eventually reset the boost value
- then applying the correct IO boost value
if required by the caller
Reported-by: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Patrick Bellasi <patrick.bellasi@arm.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Cc: Joel Fernandes <joelaf@google.com>
Cc: Juri Lelli <juri.lelli@redhat.com>
Cc: Dietmar Eggemann <dietmar.eggemann@arm.com>
Cc: linux-kernel@vger.kernel.org
Cc: linux-pm@vger.kernel.org
Fixes: a5a0809bc58e ("cpufreq: schedutil: Make iowait boost more energy efficient")
Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
Changes in v4:
- added Reviewed-by Joel Fernandes
- added Acked-by Viresh and Peter
Changes in v3:
- split the fix into a separated patch (this one)
- added "Fixes" tag (Viresh)
---
kernel/sched/cpufreq_schedutil.c | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
index e13df951aca7..c4063e578e4d 100644
--- a/kernel/sched/cpufreq_schedutil.c
+++ b/kernel/sched/cpufreq_schedutil.c
@@ -203,6 +203,16 @@ static unsigned long sugov_aggregate_util(struct sugov_cpu *sg_cpu)
static void sugov_set_iowait_boost(struct sugov_cpu *sg_cpu, u64 time, unsigned int flags)
{
+ /* Clear iowait_boost if the CPU apprears to have been idle. */
+ if (sg_cpu->iowait_boost) {
+ s64 delta_ns = time - sg_cpu->last_update;
+
+ if (delta_ns > TICK_NSEC) {
+ sg_cpu->iowait_boost = 0;
+ sg_cpu->iowait_boost_pending = false;
+ }
+ }
+
if (flags & SCHED_CPUFREQ_IOWAIT) {
if (sg_cpu->iowait_boost_pending)
return;
@@ -216,14 +226,6 @@ static void sugov_set_iowait_boost(struct sugov_cpu *sg_cpu, u64 time, unsigned
} else {
sg_cpu->iowait_boost = sg_cpu->sg_policy->policy->min;
}
- } else if (sg_cpu->iowait_boost) {
- s64 delta_ns = time - sg_cpu->last_update;
-
- /* Clear iowait_boost if the CPU apprears to have been idle. */
- if (delta_ns > TICK_NSEC) {
- sg_cpu->iowait_boost = 0;
- sg_cpu->iowait_boost_pending = false;
- }
}
}
--
2.15.1
^ permalink raw reply related
* [PATCH v4 0/2] Fix and cleanup iowait boost
From: Patrick Bellasi @ 2018-05-22 11:07 UTC (permalink / raw)
To: linux-kernel, linux-pm
Cc: Ingo Molnar, Peter Zijlstra, Rafael J . Wysocki, Viresh Kumar,
Vincent Guittot, Dietmar Eggemann, Juri Lelli, Joel Fernandes
Here is the (hopefully) final update of:
https://lkml.org/lkml/2018/5/21/114
20180521085120.7902-1-patrick.bellasi@arm.com
which just includes the small typo fixes suggested by Jeol and the ack
and review tags collected so far.
Cheers Patrick
Changes in v4:
- fixed typos in changelog (Joel)
- added Reviewed-by Joel Fernandes
- added Acked-by Viresh and Peter
Changes in v3:
- renamed the iowait boost functions (Peter)
- moved boost reset into a dedicated function (Peter)
- split the fix into a separated patch (Viresh)
- added "Fixes" tag (Viresh)
Patrick Bellasi (2):
cpufreq: schedutil: Fix iowait boost reset
cpufreq: schedutil: Cleanup and document iowait boost
kernel/sched/cpufreq_schedutil.c | 152 +++++++++++++++++++++++++++------------
1 file changed, 108 insertions(+), 44 deletions(-)
--
2.15.1
^ permalink raw reply
* [PATCH] PM / core: Fix direct_complete handling for devices with no callbacks
From: Rafael J. Wysocki @ 2018-05-22 11:02 UTC (permalink / raw)
To: Linux PM
Cc: LKML, Thomas Martitz, Ulf Hansson, Lukas Wunner,
Greg Kroah-Hartman
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Commit 08810a4119aa (PM / core: Add NEVER_SKIP and SMART_PREPARE
driver flags) inadvertently prevented the power.direct_complete flag
from being set for devices without PM callbacks and with disabled
runtime PM which also prevents power.direct_complete from being set
for their parents. That led to problems including a resume crash on
HP ZBook 14u.
Restore the previous behavior by causing power.direct_complete to be
set for those devices again, but do that in a more direct way to
avoid overlooking that case in the future.
Link: https://bugzilla.kernel.org/show_bug.cgi?id=199693
Fixes: 08810a4119aa (PM / core: Add NEVER_SKIP and SMART_PREPARE driver flags)
Reported-by: Thomas Martitz <kugel@rockbox.org>
Tested-by: Thomas Martitz <kugel@rockbox.org>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
drivers/base/power/main.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
Index: linux-pm/drivers/base/power/main.c
===================================================================
--- linux-pm.orig/drivers/base/power/main.c
+++ linux-pm/drivers/base/power/main.c
@@ -1920,10 +1920,8 @@ static int device_prepare(struct device
dev->power.wakeup_path = false;
- if (dev->power.no_pm_callbacks) {
- ret = 1; /* Let device go direct_complete */
+ if (dev->power.no_pm_callbacks)
goto unlock;
- }
if (dev->pm_domain)
callback = dev->pm_domain->ops.prepare;
@@ -1957,7 +1955,8 @@ unlock:
*/
spin_lock_irq(&dev->power.lock);
dev->power.direct_complete = state.event == PM_EVENT_SUSPEND &&
- pm_runtime_suspended(dev) && ret > 0 &&
+ ((pm_runtime_suspended(dev) && ret > 0) ||
+ dev->power.no_pm_callbacks) &&
!dev_pm_test_driver_flags(dev, DPM_FLAG_NEVER_SKIP);
spin_unlock_irq(&dev->power.lock);
return 0;
^ permalink raw reply
* Re: [PATCH v2] schedutil: Allow cpufreq requests to be made even when kthread kicked
From: Viresh Kumar @ 2018-05-22 10:56 UTC (permalink / raw)
To: Patrick Bellasi
Cc: Joel Fernandes (Google.), linux-kernel, Joel Fernandes (Google),
Rafael J . Wysocki, Peter Zijlstra, Ingo Molnar, Juri Lelli,
Luca Abeni, Todd Kjos, claudio, kernel-team, linux-pm
In-Reply-To: <20180522105157.GX30654@e110439-lin>
On 22-05-18, 11:51, Patrick Bellasi wrote:
> It could happen, but using:
>
> raw_spin_lock_irqsave(&sg_policy->update_lock, flags);
> freq = READ_ONCE(sg_policy->next_freq)
> WRITE_ONCE(sg_policy->work_in_progress, false);
> raw_spin_unlock_irqrestore(&sg_policy->update_lock, flags);
>
> if (!READ_ONCE(sg_policy->work_in_progress)) {
> WRITE_ONCE(sg_policy->work_in_progress, true);
> irq_work_queue(&sg_policy->irq_work);
> }
I think its better to get locking in place for non-fast switching case in
single-policy systems right now.
> should fix it by enforcing the ordering as well as documenting the
> concurrent access.
>
> However, in the "sched update" side, where do we have the sequence:
>
> sg_policy->next_freq = 0;
> sg_policy->next_freq = real-next-freq;
Ah, that was just an example of what a compiler may do (though it shouldn't do).
--
viresh
^ permalink raw reply
* Re: [PATCH v2] schedutil: Allow cpufreq requests to be made even when kthread kicked
From: Viresh Kumar @ 2018-05-22 10:54 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Joel Fernandes (Google.), linux-kernel, Joel Fernandes (Google),
Rafael J . Wysocki, Peter Zijlstra, Ingo Molnar, Patrick Bellasi,
Juri Lelli, Luca Abeni, Todd Kjos, claudio, kernel-team, linux-pm
In-Reply-To: <4393838.cQWhzXzUcj@aspire.rjw.lan>
On 22-05-18, 12:50, Rafael J. Wysocki wrote:
> Ugly indeed.
Hehe. I was thinking, maybe we can write wrapper helpers around lock/unlock
which are stored as pointers in sg_policy. So that those are only set to
non-NULL values (or non-Noop routines) for slow-switching single policy or
any-switching shared policy systems. Then we can get rid of such conditional
locking attempts :)
--
viresh
^ permalink raw reply
* Re: [PATCH v2] schedutil: Allow cpufreq requests to be made even when kthread kicked
From: Patrick Bellasi @ 2018-05-22 10:51 UTC (permalink / raw)
To: Viresh Kumar
Cc: Joel Fernandes (Google.), linux-kernel, Joel Fernandes (Google),
Rafael J . Wysocki, Peter Zijlstra, Ingo Molnar, Juri Lelli,
Luca Abeni, Todd Kjos, claudio, kernel-team, linux-pm
In-Reply-To: <20180522103415.cuutobi5kbhj4gcw@vireshk-i7>
On 22-May 16:04, Viresh Kumar wrote:
> Okay, me and Rafael were discussing this patch, locking and races around this.
>
> On 18-05-18, 11:55, Joel Fernandes (Google.) wrote:
> > @@ -382,13 +386,27 @@ sugov_update_shared(struct update_util_data *hook, u64 time, unsigned int flags)
> > static void sugov_work(struct kthread_work *work)
> > {
> > struct sugov_policy *sg_policy = container_of(work, struct sugov_policy, work);
> > + unsigned int freq;
> > + unsigned long flags;
> > +
> > + /*
> > + * Hold sg_policy->update_lock shortly to handle the case where:
> > + * incase sg_policy->next_freq is read here, and then updated by
> > + * sugov_update_shared just before work_in_progress is set to false
> > + * here, we may miss queueing the new update.
> > + *
> > + * Note: If a work was queued after the update_lock is released,
> > + * sugov_work will just be called again by kthread_work code; and the
> > + * request will be proceed before the sugov thread sleeps.
> > + */
> > + raw_spin_lock_irqsave(&sg_policy->update_lock, flags);
> > + freq = sg_policy->next_freq;
> > + sg_policy->work_in_progress = false;
> > + raw_spin_unlock_irqrestore(&sg_policy->update_lock, flags);
> >
> > mutex_lock(&sg_policy->work_lock);
> > - __cpufreq_driver_target(sg_policy->policy, sg_policy->next_freq,
> > - CPUFREQ_RELATION_L);
> > + __cpufreq_driver_target(sg_policy->policy, freq, CPUFREQ_RELATION_L);
> > mutex_unlock(&sg_policy->work_lock);
> > -
> > - sg_policy->work_in_progress = false;
> > }
>
> And I do see a race here for single policy systems doing slow switching.
>
> Kthread Sched update
>
> sugov_work() sugov_update_single()
>
> lock();
> // The CPU is free to rearrange below
> // two in any order, so it may clear
> // the flag first and then read next
> // freq. Lets assume it does.
> work_in_progress = false
>
> if (work_in_progress)
> return;
>
> sg_policy->next_freq = 0;
> freq = sg_policy->next_freq;
> sg_policy->next_freq = real-next-freq;
> unlock();
>
>
>
> Is the above theory right or am I day dreaming ? :)
It could happen, but using:
raw_spin_lock_irqsave(&sg_policy->update_lock, flags);
freq = READ_ONCE(sg_policy->next_freq)
WRITE_ONCE(sg_policy->work_in_progress, false);
raw_spin_unlock_irqrestore(&sg_policy->update_lock, flags);
if (!READ_ONCE(sg_policy->work_in_progress)) {
WRITE_ONCE(sg_policy->work_in_progress, true);
irq_work_queue(&sg_policy->irq_work);
}
should fix it by enforcing the ordering as well as documenting the
concurrent access.
However, in the "sched update" side, where do we have the sequence:
sg_policy->next_freq = 0;
sg_policy->next_freq = real-next-freq;
AFAICS we always use locals for next_freq and do one single assignment
in sugov_update_commit(), isn't it?
--
#include <best/regards.h>
Patrick Bellasi
^ permalink raw reply
* Re: [PATCH v2] schedutil: Allow cpufreq requests to be made even when kthread kicked
From: Rafael J. Wysocki @ 2018-05-22 10:50 UTC (permalink / raw)
To: Viresh Kumar
Cc: Joel Fernandes (Google.), linux-kernel, Joel Fernandes (Google),
Rafael J . Wysocki, Peter Zijlstra, Ingo Molnar, Patrick Bellasi,
Juri Lelli, Luca Abeni, Todd Kjos, claudio, kernel-team, linux-pm
In-Reply-To: <20180522105006.hvntwnhzzvhznij2@vireshk-i7>
On Tuesday, May 22, 2018 12:50:06 PM CEST Viresh Kumar wrote:
> On 22-05-18, 16:04, Viresh Kumar wrote:
> > Okay, me and Rafael were discussing this patch, locking and races around this.
> >
> > On 18-05-18, 11:55, Joel Fernandes (Google.) wrote:
> > > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> > > index e13df951aca7..5c482ec38610 100644
> > > --- a/kernel/sched/cpufreq_schedutil.c
> > > +++ b/kernel/sched/cpufreq_schedutil.c
> > > @@ -92,9 +92,6 @@ static bool sugov_should_update_freq(struct sugov_policy *sg_policy, u64 time)
> > > !cpufreq_can_do_remote_dvfs(sg_policy->policy))
> > > return false;
> > >
> > > - if (sg_policy->work_in_progress)
> > > - return false;
> > > -
> > > if (unlikely(sg_policy->need_freq_update)) {
> > > sg_policy->need_freq_update = false;
> > > /*
> > > @@ -128,7 +125,7 @@ static void sugov_update_commit(struct sugov_policy *sg_policy, u64 time,
> > >
> > > policy->cur = next_freq;
> > > trace_cpu_frequency(next_freq, smp_processor_id());
> > > - } else {
> > > + } else if (!sg_policy->work_in_progress) {
> > > sg_policy->work_in_progress = true;
> > > irq_work_queue(&sg_policy->irq_work);
> > > }
> > > @@ -291,6 +288,13 @@ static void sugov_update_single(struct update_util_data *hook, u64 time,
> > >
> > > ignore_dl_rate_limit(sg_cpu, sg_policy);
> > >
> > > + /*
> > > + * For slow-switch systems, single policy requests can't run at the
> > > + * moment if update is in progress, unless we acquire update_lock.
> > > + */
> > > + if (sg_policy->work_in_progress)
> > > + return;
> > > +
> > > if (!sugov_should_update_freq(sg_policy, time))
> > > return;
> > >
> > > @@ -382,13 +386,27 @@ sugov_update_shared(struct update_util_data *hook, u64 time, unsigned int flags)
> > > static void sugov_work(struct kthread_work *work)
> > > {
> > > struct sugov_policy *sg_policy = container_of(work, struct sugov_policy, work);
> > > + unsigned int freq;
> > > + unsigned long flags;
> > > +
> > > + /*
> > > + * Hold sg_policy->update_lock shortly to handle the case where:
> > > + * incase sg_policy->next_freq is read here, and then updated by
> > > + * sugov_update_shared just before work_in_progress is set to false
> > > + * here, we may miss queueing the new update.
> > > + *
> > > + * Note: If a work was queued after the update_lock is released,
> > > + * sugov_work will just be called again by kthread_work code; and the
> > > + * request will be proceed before the sugov thread sleeps.
> > > + */
> > > + raw_spin_lock_irqsave(&sg_policy->update_lock, flags);
> > > + freq = sg_policy->next_freq;
> > > + sg_policy->work_in_progress = false;
> > > + raw_spin_unlock_irqrestore(&sg_policy->update_lock, flags);
> > >
> > > mutex_lock(&sg_policy->work_lock);
> > > - __cpufreq_driver_target(sg_policy->policy, sg_policy->next_freq,
> > > - CPUFREQ_RELATION_L);
> > > + __cpufreq_driver_target(sg_policy->policy, freq, CPUFREQ_RELATION_L);
> > > mutex_unlock(&sg_policy->work_lock);
> > > -
> > > - sg_policy->work_in_progress = false;
> > > }
> >
> > And I do see a race here for single policy systems doing slow switching.
> >
> > Kthread Sched update
> >
> > sugov_work() sugov_update_single()
> >
> > lock();
> > // The CPU is free to rearrange below
> > // two in any order, so it may clear
> > // the flag first and then read next
> > // freq. Lets assume it does.
> > work_in_progress = false
> >
> > if (work_in_progress)
> > return;
> >
> > sg_policy->next_freq = 0;
> > freq = sg_policy->next_freq;
> > sg_policy->next_freq = real-next-freq;
> > unlock();
> >
> >
> >
> > Is the above theory right or am I day dreaming ? :)
>
> And here comes the ugly fix:
>
> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> index 35826f4ec43c..1665da31862e 100644
> --- a/kernel/sched/cpufreq_schedutil.c
> +++ b/kernel/sched/cpufreq_schedutil.c
> @@ -283,6 +283,9 @@ static void sugov_update_single(struct update_util_data *hook, u64 time,
>
> ignore_dl_rate_limit(sg_cpu, sg_policy);
>
> + if (!policy->fast_switch_enabled)
> + raw_spin_lock(&sg_policy->update_lock);
> +
> /*
> * For slow-switch systems, single policy requests can't run at the
> * moment if update is in progress, unless we acquire update_lock.
> @@ -312,6 +315,9 @@ static void sugov_update_single(struct update_util_data *hook, u64 time,
> }
>
> sugov_update_commit(sg_policy, time, next_f);
> +
> + if (!policy->fast_switch_enabled)
> + raw_spin_unlock(&sg_policy->update_lock);
> }
>
> static unsigned int sugov_next_freq_shared(struct sugov_cpu *sg_cpu, u64 time)
>
>
>
Ugly indeed.
^ permalink raw reply
* Re: [PATCH v2] schedutil: Allow cpufreq requests to be made even when kthread kicked
From: Viresh Kumar @ 2018-05-22 10:50 UTC (permalink / raw)
To: Joel Fernandes (Google.)
Cc: linux-kernel, Joel Fernandes (Google), Rafael J . Wysocki,
Peter Zijlstra, Ingo Molnar, Patrick Bellasi, Juri Lelli,
Luca Abeni, Todd Kjos, claudio, kernel-team, linux-pm
In-Reply-To: <20180522103415.cuutobi5kbhj4gcw@vireshk-i7>
On 22-05-18, 16:04, Viresh Kumar wrote:
> Okay, me and Rafael were discussing this patch, locking and races around this.
>
> On 18-05-18, 11:55, Joel Fernandes (Google.) wrote:
> > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> > index e13df951aca7..5c482ec38610 100644
> > --- a/kernel/sched/cpufreq_schedutil.c
> > +++ b/kernel/sched/cpufreq_schedutil.c
> > @@ -92,9 +92,6 @@ static bool sugov_should_update_freq(struct sugov_policy *sg_policy, u64 time)
> > !cpufreq_can_do_remote_dvfs(sg_policy->policy))
> > return false;
> >
> > - if (sg_policy->work_in_progress)
> > - return false;
> > -
> > if (unlikely(sg_policy->need_freq_update)) {
> > sg_policy->need_freq_update = false;
> > /*
> > @@ -128,7 +125,7 @@ static void sugov_update_commit(struct sugov_policy *sg_policy, u64 time,
> >
> > policy->cur = next_freq;
> > trace_cpu_frequency(next_freq, smp_processor_id());
> > - } else {
> > + } else if (!sg_policy->work_in_progress) {
> > sg_policy->work_in_progress = true;
> > irq_work_queue(&sg_policy->irq_work);
> > }
> > @@ -291,6 +288,13 @@ static void sugov_update_single(struct update_util_data *hook, u64 time,
> >
> > ignore_dl_rate_limit(sg_cpu, sg_policy);
> >
> > + /*
> > + * For slow-switch systems, single policy requests can't run at the
> > + * moment if update is in progress, unless we acquire update_lock.
> > + */
> > + if (sg_policy->work_in_progress)
> > + return;
> > +
> > if (!sugov_should_update_freq(sg_policy, time))
> > return;
> >
> > @@ -382,13 +386,27 @@ sugov_update_shared(struct update_util_data *hook, u64 time, unsigned int flags)
> > static void sugov_work(struct kthread_work *work)
> > {
> > struct sugov_policy *sg_policy = container_of(work, struct sugov_policy, work);
> > + unsigned int freq;
> > + unsigned long flags;
> > +
> > + /*
> > + * Hold sg_policy->update_lock shortly to handle the case where:
> > + * incase sg_policy->next_freq is read here, and then updated by
> > + * sugov_update_shared just before work_in_progress is set to false
> > + * here, we may miss queueing the new update.
> > + *
> > + * Note: If a work was queued after the update_lock is released,
> > + * sugov_work will just be called again by kthread_work code; and the
> > + * request will be proceed before the sugov thread sleeps.
> > + */
> > + raw_spin_lock_irqsave(&sg_policy->update_lock, flags);
> > + freq = sg_policy->next_freq;
> > + sg_policy->work_in_progress = false;
> > + raw_spin_unlock_irqrestore(&sg_policy->update_lock, flags);
> >
> > mutex_lock(&sg_policy->work_lock);
> > - __cpufreq_driver_target(sg_policy->policy, sg_policy->next_freq,
> > - CPUFREQ_RELATION_L);
> > + __cpufreq_driver_target(sg_policy->policy, freq, CPUFREQ_RELATION_L);
> > mutex_unlock(&sg_policy->work_lock);
> > -
> > - sg_policy->work_in_progress = false;
> > }
>
> And I do see a race here for single policy systems doing slow switching.
>
> Kthread Sched update
>
> sugov_work() sugov_update_single()
>
> lock();
> // The CPU is free to rearrange below
> // two in any order, so it may clear
> // the flag first and then read next
> // freq. Lets assume it does.
> work_in_progress = false
>
> if (work_in_progress)
> return;
>
> sg_policy->next_freq = 0;
> freq = sg_policy->next_freq;
> sg_policy->next_freq = real-next-freq;
> unlock();
>
>
>
> Is the above theory right or am I day dreaming ? :)
And here comes the ugly fix:
diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
index 35826f4ec43c..1665da31862e 100644
--- a/kernel/sched/cpufreq_schedutil.c
+++ b/kernel/sched/cpufreq_schedutil.c
@@ -283,6 +283,9 @@ static void sugov_update_single(struct update_util_data *hook, u64 time,
ignore_dl_rate_limit(sg_cpu, sg_policy);
+ if (!policy->fast_switch_enabled)
+ raw_spin_lock(&sg_policy->update_lock);
+
/*
* For slow-switch systems, single policy requests can't run at the
* moment if update is in progress, unless we acquire update_lock.
@@ -312,6 +315,9 @@ static void sugov_update_single(struct update_util_data *hook, u64 time,
}
sugov_update_commit(sg_policy, time, next_f);
+
+ if (!policy->fast_switch_enabled)
+ raw_spin_unlock(&sg_policy->update_lock);
}
static unsigned int sugov_next_freq_shared(struct sugov_cpu *sg_cpu, u64 time)
--
viresh
^ permalink raw reply related
* Re: [PATCH v2 2/2] cpufreq: qcom-fw: Add support for QCOM cpufreq FW driver
From: Taniya Das @ 2018-05-22 10:43 UTC (permalink / raw)
To: skannan, Viresh Kumar
Cc: Rafael J. Wysocki, linux-kernel, linux-pm, Stephen Boyd, robh,
Rajendra Nayak, Amit Nischal, devicetree, amit.kucheria
In-Reply-To: <0fccdc1880c912102548a85008482fa0@codeaurora.org>
Hello Viresh,
Thanks for your comments.
On 5/22/2018 12:36 AM, skannan@codeaurora.org wrote:
> On 2018-05-21 02:01, Viresh Kumar wrote:
>> On 19-05-18, 23:04, Taniya Das wrote:
>>> The CPUfreq FW present in some QCOM chipsets offloads the steps
>>> necessary
>>> for changing the frequency of CPUs. The driver implements the cpufreq
>>> driver interface for this firmware.
>>>
>>> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
>>> Signed-off-by: Taniya Das <tdas@codeaurora.org>
>>> ---
>>> drivers/cpufreq/Kconfig.arm | 9 ++
>>> drivers/cpufreq/Makefile | 1 +
>>> drivers/cpufreq/qcom-cpufreq-fw.c | 317
>>> ++++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 327 insertions(+)
>>> create mode 100644 drivers/cpufreq/qcom-cpufreq-fw.c
>>>
>>> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
>>> index 96b35b8..571f6b4 100644
>>> --- a/drivers/cpufreq/Kconfig.arm
>>> +++ b/drivers/cpufreq/Kconfig.arm
>>> @@ -301,3 +301,12 @@ config ARM_PXA2xx_CPUFREQ
>>> This add the CPUFreq driver support for Intel PXA2xx SOCs.
>>>
>>> If in doubt, say N.
>>> +
>>> +config ARM_QCOM_CPUFREQ_FW
>>> + bool "QCOM CPUFreq FW driver"
>>
>> During last review I didn't say that this driver shouldn't be a
>> module, but that you need to fix things to make it a module. I am fine
>> though if you don't want this to be a module ever.
>>
>>> + help
>>> + Support for the CPUFreq FW driver.
>>> + The CPUfreq FW preset in some QCOM chipsets offloads the steps
>>> + necessary for changing the frequency of CPUs. The driver
>>> + implements the cpufreq driver interface for this firmware.
>>> + Say Y if you want to support CPUFreq FW.
>>> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
>>> index 8d24ade..a3edbce 100644
>>> --- a/drivers/cpufreq/Makefile
>>> +++ b/drivers/cpufreq/Makefile
>>> @@ -85,6 +85,7 @@ obj-$(CONFIG_ARM_TEGRA124_CPUFREQ) +=
>>> tegra124-cpufreq.o
>>> obj-$(CONFIG_ARM_TEGRA186_CPUFREQ) += tegra186-cpufreq.o
>>> obj-$(CONFIG_ARM_TI_CPUFREQ) += ti-cpufreq.o
>>> obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ) += vexpress-spc-cpufreq.o
>>> +obj-$(CONFIG_ARM_QCOM_CPUFREQ_FW) += qcom-cpufreq-fw.o
>>>
>>>
>>>
>>> ##################################################################################
>>>
>>> diff --git a/drivers/cpufreq/qcom-cpufreq-fw.c
>>> b/drivers/cpufreq/qcom-cpufreq-fw.c
>>> new file mode 100644
>>> index 0000000..0e66de0
>>> --- /dev/null
>>> +++ b/drivers/cpufreq/qcom-cpufreq-fw.c
>>> @@ -0,0 +1,317 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
>>> + */
>>> +
>>> +#include <linux/cpufreq.h>
>>> +#include <linux/init.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/module.h>
>>> +#include <linux/of_address.h>
>>> +#include <linux/of_platform.h>
>>> +
>>> +#define INIT_RATE 300000000UL
>>> +#define XO_RATE 19200000UL
>>> +#define LUT_MAX_ENTRIES 40U
>>> +#define CORE_COUNT_VAL(val) ((val & GENMASK(18, 16)) >> 16)
>>> +#define LUT_ROW_SIZE 32
>>> +
>>> +struct cpufreq_qcom {
>>> + struct cpufreq_frequency_table *table;
>>> + struct device *dev;
>>> + void __iomem *perf_base;
>>> + void __iomem *lut_base;
>>> + cpumask_t related_cpus;
>>> + unsigned int max_cores;
>>> +};
>>> +
>>> +static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS];
>>> +
>>> +static int
>>> +qcom_cpufreq_fw_target_index(struct cpufreq_policy *policy, unsigned
>>> int index)
>>> +{
>>> + struct cpufreq_qcom *c = policy->driver_data;
>>> +
>>> + if (index >= LUT_MAX_ENTRIES) {
>>> + dev_err(c->dev,
>>> + "Passing an index (%u) that's greater than max (%d)\n",
>>> + index, LUT_MAX_ENTRIES - 1);
>>> + return -EINVAL;
>>> + }
>>
>> This is never going to happen unless there is a bug in cpufreq core.
>> You are allocating only 40 entries for the cpufreq table and this will
>> always be 0-39. None of the other drivers is checking this I believe
>> and neither should you. This is the only routine which will get call
>> very frequently and we better not add unnecessary stuff here.
>>
Yes, I would remove this in the next series.
>>> + writel_relaxed(index, c->perf_base);
>>> +
>>> + /* Make sure the write goes through before proceeding */
>>> + mb();
>>
>> Btw what happens right after this is done ? Are we guaranteed that the
>> frequency is updated in the hardware after this ? What about enabling
>> fast-switch for your platform ? Look at drivers/cpufreq/scmi-cpufreq.c
>> to see how that is done.
>
> Yeah, I don't think this is needed really.
>
Just want to make sure it doesn't really sit in the write buffer before
return.
>>
>>> + return 0;
>>> +}
>>> +
>>> +static unsigned int qcom_cpufreq_fw_get(unsigned int cpu)
>>> +{
>>> + struct cpufreq_qcom *c;
>>> + unsigned int index;
>>> +
>>> + c = qcom_freq_domain_map[cpu];
>>> + if (!c)
>>> + return -ENODEV;
>>
>> Return 0 on error here.
>>
Would update this in the next patch.
>>> +
>>> + index = readl_relaxed(c->perf_base);
>>> + index = min(index, LUT_MAX_ENTRIES - 1);
>>
>> Will the hardware ever read a value over 39 here ?
>
> The register could be initialized to whatever before the kernel is
> brought up. Don't want to depend on it being correct to avoid out of
> bounds access that could leak data.
>
>
>>> +
>>> + return c->table[index].frequency;
>>> +}
>>> +
>>> +static int qcom_cpufreq_fw_cpu_init(struct cpufreq_policy *policy)
>>> +{
>>> + struct cpufreq_qcom *c;
>>> +
>>> + c = qcom_freq_domain_map[policy->cpu];
>>> + if (!c) {
>>> + pr_err("No scaling support for CPU%d\n", policy->cpu);
>>> + return -ENODEV;
>>> + }
>>> +
>>> + cpumask_copy(policy->cpus, &c->related_cpus);
>>> + policy->freq_table = c->table;
>>> + policy->driver_data = c;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static struct freq_attr *qcom_cpufreq_fw_attr[] = {
>>> + &cpufreq_freq_attr_scaling_available_freqs,
>>> + &cpufreq_freq_attr_scaling_boost_freqs,
>>> + NULL
>>> +};
>>> +
>>> +static struct cpufreq_driver cpufreq_qcom_fw_driver = {
>>> + .flags = CPUFREQ_STICKY | CPUFREQ_NEED_INITIAL_FREQ_CHECK |
>>> + CPUFREQ_HAVE_GOVERNOR_PER_POLICY,
>>> + .verify = cpufreq_generic_frequency_table_verify,
>>> + .target_index = qcom_cpufreq_fw_target_index,
>>> + .get = qcom_cpufreq_fw_get,
>>> + .init = qcom_cpufreq_fw_cpu_init,
>>> + .name = "qcom-cpufreq-fw",
>>> + .attr = qcom_cpufreq_fw_attr,
>>> + .boost_enabled = true,
>>> +};
>>> +
>>> +static int qcom_read_lut(struct platform_device *pdev,
>>> + struct cpufreq_qcom *c)
>>> +{
>>> + struct device *dev = &pdev->dev;
>>> + u32 data, src, lval, i, core_count, prev_cc;
>>> +
>>> + c->table = devm_kcalloc(dev, LUT_MAX_ENTRIES + 1,
>>> + sizeof(*c->table), GFP_KERNEL);
>>> + if (!c->table)
>>> + return -ENOMEM;
>>> +
>>> + for (i = 0; i < LUT_MAX_ENTRIES; i++) {
>>> + data = readl_relaxed(c->lut_base + i * LUT_ROW_SIZE);
>>> + src = ((data & GENMASK(31, 30)) >> 30);
>>> + lval = (data & GENMASK(7, 0));
>>> + core_count = CORE_COUNT_VAL(data);
>>> +
>>> + if (!src)
>>> + c->table[i].frequency = INIT_RATE / 1000;
>>> + else
>>> + c->table[i].frequency = XO_RATE * lval / 1000;
>>> +
>>> + c->table[i].driver_data = c->table[i].frequency;
>>
>> Why do you need to use driver_data here? Why can't you simple use
>> frequency field in the below conditional expressions ?
>>
The frequency field would be marked INVALID in case the core count does
not match and the frequency data would be lost.
>>> +
>>> + dev_dbg(dev, "index=%d freq=%d, core_count %d\n",
>>> + i, c->table[i].frequency, core_count);
>>> +
>>> + if (core_count != c->max_cores)
>>> + c->table[i].frequency = CPUFREQ_ENTRY_INVALID;
>>> +
>>> + /*
>>> + * Two of the same frequencies with the same core counts means
>>> + * end of table.
>>> + */
>>> + if (i > 0 && c->table[i - 1].driver_data ==
>>> + c->table[i].driver_data && prev_cc == core_count) {
>>> + struct cpufreq_frequency_table *prev = &c->table[i - 1];
>>> +
>>> + if (prev->frequency == CPUFREQ_ENTRY_INVALID) {
>>
>> There can only be a single boost frequency at max ?
>
> As of now, yes. If that changes, we'll change this code later.
>
>>> + prev->flags = CPUFREQ_BOOST_FREQ;
>>> + prev->frequency = prev->driver_data;
>>
>> Okay you are using driver_data as a local variable to keep this value
>> safe which you might have overwritten. Maybe use a simple variable
>> prev_freq for this. It would be much more readable in that case and
>> you wouldn't end up abusing the driver_data field.
>>
Please correct me, currently the driver_data is not used by cpufreq core
and that was the reason to use it. In case you still think it is not a
good way to handle it, I would try to handle it differently.
>>> + }
>>> +
>>> + break;
>>> + }
>>> + prev_cc = core_count;
>>> + }
>>> + c->table[i].frequency = CPUFREQ_TABLE_END;
>>
>> Wouldn't you end up writing on c->table[40].frequency here if there
>> are 40 frequency value present ?
>
> Yeah, the loop condition needs to be fixed.
>
The table allocation is done for 'LUT_MAX_ENTRIES + 1'.
Yes in case we have all [0-39] (i.e 40 entries) read from the hardware,
would store the same and mark the 40th index as table end. Please
correct if I missed something in your comment.
> -Saravana
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation.
--
^ permalink raw reply
* Re: [PATCH v2] schedutil: Allow cpufreq requests to be made even when kthread kicked
From: Patrick Bellasi @ 2018-05-22 10:38 UTC (permalink / raw)
To: Viresh Kumar
Cc: Joel Fernandes, Joel Fernandes (Google.), linux-kernel,
Rafael J . Wysocki, Peter Zijlstra, Ingo Molnar, Juri Lelli,
Luca Abeni, Todd Kjos, claudio, kernel-team, linux-pm
In-Reply-To: <20180522102305.uxph4u4o2zrvu4tx@vireshk-i7>
Hi Viresh,
thanks for clarifying...
On 22-May 15:53, Viresh Kumar wrote:
> On 21-05-18, 10:20, Joel Fernandes wrote:
> > On Mon, May 21, 2018 at 06:00:50PM +0100, Patrick Bellasi wrote:
> > > If that's the case, this means that if, for example, during a
> > > frequency switch you get a request to reduce the frequency (e.g.
> > > deadline task passing the 0-lag time) and right after a request to
> > > increase the frequency (e.g. the current FAIR task tick)... you will
> > > enqueue a freq drop followed by a freq increase and actually do two
> > > frequency hops?
>
> I don't think so.
>
> Consider the kthread as running currently and has just cleared the
> work_in_progress flag. Sched update comes at that time and we decide
> to reduce the frequency, we queue another work and update next_freq.
> Now if another sched update comes before the kthread finishes its
> previous loop, we will simply update next_freq and return. So when the
> next time kthread runs, it will pick the most recent update.
Mmm... right... looking better at the two execution contexts:
// A) Frequency update requests
sugov_update_commit() {
sg_policy->next_freq = next_freq;
if (!sg_policy->work_in_progress) {
sg_policy->work_in_progress = true;
irq_work_queue(&sg_policy->irq_work);
}
}
// B) Actual frequency updates
sugov_work() {
freq = sg_policy->next_freq;
sg_policy->work_in_progress = false;
__cpufreq_driver_target(sg_policy->policy, freq, CPUFREQ_RELATION_L);
}
It's true that A will enqueue only one B at the first next_freq update
and then it will keep just updating the next_freq.
Thus, we should be ensure to have always just one kwork pending in the
queue.
> Where is the problem both of you see ?
Perhaps the confusion comes just from the naming of
"work_in_progress", which is confusing since we use it now to
represent that we enqueued a frequency change and we wait for the
kwork to pick it up.
Maybe it can help to rename it to something like kwork_queued or
update_pending, update_queued... ?
--
#include <best/regards.h>
Patrick Bellasi
^ permalink raw reply
* Re: [PATCH v2] schedutil: Allow cpufreq requests to be made even when kthread kicked
From: Viresh Kumar @ 2018-05-22 10:34 UTC (permalink / raw)
To: Joel Fernandes (Google.)
Cc: linux-kernel, Joel Fernandes (Google), Rafael J . Wysocki,
Peter Zijlstra, Ingo Molnar, Patrick Bellasi, Juri Lelli,
Luca Abeni, Todd Kjos, claudio, kernel-team, linux-pm
In-Reply-To: <20180518185501.173552-1-joel@joelfernandes.org>
Okay, me and Rafael were discussing this patch, locking and races around this.
On 18-05-18, 11:55, Joel Fernandes (Google.) wrote:
> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> index e13df951aca7..5c482ec38610 100644
> --- a/kernel/sched/cpufreq_schedutil.c
> +++ b/kernel/sched/cpufreq_schedutil.c
> @@ -92,9 +92,6 @@ static bool sugov_should_update_freq(struct sugov_policy *sg_policy, u64 time)
> !cpufreq_can_do_remote_dvfs(sg_policy->policy))
> return false;
>
> - if (sg_policy->work_in_progress)
> - return false;
> -
> if (unlikely(sg_policy->need_freq_update)) {
> sg_policy->need_freq_update = false;
> /*
> @@ -128,7 +125,7 @@ static void sugov_update_commit(struct sugov_policy *sg_policy, u64 time,
>
> policy->cur = next_freq;
> trace_cpu_frequency(next_freq, smp_processor_id());
> - } else {
> + } else if (!sg_policy->work_in_progress) {
> sg_policy->work_in_progress = true;
> irq_work_queue(&sg_policy->irq_work);
> }
> @@ -291,6 +288,13 @@ static void sugov_update_single(struct update_util_data *hook, u64 time,
>
> ignore_dl_rate_limit(sg_cpu, sg_policy);
>
> + /*
> + * For slow-switch systems, single policy requests can't run at the
> + * moment if update is in progress, unless we acquire update_lock.
> + */
> + if (sg_policy->work_in_progress)
> + return;
> +
> if (!sugov_should_update_freq(sg_policy, time))
> return;
>
> @@ -382,13 +386,27 @@ sugov_update_shared(struct update_util_data *hook, u64 time, unsigned int flags)
> static void sugov_work(struct kthread_work *work)
> {
> struct sugov_policy *sg_policy = container_of(work, struct sugov_policy, work);
> + unsigned int freq;
> + unsigned long flags;
> +
> + /*
> + * Hold sg_policy->update_lock shortly to handle the case where:
> + * incase sg_policy->next_freq is read here, and then updated by
> + * sugov_update_shared just before work_in_progress is set to false
> + * here, we may miss queueing the new update.
> + *
> + * Note: If a work was queued after the update_lock is released,
> + * sugov_work will just be called again by kthread_work code; and the
> + * request will be proceed before the sugov thread sleeps.
> + */
> + raw_spin_lock_irqsave(&sg_policy->update_lock, flags);
> + freq = sg_policy->next_freq;
> + sg_policy->work_in_progress = false;
> + raw_spin_unlock_irqrestore(&sg_policy->update_lock, flags);
>
> mutex_lock(&sg_policy->work_lock);
> - __cpufreq_driver_target(sg_policy->policy, sg_policy->next_freq,
> - CPUFREQ_RELATION_L);
> + __cpufreq_driver_target(sg_policy->policy, freq, CPUFREQ_RELATION_L);
> mutex_unlock(&sg_policy->work_lock);
> -
> - sg_policy->work_in_progress = false;
> }
And I do see a race here for single policy systems doing slow switching.
Kthread Sched update
sugov_work() sugov_update_single()
lock();
// The CPU is free to rearrange below
// two in any order, so it may clear
// the flag first and then read next
// freq. Lets assume it does.
work_in_progress = false
if (work_in_progress)
return;
sg_policy->next_freq = 0;
freq = sg_policy->next_freq;
sg_policy->next_freq = real-next-freq;
unlock();
Is the above theory right or am I day dreaming ? :)
--
viresh
^ permalink raw reply
* Re: [PATCH v2] schedutil: Allow cpufreq requests to be made even when kthread kicked
From: Patrick Bellasi @ 2018-05-22 10:26 UTC (permalink / raw)
To: Joel Fernandes
Cc: linux-kernel, Viresh Kumar, Rafael J . Wysocki, Peter Zijlstra,
Ingo Molnar, Juri Lelli, Luca Abeni, Todd Kjos, claudio,
kernel-team, linux-pm
In-Reply-To: <20180521180557.GA40541@joelaf.mtv.corp.google.com>
On 21-May 11:05, Joel Fernandes wrote:
> On Mon, May 21, 2018 at 11:50:55AM +0100, Patrick Bellasi wrote:
> > On 18-May 11:55, Joel Fernandes (Google.) wrote:
> > > From: "Joel Fernandes (Google)" <joel@joelfernandes.org>
> > >
> > > Currently there is a chance of a schedutil cpufreq update request to be
> > > dropped if there is a pending update request. This pending request can
> > > be delayed if there is a scheduling delay of the irq_work and the wake
> > > up of the schedutil governor kthread.
> > >
> > > A very bad scenario is when a schedutil request was already just made,
> > > such as to reduce the CPU frequency, then a newer request to increase
> > > CPU frequency (even sched deadline urgent frequency increase requests)
> > > can be dropped, even though the rate limits suggest that its Ok to
> > > process a request. This is because of the way the work_in_progress flag
> > > is used.
> > >
> > > This patch improves the situation by allowing new requests to happen
> > > even though the old one is still being processed. Note that in this
> > > approach, if an irq_work was already issued, we just update next_freq
> > > and don't bother to queue another request so there's no extra work being
> > > done to make this happen.
> >
> > Maybe I'm missing something but... is not this patch just a partial
> > mitigation of the issue you descrive above?
> >
> > If a DL freq increase is queued, with this patch we store the request
> > but we don't actually increase the frequency until the next schedutil
> > update, which can be one tick away... isn't it?
> >
> > If that's the case, maybe something like the following can complete
> > the cure?
> >
> > ---8<---
> > #define SUGOV_FREQ_NONE 0
> >
> > static unsigned int sugov_work_update(struct sugov_policy *sg_policy,
> > unsigned int prev_freq)
> > {
> > unsigned long irq_flags;
> > bool update_freq = true;
> > unsigned int next_freq;
> >
> > /*
> > * Hold sg_policy->update_lock shortly to handle the case where:
> > * incase sg_policy->next_freq is read here, and then updated by
> > * sugov_update_shared just before work_in_progress is set to false
> > * here, we may miss queueing the new update.
> > *
> > * Note: If a work was queued after the update_lock is released,
> > * sugov_work will just be called again by kthread_work code; and the
> > * request will be proceed before the sugov thread sleeps.
> > */
> > raw_spin_lock_irqsave(&sg_policy->update_lock, irq_flags);
> > next_freq = sg_policy->next_freq;
> > sg_policy->work_in_progress = false;
> > if (prev_freq == next_freq)
> > update_freq = false;
>
> About this patch on top of mine, I believe this check is already being done
> by sugov_update_commit? :
No, that check is different...
>
> static void sugov_update_commit(struct sugov_policy *sg_policy, u64 time,
> unsigned int next_freq)
> {
> struct cpufreq_policy *policy = sg_policy->policy;
>
> if (sg_policy->next_freq == next_freq)
> return;
>
> sg_policy->next_freq = next_freq;
> sg_policy->last_freq_update_time = time;
> ----
... in my snippet the check is required to verify if, once a freq
swich has been completed by the kthread, the sugov_update_commit has
actually committed a new and different frequency wrt the one the
kthread has just configured.
It means we will have two async paths:
1. sugov_update_commit()
which updates sg_policy->next_freq
2. sugov_work_update()
which will run in a loop until the last freq it configures matches
with the current value of sg_policy->next_freq
But again, as we was discussing yesterday, we can have these
additional bits in a following patch on top of your.
--
#include <best/regards.h>
Patrick Bellasi
^ permalink raw reply
* Re: [PATCH v2] schedutil: Allow cpufreq requests to be made even when kthread kicked
From: Viresh Kumar @ 2018-05-22 10:23 UTC (permalink / raw)
To: Joel Fernandes
Cc: Patrick Bellasi, Joel Fernandes (Google.), linux-kernel,
Rafael J . Wysocki, Peter Zijlstra, Ingo Molnar, Juri Lelli,
Luca Abeni, Todd Kjos, claudio, kernel-team, linux-pm
In-Reply-To: <20180521172001.GA21678@joelaf.mtv.corp.google.com>
On 21-05-18, 10:20, Joel Fernandes wrote:
> On Mon, May 21, 2018 at 06:00:50PM +0100, Patrick Bellasi wrote:
> > If that's the case, this means that if, for example, during a
> > frequency switch you get a request to reduce the frequency (e.g.
> > deadline task passing the 0-lag time) and right after a request to
> > increase the frequency (e.g. the current FAIR task tick)... you will
> > enqueue a freq drop followed by a freq increase and actually do two
> > frequency hops?
I don't think so.
Consider the kthread as running currently and has just cleared the
work_in_progress flag. Sched update comes at that time and we decide
to reduce the frequency, we queue another work and update next_freq.
Now if another sched update comes before the kthread finishes its
previous loop, we will simply update next_freq and return. So when the
next time kthread runs, it will pick the most recent update.
Where is the problem both of you see ?
--
viresh
^ permalink raw reply
* Re: [PATCH v2] schedutil: Allow cpufreq requests to be made even when kthread kicked
From: Rafael J. Wysocki @ 2018-05-22 10:02 UTC (permalink / raw)
To: Joel Fernandes
Cc: Rafael J. Wysocki, Viresh Kumar, Joel Fernandes (Google.),
Linux Kernel Mailing List, Rafael J . Wysocki, Peter Zijlstra,
Ingo Molnar, Patrick Bellasi, Juri Lelli, Luca Abeni, Todd Kjos,
Claudio Scordino, kernel-team, Linux PM
In-Reply-To: <20180521161346.GB14065@joelaf.mtv.corp.google.com>
On Mon, May 21, 2018 at 6:13 PM, Joel Fernandes <joel@joelfernandes.org> wrote:
> On Mon, May 21, 2018 at 10:29:52AM +0200, Rafael J. Wysocki wrote:
>> On Mon, May 21, 2018 at 7:14 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>> > On 18-05-18, 11:55, Joel Fernandes (Google.) wrote:
>> >> From: "Joel Fernandes (Google)" <joel@joelfernandes.org>
>> >>
>> >> Currently there is a chance of a schedutil cpufreq update request to be
>> >> dropped if there is a pending update request. This pending request can
>> >> be delayed if there is a scheduling delay of the irq_work and the wake
>> >> up of the schedutil governor kthread.
>> >>
>> >> A very bad scenario is when a schedutil request was already just made,
>> >> such as to reduce the CPU frequency, then a newer request to increase
>> >> CPU frequency (even sched deadline urgent frequency increase requests)
>> >> can be dropped, even though the rate limits suggest that its Ok to
>> >> process a request. This is because of the way the work_in_progress flag
>> >> is used.
>> >>
>> >> This patch improves the situation by allowing new requests to happen
>> >> even though the old one is still being processed. Note that in this
>> >> approach, if an irq_work was already issued, we just update next_freq
>> >> and don't bother to queue another request so there's no extra work being
>> >> done to make this happen.
>> >
>> > Now that this isn't an RFC anymore, you shouldn't have added below
>> > paragraph here. It could go to the comments section though.
>> >
>> >> I had brought up this issue at the OSPM conference and Claudio had a
>> >> discussion RFC with an alternate approach [1]. I prefer the approach as
>> >> done in the patch below since it doesn't need any new flags and doesn't
>> >> cause any other extra overhead.
>> >>
>> >> [1] https://patchwork.kernel.org/patch/10384261/
>> >>
>> >> LGTMed-by: Viresh Kumar <viresh.kumar@linaro.org>
>> >> LGTMed-by: Juri Lelli <juri.lelli@redhat.com>
>> >
>> > Looks like a Tag you just invented ? :)
>>
>> Yeah.
>>
>> The LGTM from Juri can be converned into an ACK silently IMO. That
>> said I have added Looks-good-to: tags to a couple of commits. :-)
>
> Cool, I'll covert them to Acks :-)
So it looks like I should expect an update of this patch, right?
Or do you prefer the current one to be applied and work on top of it?
> [..]
>> >> v1 -> v2: Minor style related changes.
>> >>
>> >> kernel/sched/cpufreq_schedutil.c | 34 ++++++++++++++++++++++++--------
>> >> 1 file changed, 26 insertions(+), 8 deletions(-)
>> >>
>> >> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
>> >> index e13df951aca7..5c482ec38610 100644
>> >> --- a/kernel/sched/cpufreq_schedutil.c
>> >> +++ b/kernel/sched/cpufreq_schedutil.c
>> >> @@ -92,9 +92,6 @@ static bool sugov_should_update_freq(struct sugov_policy *sg_policy, u64 time)
>> >> !cpufreq_can_do_remote_dvfs(sg_policy->policy))
>> >> return false;
>> >>
>> >> - if (sg_policy->work_in_progress)
>> >> - return false;
>> >> -
>> >> if (unlikely(sg_policy->need_freq_update)) {
>> >> sg_policy->need_freq_update = false;
>> >> /*
>> >> @@ -128,7 +125,7 @@ static void sugov_update_commit(struct sugov_policy *sg_policy, u64 time,
>> >>
>> >> policy->cur = next_freq;
>> >> trace_cpu_frequency(next_freq, smp_processor_id());
>> >> - } else {
>> >> + } else if (!sg_policy->work_in_progress) {
>> >> sg_policy->work_in_progress = true;
>> >> irq_work_queue(&sg_policy->irq_work);
>> >> }
>> >> @@ -291,6 +288,13 @@ static void sugov_update_single(struct update_util_data *hook, u64 time,
>> >>
>> >> ignore_dl_rate_limit(sg_cpu, sg_policy);
>> >>
>> >> + /*
>> >> + * For slow-switch systems, single policy requests can't run at the
>> >> + * moment if update is in progress, unless we acquire update_lock.
>> >> + */
>> >> + if (sg_policy->work_in_progress)
>> >> + return;
>> >> +
>> >
>> > I would still want this to go away :)
>> >
>> > @Rafael, will it be fine to get locking in place for unshared policy
>> > platforms ?
>>
>> As long as it doesn't affect the fast switch path in any way.
>
> I just realized that on a single policy switch that uses the governor thread,
> there will be 1 thread per-CPU. The sugov_update_single will be called on the
> same CPU with interrupts disabled.
sugov_update_single() doesn't have to run on the target CPU.
> In sugov_work, we are doing a
> raw_spin_lock_irqsave which also disables interrupts. So I don't think
> there's any possibility of a race happening on the same CPU between the
> frequency update request and the sugov_work executing. In other words, I feel
> we can drop the above if (..) statement for single policies completely and
> only keep the changes for the shared policy. Viresh since you brought up the
> single policy issue initially which made me add this if statememnt, could you
> let me know if you agree with what I just said?
Which is why you need the spinlock too.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox