Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1591212 > unrolled thread

[PATCH 0/6] cpufreq: schedutil: fixes for flags updates

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/6] cpufreq: schedutil: fixes for flags updates Patrick Bellasi <patrick.bellasi@arm.com> - 2017-03-02 16:50 +0100
    [PATCH 3/6] cpufreq: schedutil: ensure max frequency while running RT/DL tasks Patrick Bellasi <patrick.bellasi@arm.com> - 2017-03-02 16:50 +0100
      Re: [PATCH 3/6] cpufreq: schedutil: ensure max frequency while  running RT/DL tasks Viresh Kumar <viresh.kumar@linaro.org> - 2017-03-03 10:00 +0100
        Re: [PATCH 3/6] cpufreq: schedutil: ensure max frequency while  running RT/DL tasks Patrick Bellasi <patrick.bellasi@arm.com> - 2017-03-03 13:50 +0100
    [PATCH 5/6] cpufreq: schedutil: avoid utilisation update when not necessary Patrick Bellasi <patrick.bellasi@arm.com> - 2017-03-02 17:00 +0100
    [PATCH 2/6] cpufreq: schedutil: ignore the sugov kthread for frequencies selections Patrick Bellasi <patrick.bellasi@arm.com> - 2017-03-02 17:00 +0100
      Re: [PATCH 2/6] cpufreq: schedutil: ignore the sugov kthread for  frequencies selections Viresh Kumar <viresh.kumar@linaro.org> - 2017-03-03 07:30 +0100
        Re: [PATCH 2/6] cpufreq: schedutil: ignore the sugov kthread for  frequencies selections Patrick Bellasi <patrick.bellasi@arm.com> - 2017-03-03 14:20 +0100
          Re: [PATCH 2/6] cpufreq: schedutil: ignore the sugov kthread for  frequencies selections Viresh Kumar <viresh.kumar@linaro.org> - 2017-03-06 06:20 +0100
      Re: [PATCH 2/6] cpufreq: schedutil: ignore the sugov kthread for  frequencies selections Steven Rostedt <rostedt@goodmis.org> - 2017-03-06 15:40 +0100
    [PATCH 6/6] sched/rt: fast switch to maximum frequency when RT tasks are scheduled Patrick Bellasi <patrick.bellasi@arm.com> - 2017-03-02 17:00 +0100
    Re: [PATCH 0/6] cpufreq: schedutil: fixes for flags updates Vincent Guittot <vincent.guittot@linaro.org> - 2017-03-02 17:20 +0100
      Re: [PATCH 0/6] cpufreq: schedutil: fixes for flags updates Patrick Bellasi <patrick.bellasi@arm.com> - 2017-03-02 18:40 +0100

#1591212 — [PATCH 0/6] cpufreq: schedutil: fixes for flags updates

The current version of schedutil has some issues related to the management
of update flags used by systems with frequency domains spawning multiple CPUs.

Each time a CPU utilisation update is issued by the scheduler a set of flags
are configured to define (mainly) which class is asking for a utilisation
update. These flags are then used by the frequency selection policy to
identify the OPP to choose.

In the current implementation, CPU flags are overridden each time the
scheduler calls schedutil for an update. Such a behaviour produces issues
in these scenarios, where we assume CPU1 and CPU2 share the same frequency
domain:
a) a RT task which executed on CPU1 can keep the domain at an high frequency
   for a long period of time, even if there are no longer RT tasks on
   CPUs in that domain
b) a FAIR task co-scheduled in the same CPU of a RT task can override the
   flags configured by the RT task and potentially this can cause an
   unwanted frequency drop

These misbehaviours have been verified using a set of simple rt-app based
synthetic workloads, running on a ARM's Juno board, and results are
available in this Notebook [1].

This series proposes a set of fixes for the aforementioned issues as well
as a small improvement to speedup the selection of the maximum frequency
when RT tasks enter a CPU.

This series is based on top of today's tip/sched/core and is public available
from this repository:

  git://www.linux-arm.com/linux-pb eas/schedutil/flags_fixes

Cheers Patrick

[1] https://gist.github.com/d6a21b459a18091b2b058668a550010d

Patrick Bellasi (6):
  cpufreq: schedutil: reset sg_cpus's flags at IDLE enter
  cpufreq: schedutil: ignore the sugov kthread for frequencies
    selections
  cpufreq: schedutil: ensure max frequency while running RT/DL tasks
  cpufreq: schedutil: relax rate-limiting while running RT/DL tasks
  cpufreq: schedutil: avoid utilisation update when not necessary
  sched/rt: fast switch to maximum frequency when RT tasks are scheduled

 include/linux/sched.h            |  1 +
 kernel/sched/cpufreq_schedutil.c | 59 ++++++++++++++++++++++++++++++++++------
 kernel/sched/idle_task.c         |  4 +++
 kernel/sched/rt.c                | 15 ++++++++--
 4 files changed, 68 insertions(+), 11 deletions(-)

-- 
2.7.4

[toc] | [next] | [standalone]

#1591213 — [PATCH 3/6] cpufreq: schedutil: ensure max frequency while running RT/DL tasks

The policy in use for RT/DL tasks sets the maximum frequency when a task
in these classes calls for a cpufreq_update_this_cpu().  However, the
current implementation might cause a frequency drop while a RT/DL task
is still running, just because for example a FAIR task wakes up and is
enqueued in the same CPU.

This issue is due to the sg_cpu's flags being overwritten at each call
of sugov_update_*. The wakeup of a FAIR task resets the flags and can
trigger a frequency update thus affecting the currently running RT/DL
task.

This can be fixed, in shared frequency domains, by adding (instead of
overwriting) the new flags before triggering a frequency update.  This
grants to stay at least at the frequency requested by the RT/DL class,
which is the maximum one for the time being, but can also be lower when
for example DL will be extended to provide a precise bandwidth
requirement.

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: linux-kernel@vger.kernel.org
Cc: linux-pm@vger.kernel.org
---
 kernel/sched/cpufreq_schedutil.c | 32 +++++++++++++++++++++++++++++---
 1 file changed, 29 insertions(+), 3 deletions(-)

diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
index a3fe5e4..b98a167 100644
--- a/kernel/sched/cpufreq_schedutil.c
+++ b/kernel/sched/cpufreq_schedutil.c
@@ -196,10 +196,21 @@ static void sugov_update_single(struct update_util_data *hook, u64 time,
 				unsigned int flags)
 {
 	struct sugov_cpu *sg_cpu = container_of(hook, struct sugov_cpu, update_util);
+	struct task_struct *curr = cpu_curr(smp_processor_id());
 	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 rt_mode;
+
+	/*
+	 * While RT/DL tasks are running we do not want FAIR tasks to
+	 * overvrite this CPU's flags, still we can update utilization and
+	 * frequency (if required/possible) to be fair with these tasks.
+	 */
+	rt_mode = task_has_dl_policy(curr) ||
+		  task_has_rt_policy(curr) ||
+		  (flags & SCHED_CPUFREQ_RT_DL);
 
 	sugov_set_iowait_boost(sg_cpu, time, flags);
 	sg_cpu->last_update = time;
@@ -207,7 +218,7 @@ static void sugov_update_single(struct update_util_data *hook, u64 time,
 	if (!sugov_should_update_freq(sg_policy, time))
 		return;
 
-	if (flags & SCHED_CPUFREQ_RT_DL) {
+	if (rt_mode) {
 		next_f = policy->cpuinfo.max_freq;
 	} else {
 		sugov_get_util(&util, &max);
@@ -278,6 +289,7 @@ static void sugov_update_shared(struct update_util_data *hook, u64 time,
 	struct task_struct *curr = cpu_curr(cpu);
 	unsigned long util, max;
 	unsigned int next_f;
+	bool rt_mode;
 
 	sugov_get_util(&util, &max);
 
@@ -293,15 +305,29 @@ static void sugov_update_shared(struct update_util_data *hook, u64 time,
 	if (curr == sg_policy->thread)
 		goto done;
 
+	/*
+	 * While RT/DL tasks are running we do not want FAIR tasks to
+	 * overwrite this CPU's flags, still we can update utilization and
+	 * frequency (if required/possible) to be fair with these tasks.
+	 */
+	rt_mode = task_has_dl_policy(curr) ||
+		  task_has_rt_policy(curr) ||
+		  (flags & SCHED_CPUFREQ_RT_DL);
+	if (rt_mode)
+		sg_cpu->flags |= flags;
+	else
+		sg_cpu->flags = flags;
+
 	sg_cpu->util = util;
 	sg_cpu->max = max;
-	sg_cpu->flags = flags;
 
 	sugov_set_iowait_boost(sg_cpu, time, flags);
 	sg_cpu->last_update = time;
 
 	if (sugov_should_update_freq(sg_policy, time)) {
-		next_f = sugov_next_freq_shared(sg_cpu, util, max, flags);
+		next_f = sg_policy->policy->cpuinfo.max_freq;
+		if (!rt_mode)
+			next_f = sugov_next_freq_shared(sg_cpu, util, max, flags);
 		sugov_update_commit(sg_policy, time, next_f);
 	}
 
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1591759 — Re: [PATCH 3/6] cpufreq: schedutil: ensure max frequency while running RT/DL tasks

On 02-03-17, 15:45, Patrick Bellasi wrote:
> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> @@ -293,15 +305,29 @@ static void sugov_update_shared(struct update_util_data *hook, u64 time,
>  	if (curr == sg_policy->thread)
>  		goto done;
>  
> +	/*
> +	 * While RT/DL tasks are running we do not want FAIR tasks to
> +	 * overwrite this CPU's flags, still we can update utilization and
> +	 * frequency (if required/possible) to be fair with these tasks.
> +	 */
> +	rt_mode = task_has_dl_policy(curr) ||
> +		  task_has_rt_policy(curr) ||
> +		  (flags & SCHED_CPUFREQ_RT_DL);
> +	if (rt_mode)
> +		sg_cpu->flags |= flags;
> +	else
> +		sg_cpu->flags = flags;

This looks so hacked up :)

Wouldn't it be better to let the scheduler tell us what all kind of tasks it has
in the rq of a CPU and pass a mask of flags? I think it wouldn't be difficult
(or time consuming) for the scheduler to know that, but I am not 100% sure.

IOW, the flags field in cpufreq_update_util() will represent all tasks in the
rq, instead of just the task that is getting enqueued/dequeued..

And obviously we need to get some utilization numbers for the RT and DL tasks
going forward, switching to max isn't going to work for ever :)

-- 
viresh

[toc] | [prev] | [next] | [standalone]

#1591921 — Re: [PATCH 3/6] cpufreq: schedutil: ensure max frequency while running RT/DL tasks

On 03-Mar 14:01, Viresh Kumar wrote:
> On 02-03-17, 15:45, Patrick Bellasi wrote:
> > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> > @@ -293,15 +305,29 @@ static void sugov_update_shared(struct update_util_data *hook, u64 time,
> >  	if (curr == sg_policy->thread)
> >  		goto done;
> >  
> > +	/*
> > +	 * While RT/DL tasks are running we do not want FAIR tasks to
> > +	 * overwrite this CPU's flags, still we can update utilization and
> > +	 * frequency (if required/possible) to be fair with these tasks.
> > +	 */
> > +	rt_mode = task_has_dl_policy(curr) ||
> > +		  task_has_rt_policy(curr) ||
> > +		  (flags & SCHED_CPUFREQ_RT_DL);
> > +	if (rt_mode)
> > +		sg_cpu->flags |= flags;
> > +	else
> > +		sg_cpu->flags = flags;
> 
> This looks so hacked up :)

It is... a bit... :)

> Wouldn't it be better to let the scheduler tell us what all kind of tasks it has
> in the rq of a CPU and pass a mask of flags?

That would definitively report a more consistent view of what's going
on on each CPU.

> I think it wouldn't be difficult (or time consuming) for the
> scheduler to know that, but I am not 100% sure.

Main issue perhaps is that cpufreq_update_{util,this_cpu} are
currently called by the scheduling classes codes and not from the core
scheduler. However I agree that it should be possible to build up such
information and make it available to the scheduling classes code.

I'll have a look at that.

> IOW, the flags field in cpufreq_update_util() will represent all tasks in the
> rq, instead of just the task that is getting enqueued/dequeued..
> 
> And obviously we need to get some utilization numbers for the RT and DL tasks
> going forward, switching to max isn't going to work for ever :)

Regarding this last point, there are WIP patches Juri is working on to
feed DL demands to schedutil, his presentation at last ELC partially
covers these developments:
  https://www.youtube.com/watch?v=wzrcWNIneWY&index=37&list=PLbzoR-pLrL6pSlkQDW7RpnNLuxPq6WVUR

Instead, RT tasks are currently covered by an rt_avg metric which we
already know is not fitting for most purposes.
It seems that the main goal is twofold: move people to DL whenever
possible otherwise live with the go-to-max policy which is the only
sensible solution to satisfy the RT's class main goal, i.e. latency
reduction.

Of course such a go-to-max policy for all RT tasks we already know
that is going to destroy energy on many different mobile scenarios.

As a possible mitigation for that, while still being compliant with
the main RT's class goal, we recently posted the SchedTune v3
proposal:
  https://lkml.org/lkml/2017/2/28/355

In that proposal, the simple usage of CGroups and the new capacity_max
attribute of the (existing) CPU controller should allow to define what
is the "max" value which is just enough to match the latency
constraints of a mobile application without sacrificing too much
energy.

> -- 
> viresh

Cheers Patrick

-- 
#include <best/regards.h>

Patrick Bellasi

[toc] | [prev] | [next] | [standalone]

#1591215 — [PATCH 5/6] cpufreq: schedutil: avoid utilisation update when not necessary

Under certain conditions (i.e. CPU entering idle and current task being
the sugov thread) we can skip a frequency update.
Thus, let's postpone the collection of the FAIR utilisation when really
needed.

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: linux-kernel@vger.kernel.org
Cc: linux-pm@vger.kernel.org
---
 kernel/sched/cpufreq_schedutil.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
index 44bff37..c8ed645 100644
--- a/kernel/sched/cpufreq_schedutil.c
+++ b/kernel/sched/cpufreq_schedutil.c
@@ -296,8 +296,6 @@ static void sugov_update_shared(struct update_util_data *hook, u64 time,
 	unsigned int next_f;
 	bool rt_mode;
 
-	sugov_get_util(&util, &max);
-
 	raw_spin_lock(&sg_policy->update_lock);
 
 	/* CPU is entering IDLE, reset flags without triggering an update */
@@ -323,6 +321,7 @@ static void sugov_update_shared(struct update_util_data *hook, u64 time,
 	else
 		sg_cpu->flags = flags;
 
+	sugov_get_util(&util, &max);
 	sg_cpu->util = util;
 	sg_cpu->max = max;
 
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1591216 — [PATCH 2/6] cpufreq: schedutil: ignore the sugov kthread for frequencies selections

In system where multiple CPUs shares the same frequency domain a small
workload on a CPU can still be subject 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.

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>
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
---
 kernel/sched/cpufreq_schedutil.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
index 084a98b..a3fe5e4 100644
--- a/kernel/sched/cpufreq_schedutil.c
+++ b/kernel/sched/cpufreq_schedutil.c
@@ -274,6 +274,8 @@ static void sugov_update_shared(struct update_util_data *hook, u64 time,
 {
 	struct sugov_cpu *sg_cpu = container_of(hook, struct sugov_cpu, update_util);
 	struct sugov_policy *sg_policy = sg_cpu->sg_policy;
+	unsigned int cpu = smp_processor_id();
+	struct task_struct *curr = cpu_curr(cpu);
 	unsigned long util, max;
 	unsigned int next_f;
 
@@ -287,6 +289,10 @@ static void sugov_update_shared(struct update_util_data *hook, u64 time,
 		goto done;
 	}
 
+	/* Skip updates generated by sugov kthreads */
+	if (curr == sg_policy->thread)
+		goto done;
+
 	sg_cpu->util = util;
 	sg_cpu->max = max;
 	sg_cpu->flags = flags;
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1591679 — Re: [PATCH 2/6] cpufreq: schedutil: ignore the sugov kthread for frequencies selections

On 02-03-17, 15:45, Patrick Bellasi wrote:
> In system where multiple CPUs shares the same frequency domain a small
> workload on a CPU can still be subject 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.
> 
> 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>
> 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
> ---
>  kernel/sched/cpufreq_schedutil.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> index 084a98b..a3fe5e4 100644
> --- a/kernel/sched/cpufreq_schedutil.c
> +++ b/kernel/sched/cpufreq_schedutil.c
> @@ -274,6 +274,8 @@ static void sugov_update_shared(struct update_util_data *hook, u64 time,
>  {
>  	struct sugov_cpu *sg_cpu = container_of(hook, struct sugov_cpu, update_util);
>  	struct sugov_policy *sg_policy = sg_cpu->sg_policy;
> +	unsigned int cpu = smp_processor_id();
> +	struct task_struct *curr = cpu_curr(cpu);

Maybe merge these two as you have done in the next patch.

>  	unsigned long util, max;
>  	unsigned int next_f;
>  
> @@ -287,6 +289,10 @@ static void sugov_update_shared(struct update_util_data *hook, u64 time,
>  		goto done;
>  	}
>  
> +	/* Skip updates generated by sugov kthreads */
> +	if (curr == sg_policy->thread)
> +		goto done;
> +

I always wanted to avoid such hacks when I moved to the RT thread :(

I was discussing the exact same problem with Vincent few days back and
one of the ideas we had was to clear the flags when any RT task is
dequeued from a rq. AFAIU, the problem we are discussing here
shouldn't normally occur while the sugov RT thread is running as the
work_in_progress flag makes sure we don't reevaluate the load while
the RT thread is updating the frequency. The problem perhaps occurs as
the flag for CPU X is never cleared, and so on the next callback from
the scheduler (after the frequency is updated and work_in_progress is
cleared) we move to the highest frequency.

So what about clearing the flags, just like the previous patch, when
the RT or DL task has finished?

Sorry for the noise if it was all nonsense :)

-- 
viresh

[toc] | [prev] | [next] | [standalone]

#1591932 — Re: [PATCH 2/6] cpufreq: schedutil: ignore the sugov kthread for frequencies selections

On 03-Mar 10:49, Viresh Kumar wrote:
> On 02-03-17, 15:45, Patrick Bellasi wrote:
> > In system where multiple CPUs shares the same frequency domain a small
> > workload on a CPU can still be subject 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.
> > 
> > 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>
> > 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
> > ---
> >  kernel/sched/cpufreq_schedutil.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> > index 084a98b..a3fe5e4 100644
> > --- a/kernel/sched/cpufreq_schedutil.c
> > +++ b/kernel/sched/cpufreq_schedutil.c
> > @@ -274,6 +274,8 @@ static void sugov_update_shared(struct update_util_data *hook, u64 time,
> >  {
> >  	struct sugov_cpu *sg_cpu = container_of(hook, struct sugov_cpu, update_util);
> >  	struct sugov_policy *sg_policy = sg_cpu->sg_policy;
> > +	unsigned int cpu = smp_processor_id();
> > +	struct task_struct *curr = cpu_curr(cpu);
> 
> Maybe merge these two as you have done in the next patch.

Yes, better.

> >  	unsigned long util, max;
> >  	unsigned int next_f;
> >  
> > @@ -287,6 +289,10 @@ static void sugov_update_shared(struct update_util_data *hook, u64 time,
> >  		goto done;
> >  	}
> >  
> > +	/* Skip updates generated by sugov kthreads */
> > +	if (curr == sg_policy->thread)
> > +		goto done;
> > +
> 
> I always wanted to avoid such hacks when I moved to the RT thread :(

Indeed, it is a bit of an hack... but still it's true that this is a
"special" RT thread which must not bias OPP selection.

> I was discussing the exact same problem with Vincent few days back and
> one of the ideas we had was to clear the flags when any RT task is
> dequeued from a rq.

That's a possible solution, however at dequeue time we don't know
what's going on right after. We can end up picking up another RT task.
Thus we can reset the flag when it's not really required, and this
open for possible races...

> AFAIU, the problem we are discussing here
> shouldn't normally occur while the sugov RT thread is running as the
> work_in_progress flag makes sure we don't reevaluate the load while
> the RT thread is updating the frequency.

True...

> The problem perhaps occurs as the flag for CPU X is never cleared,
> and so on the next callback from the scheduler (after the frequency
> is updated and work_in_progress is cleared) we move to the highest
> frequency.

... right.

> So what about clearing the flags, just like the previous patch, when
> the RT or DL task has finished?

As a general goal I think it would be useful to feed input only when
the scheduler knows something is going to happen. That's why in the
last patch of these series I'm proposing to remove updates from
update_curr_rt() and replace it with calls in most interesting events,
like enqueue/pickup instead of dequeue.
 
> Sorry for the noise if it was all nonsense :)

That's absolutely not nonsense, thanks for the feedback.

I agree with you that the solution proposed by this patch sound a bit
of an hack, but still we can argue that using an RT task to change an
OPP is by itself a sort-of an hack.

The main downside I see is the condition check for each and every
update. I don't completely like it, but I'm also not completely
convinced by the "always reset" policy at RT tasks dequeue.

 
> -- 
> viresh

Cheers Patrick

-- 
#include <best/regards.h>

Patrick Bellasi

[toc] | [prev] | [next] | [standalone]

#1592941 — Re: [PATCH 2/6] cpufreq: schedutil: ignore the sugov kthread for frequencies selections

On 03-03-17, 12:12, Patrick Bellasi wrote:
> On 03-Mar 10:49, Viresh Kumar wrote:
> > I always wanted to avoid such hacks when I moved to the RT thread :(
> 
> Indeed, it is a bit of an hack... but still it's true that this is a
> "special" RT thread which must not bias OPP selection.

I agree. We have a problem in hand and we need to fix it somehow.

> > I was discussing the exact same problem with Vincent few days back and
> > one of the ideas we had was to clear the flags when any RT task is
> > dequeued from a rq.
> 
> That's a possible solution, however at dequeue time we don't know
> what's going on right after. We can end up picking up another RT task.
> Thus we can reset the flag when it's not really required, and this
> open for possible races...

I don't have much knowledge of the scheduler right now and so I will not be able
to tell the exact places where we can clear the flag. So, dequeue was just one
of the places I could think of logically.

My idea was to make sure that we clear the RT and DL flags once we know that we
don't have any more RT/DL work to do. Dequeue or any other place that would suit
better, but we should be clearing it.

And once that is done, we wouldn't be required to have hacks like what this
patch is doing.

> > AFAIU, the problem we are discussing here
> > shouldn't normally occur while the sugov RT thread is running as the
> > work_in_progress flag makes sure we don't reevaluate the load while
> > the RT thread is updating the frequency.
> 
> True...
> 
> > The problem perhaps occurs as the flag for CPU X is never cleared,
> > and so on the next callback from the scheduler (after the frequency
> > is updated and work_in_progress is cleared) we move to the highest
> > frequency.
> 
> ... right.
> 
> > So what about clearing the flags, just like the previous patch, when
> > the RT or DL task has finished?
> 
> As a general goal I think it would be useful to feed input only when
> the scheduler knows something is going to happen. That's why in the
> last patch of these series I'm proposing to remove updates from
> update_curr_rt() and replace it with calls in most interesting events,
> like enqueue/pickup instead of dequeue.

Sure, I liked that. I am just wondering if we can have such a location to clear
the RT/DL flags.

> > Sorry for the noise if it was all nonsense :)
> 
> That's absolutely not nonsense, thanks for the feedback.
> 
> I agree with you that the solution proposed by this patch sound a bit
> of an hack, but still we can argue that using an RT task to change an
> OPP is by itself a sort-of an hack.

Just for the sake of argument, why do you think so ? :)

That work needs to get done via some sort of task, wq or RT thread, etc. We
chose RT to make sure we do it in time and never delay it. The ugliness happened
because we hit max frequency on RT task, which will be optimized later on.

-- 
viresh

[toc] | [prev] | [next] | [standalone]

#1593439 — Re: [PATCH 2/6] cpufreq: schedutil: ignore the sugov kthread for frequencies selections

On Thu,  2 Mar 2017 15:45:03 +0000
Patrick Bellasi <patrick.bellasi@arm.com> wrote:
 
> @@ -287,6 +289,10 @@ static void sugov_update_shared(struct update_util_data *hook, u64 time,
>  		goto done;
>  	}
>  
> +	/* Skip updates generated by sugov kthreads */
> +	if (curr == sg_policy->thread)

I think you want to put in an "unlikely()" around that statement. I'm
assuming you don't care about he performance of scheduling in the sugov
thread. At least tell gcc to optimize for the false path.

-- Steve

> +		goto done;
> +
>  	sg_cpu->util = util;
>  	sg_cpu->max = max;
>  	sg_cpu->flags = flags;

[toc] | [prev] | [next] | [standalone]

#1591229 — [PATCH 6/6] sched/rt: fast switch to maximum frequency when RT tasks are scheduled

Currently schedutil updates are triggered for the RT class using a single
call place, which is part of the rt::update_curr_rt() used in:

- dequeue_task_rt:
  but it does not make sense to set the schedutil's SCHED_CPUFREQ_RT in
  case the next task should not be an RT one

- put_prev_task_rt:
  likewise, we set the SCHED_CPUFREQ_RT flag without knowing if required
  by the next task

- pick_next_task_rt:
  likewise, the schedutil's SCHED_CPUFREQ_RT is set in case the prev task
  was RT, while we don't yet know if the next will be RT

- task_tick_rt:
  that's the only really useful call, which can ramp up the frequency in
  case a RT task started its execution without a chance to order a
  frequency switch (e.g. because of the schedutil ratelimit)

Apart from the last call in task_tick_rt, the others are at least useless.
Thus, although being a simple solution, not all the call sites of that
update_curr_rt() are interesting to trigger a frequency switch as well as
some of the most interesting points are not covered by that call.
For example, a task set to RT has to wait the next tick to get the frequency
boost.

This patch fixes these issue by placing explicitly the schedutils update
calls in the only sensible places, which are:
- when an RT task wakeups and it's enqueued in a CPU
- when we actually pick a RT task for execution
- at each tick time
- when a task is set to be RT

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: linux-kernel@vger.kernel.org
Cc: linux-pm@vger.kernel.org
---
 kernel/sched/rt.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
index 4101f9d..df7046c 100644
--- a/kernel/sched/rt.c
+++ b/kernel/sched/rt.c
@@ -958,9 +958,6 @@ static void update_curr_rt(struct rq *rq)
 	if (unlikely((s64)delta_exec <= 0))
 		return;
 
-	/* Kick cpufreq (see the comment in kernel/sched/sched.h). */
-	cpufreq_update_this_cpu(rq, SCHED_CPUFREQ_RT);
-
 	schedstat_set(curr->se.statistics.exec_max,
 		      max(curr->se.statistics.exec_max, delta_exec));
 
@@ -1326,6 +1323,9 @@ enqueue_task_rt(struct rq *rq, struct task_struct *p, int flags)
 
 	if (!task_current(rq, p) && tsk_nr_cpus_allowed(p) > 1)
 		enqueue_pushable_task(rq, p);
+
+	/* Kick cpufreq (see the comment in kernel/sched/sched.h). */
+	cpufreq_update_this_cpu(rq, SCHED_CPUFREQ_RT);
 }
 
 static void dequeue_task_rt(struct rq *rq, struct task_struct *p, int flags)
@@ -1563,6 +1563,9 @@ pick_next_task_rt(struct rq *rq, struct task_struct *prev, struct rq_flags *rf)
 
 	p = _pick_next_task_rt(rq);
 
+	/* Kick cpufreq (see the comment in kernel/sched/sched.h). */
+	cpufreq_update_this_cpu(rq, SCHED_CPUFREQ_RT);
+
 	/* The running task is never eligible for pushing */
 	dequeue_pushable_task(rq, p);
 
@@ -2272,6 +2275,9 @@ static void task_tick_rt(struct rq *rq, struct task_struct *p, int queued)
 {
 	struct sched_rt_entity *rt_se = &p->rt;
 
+	/* Kick cpufreq (see the comment in kernel/sched/sched.h). */
+	cpufreq_update_this_cpu(rq, SCHED_CPUFREQ_RT);
+
 	update_curr_rt(rq);
 
 	watchdog(rq, p);
@@ -2307,6 +2313,9 @@ static void set_curr_task_rt(struct rq *rq)
 
 	p->se.exec_start = rq_clock_task(rq);
 
+	/* Kick cpufreq (see the comment in kernel/sched/sched.h). */
+	cpufreq_update_this_cpu(rq, SCHED_CPUFREQ_RT);
+
 	/* The running task is never eligible for pushing */
 	dequeue_pushable_task(rq, p);
 }
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1591254

On 2 March 2017 at 16:45, Patrick Bellasi <patrick.bellasi@arm.com> wrote:
> The current version of schedutil has some issues related to the management
> of update flags used by systems with frequency domains spawning multiple CPUs.
>
> Each time a CPU utilisation update is issued by the scheduler a set of flags
> are configured to define (mainly) which class is asking for a utilisation
> update. These flags are then used by the frequency selection policy to
> identify the OPP to choose.
>
> In the current implementation, CPU flags are overridden each time the
> scheduler calls schedutil for an update. Such a behaviour produces issues
> in these scenarios, where we assume CPU1 and CPU2 share the same frequency
> domain:
> a) a RT task which executed on CPU1 can keep the domain at an high frequency
>    for a long period of time, even if there are no longer RT tasks on
>    CPUs in that domain

Normally this is dropped after a tick.
Nevertheless, there is an issue if the freq_update_delay_ns is shorter
than a tick because of sugov RT thread

> b) a FAIR task co-scheduled in the same CPU of a RT task can override the
>    flags configured by the RT task and potentially this can cause an
>    unwanted frequency drop
>
> These misbehaviours have been verified using a set of simple rt-app based
> synthetic workloads, running on a ARM's Juno board, and results are
> available in this Notebook [1].
>
> This series proposes a set of fixes for the aforementioned issues as well
> as a small improvement to speedup the selection of the maximum frequency
> when RT tasks enter a CPU.
>
> This series is based on top of today's tip/sched/core and is public available
> from this repository:
>
>   git://www.linux-arm.com/linux-pb eas/schedutil/flags_fixes
>
> Cheers Patrick
>
> [1] https://gist.github.com/d6a21b459a18091b2b058668a550010d
>
> Patrick Bellasi (6):
>   cpufreq: schedutil: reset sg_cpus's flags at IDLE enter
>   cpufreq: schedutil: ignore the sugov kthread for frequencies
>     selections
>   cpufreq: schedutil: ensure max frequency while running RT/DL tasks
>   cpufreq: schedutil: relax rate-limiting while running RT/DL tasks
>   cpufreq: schedutil: avoid utilisation update when not necessary
>   sched/rt: fast switch to maximum frequency when RT tasks are scheduled
>
>  include/linux/sched.h            |  1 +
>  kernel/sched/cpufreq_schedutil.c | 59 ++++++++++++++++++++++++++++++++++------
>  kernel/sched/idle_task.c         |  4 +++
>  kernel/sched/rt.c                | 15 ++++++++--
>  4 files changed, 68 insertions(+), 11 deletions(-)
>
> --
> 2.7.4
>

[toc] | [prev] | [next] | [standalone]

#1591334

On 02-Mar 17:09, Vincent Guittot wrote:
> On 2 March 2017 at 16:45, Patrick Bellasi <patrick.bellasi@arm.com> wrote:
> > The current version of schedutil has some issues related to the management
> > of update flags used by systems with frequency domains spawning multiple CPUs.
> >
> > Each time a CPU utilisation update is issued by the scheduler a set of flags
> > are configured to define (mainly) which class is asking for a utilisation
> > update. These flags are then used by the frequency selection policy to
> > identify the OPP to choose.
> >
> > In the current implementation, CPU flags are overridden each time the
> > scheduler calls schedutil for an update. Such a behaviour produces issues
> > in these scenarios, where we assume CPU1 and CPU2 share the same frequency
> > domain:
> > a) a RT task which executed on CPU1 can keep the domain at an high frequency
> >    for a long period of time, even if there are no longer RT tasks on
> >    CPUs in that domain
> 
> Normally this is dropped after a tick.
> Nevertheless, there is an issue if the freq_update_delay_ns is shorter
> than a tick because of sugov RT thread

Indeed, I've noticed that a small FAIR task running on CPU2 is quite
likely to be running at an higher OPP just because on the other CPU of
the same frequency domain (i.e. CPU1) sometimes we have the sugov RT
thread running.

In this case, the RT flag in CPU1 is never cleared when that core is
always idle but for the execution of the sugov thread.

-- 
#include <best/regards.h>

Patrick Bellasi

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web