[v3,6/6] cpufreq: schedutil: ignore sugov kthreads

Message ID 20171130114723.29210-7-patrick.bellasi@arm.com
State New
Headers show
Series
  • [v3,1/6] cpufreq: schedutil: reset sg_cpus's flags at IDLE enter
Related show

Commit Message

Patrick Bellasi Nov. 30, 2017, 11:47 a.m.
In system where multiple CPUs shares the same frequency domain a small
workload on a CPU can still be subject to frequency spikes, generated by
the activation of the sugov's kthread.

Since the sugov kthread is a special RT task, which goal is just that to
activate a frequency transition, it does not make sense for it to bias
the schedutil's frequency selection policy.

This patch exploits the information related to the current task to silently
ignore cpufreq_update_this_cpu() calls, coming from the RT scheduler, while
the sugov kthread is running.

Signed-off-by: Patrick Bellasi <patrick.bellasi@arm.com>

Reviewed-by: Dietmar Eggemann <dietmar.eggemann@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: linux-kernel@vger.kernel.org
Cc: linux-pm@vger.kernel.org

---
Changes from v2:
- rebased on v4.15-rc1
- moved at the end of the stack since considered more controversial
Changes from v1:
- move check before policy spinlock (JuriL)

Change-Id: I4d749458229b6496dd24a8c357be42cd35a739fd
---
 kernel/sched/cpufreq_schedutil.c | 8 ++++++++
 1 file changed, 8 insertions(+)

-- 
2.14.1

Comments

Juri Lelli Nov. 30, 2017, 1:41 p.m. | #1
Hi,

On 30/11/17 11:47, Patrick Bellasi wrote:
> In system where multiple CPUs shares the same frequency domain a small

> workload on a CPU can still be subject to frequency spikes, generated by

> the activation of the sugov's kthread.

> 

> Since the sugov kthread is a special RT task, which goal is just that to

> activate a frequency transition, it does not make sense for it to bias

> the schedutil's frequency selection policy.

> 

> This patch exploits the information related to the current task to silently

> ignore cpufreq_update_this_cpu() calls, coming from the RT scheduler, while

> the sugov kthread is running.

> 

> Signed-off-by: Patrick Bellasi <patrick.bellasi@arm.com>

> Reviewed-by: Dietmar Eggemann <dietmar.eggemann@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: linux-kernel@vger.kernel.org

> Cc: linux-pm@vger.kernel.org

> 

> ---

> Changes from v2:

> - rebased on v4.15-rc1

> - moved at the end of the stack since considered more controversial

> Changes from v1:

> - move check before policy spinlock (JuriL)

> 

> Change-Id: I4d749458229b6496dd24a8c357be42cd35a739fd

> ---

>  kernel/sched/cpufreq_schedutil.c | 8 ++++++++

>  1 file changed, 8 insertions(+)

> 

> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c

> index 3eea8884e61b..a93ad5b0c40d 100644

> --- a/kernel/sched/cpufreq_schedutil.c

> +++ b/kernel/sched/cpufreq_schedutil.c

> @@ -270,6 +270,10 @@ static void sugov_update_single(struct update_util_data *hook, u64 time,

>  	bool rt_mode;

>  	bool busy;

>  

> +	/* Skip updates generated by sugov kthreads */

> +	if (unlikely(current == sg_policy->thread))

> +		return;

> +

>  	sugov_set_iowait_boost(sg_cpu, time, flags);

>  	sg_cpu->last_update = time;

>  

> @@ -356,6 +360,10 @@ static void sugov_update_shared(struct update_util_data *hook, u64 time,

>  	unsigned int next_f;

>  	bool rt_mode;

>  

> +	/* Skip updates generated by sugov kthreads */

> +	if (unlikely(current == sg_policy->thread))

> +		return;

> +

>  	raw_spin_lock(&sg_policy->update_lock);

>  

>  	sugov_get_util(&util, &max, sg_cpu->cpu);


If the DL changes (which I shall post again as soon as tip/sched/core is
bumped up to 4.15-rc1) get in first, this is going to be useless (as the
DL kthread gets ignored by the scheduling class itself). But, this looks
good to me "in the meantime".

Reviewed-by: Juri Lelli <juri.lelli@redhat.com>


Best,

Juri
Juri Lelli Nov. 30, 2017, 4:12 p.m. | #2
On 30/11/17 16:02, Patrick Bellasi wrote:
> On 30-Nov 14:41, Juri Lelli wrote:


[...]

> > If the DL changes (which I shall post again as soon as tip/sched/core is

> > bumped up to 4.15-rc1) get in first, this is going to be useless (as the

> > DL kthread gets ignored by the scheduling class itself). But, this looks

> > good to me "in the meantime".

> 

> Just to better understand, you mean that the DL kthread does not send

> out schedutil updates?


It doesn't have a "proper" bandwidth (utilization) :/. So it gets
"unnoticed" and schedutil updates are not triggered.

> 

> If that's the case I agree we can discard this patch... that's also

> one of the reasons why I move it at the end of this series.


Not sure about this though, not my call :). I guess this still helps
until we get the DL changes in.

Best,

Juri
Patrick Bellasi Nov. 30, 2017, 4:42 p.m. | #3
On 30-Nov 17:12, Juri Lelli wrote:
> On 30/11/17 16:02, Patrick Bellasi wrote:

> > On 30-Nov 14:41, Juri Lelli wrote:

> 

> [...]

> 

> > > If the DL changes (which I shall post again as soon as tip/sched/core is

> > > bumped up to 4.15-rc1) get in first, this is going to be useless (as the

> > > DL kthread gets ignored by the scheduling class itself). But, this looks

> > > good to me "in the meantime".

> > 

> > Just to better understand, you mean that the DL kthread does not send

> > out schedutil updates?

> 

> It doesn't have a "proper" bandwidth (utilization) :/. So it gets

> "unnoticed" and schedutil updates are not triggered.


Ah, right... that was the "let CBS ignore this DL task" solution! ;-)

> > 

> > If that's the case I agree we can discard this patch... that's also

> > one of the reasons why I move it at the end of this series.

> 

> Not sure about this though, not my call :). I guess this still helps

> until we get the DL changes in.


Yes, agree... well Viresh had some concerns about this patch.
Let see what he thinks.
 
> Best,

> 

> Juri


-- 
#include <best/regards.h>

Patrick Bellasi
Viresh Kumar Dec. 7, 2017, 9:24 a.m. | #4
On 30-11-17, 16:42, Patrick Bellasi wrote:
> On 30-Nov 17:12, Juri Lelli wrote:

> > Not sure about this though, not my call :). I guess this still helps

> > until we get the DL changes in.

> 

> Yes, agree... well Viresh had some concerns about this patch.

> Let see what he thinks.


And the problem is that we never got to a conclusion in V2 for this
patch as well, as you never responded to the last set of queries I
had. Unless we finish the ongoing conversations, we will have the same
queries again.

IOW, can you explain the exact scenario where this patch will be
helpful ? I am not sure if this patch is that helpful at all :)

-- 
viresh
Patrick Bellasi Dec. 7, 2017, 3:47 p.m. | #5
On 07-Dec 14:54, Viresh Kumar wrote:
> On 30-11-17, 16:42, Patrick Bellasi wrote:

> > On 30-Nov 17:12, Juri Lelli wrote:

> > > Not sure about this though, not my call :). I guess this still helps

> > > until we get the DL changes in.

> > 

> > Yes, agree... well Viresh had some concerns about this patch.

> > Let see what he thinks.

> 

> And the problem is that we never got to a conclusion in V2 for this

> patch as well, as you never responded to the last set of queries I

> had. Unless we finish the ongoing conversations, we will have the same

> queries again.

> 

> IOW, can you explain the exact scenario where this patch will be

> helpful ? I am not sure if this patch is that helpful at all :)


My use case is considering a small FAIR task which triggers an OPP
change while running on a CPU which is different from the one running
the schedutil kthread.

If we go for a "clear flags semantics", where the RT class clears its
flag as soon as there are not more RT tasks runnable, then this patch
likely is not more required.

Otherwise, there can be corner cases in which the RT flag remain set in
the kthread CPU thus affecting OPP decisions even then there are not
RT tasks around. Consider for example this scenarios:


CPU0:               | SUGOV |            | SUGOV |
         small FAIR |       | small FAIR |       |
                      flags = RT          ^
                    ^                     |
                    | (A)                 | (B)
                    | sugov_update_util() |
                    |                     | sugov_update_util()
CPU1:    medium growing FAIR task | another FAIR enqueued


In this case:
- in A we set the RT flag
- in B we pick the MAX OPP even if there are only small tasks.


This will not happen with a "clear flags semantics" because we ensure
to release the RT flag every time an RT task exit the CPU.
Thus, if we go for that semantics, we can skip this patch.

-- 
#include <best/regards.h>

Patrick Bellasi

Patch

diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
index 3eea8884e61b..a93ad5b0c40d 100644
--- a/kernel/sched/cpufreq_schedutil.c
+++ b/kernel/sched/cpufreq_schedutil.c
@@ -270,6 +270,10 @@  static void sugov_update_single(struct update_util_data *hook, u64 time,
 	bool rt_mode;
 	bool busy;
 
+	/* Skip updates generated by sugov kthreads */
+	if (unlikely(current == sg_policy->thread))
+		return;
+
 	sugov_set_iowait_boost(sg_cpu, time, flags);
 	sg_cpu->last_update = time;
 
@@ -356,6 +360,10 @@  static void sugov_update_shared(struct update_util_data *hook, u64 time,
 	unsigned int next_f;
 	bool rt_mode;
 
+	/* Skip updates generated by sugov kthreads */
+	if (unlikely(current == sg_policy->thread))
+		return;
+
 	raw_spin_lock(&sg_policy->update_lock);
 
 	sugov_get_util(&util, &max, sg_cpu->cpu);