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

Groups > linux.kernel > #1603976 > unrolled thread

[PATCH 0/2] cpufreq: schedutil: Fix and optimization

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/2] cpufreq: schedutil: Fix and optimization "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-03-19 14:50 +0100
    [PATCH 1/2] cpufreq: schedutil: Fix per-CPU structure initialization in sugov_start() "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-03-19 14:50 +0100
      Re: [PATCH 1/2] cpufreq: schedutil: Fix per-CPU structure  initialization in sugov_start() Viresh Kumar <viresh.kumar@linaro.org> - 2017-03-20 04:30 +0100
        Re: [PATCH 1/2] cpufreq: schedutil: Fix per-CPU structure initialization in sugov_start() "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-03-20 13:50 +0100
    [RFC][PATCH 2/2] cpufreq: schedutil: Force max frequency on busy CPUs "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-03-19 14:50 +0100
      Re: [RFC][PATCH 2/2] cpufreq: schedutil: Force max frequency on busy CPUs "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-03-19 22:40 +0100
        Re: [RFC][PATCH 2/2] cpufreq: schedutil: Force max frequency on busy CPUs "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-03-19 22:50 +0100
        Re: [RFC][PATCH 2/2] cpufreq: schedutil: Force max frequency on busy  CPUs Peter Zijlstra <peterz@infradead.org> - 2017-03-20 12:10 +0100
          Re: [RFC][PATCH 2/2] cpufreq: schedutil: Force max frequency on busy CPUs "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-03-20 13:50 +0100
      Re: [RFC][PATCH 2/2] cpufreq: schedutil: Force max frequency on busy  CPUs Viresh Kumar <viresh.kumar@linaro.org> - 2017-03-20 05:00 +0100
        Re: [RFC][PATCH 2/2] cpufreq: schedutil: Force max frequency on busy CPUs Vincent Guittot <vincent.guittot@linaro.org> - 2017-03-20 09:30 +0100
          Re: [RFC][PATCH 2/2] cpufreq: schedutil: Force max frequency on busy  CPUs Patrick Bellasi <patrick.bellasi@arm.com> - 2017-03-20 13:40 +0100
            Re: [RFC][PATCH 2/2] cpufreq: schedutil: Force max frequency on busy CPUs Joel Fernandes <joelaf@google.com> - 2017-03-23 01:00 +0100
              Re: [RFC][PATCH 2/2] cpufreq: schedutil: Force max frequency on busy CPUs Vincent Guittot <vincent.guittot@linaro.org> - 2017-03-23 23:10 +0100
                Re: [RFC][PATCH 2/2] cpufreq: schedutil: Force max frequency on busy CPUs Joel Fernandes <joelaf@google.com> - 2017-03-25 04:50 +0100
                  Re: [RFC][PATCH 2/2] cpufreq: schedutil: Force max frequency on busy CPUs Vincent Guittot <vincent.guittot@linaro.org> - 2017-03-27 09:10 +0200
          Re: [RFC][PATCH 2/2] cpufreq: schedutil: Force max frequency on busy CPUs "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-03-20 14:10 +0100
            Re: [RFC][PATCH 2/2] cpufreq: schedutil: Force max frequency on busy CPUs Vincent Guittot <vincent.guittot@linaro.org> - 2017-03-20 14:40 +0100
        Re: [RFC][PATCH 2/2] cpufreq: schedutil: Force max frequency on busy CPUs "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-03-20 14:30 +0100
      Re: [RFC][PATCH 2/2] cpufreq: schedutil: Force max frequency on busy  CPUs Peter Zijlstra <peterz@infradead.org> - 2017-03-20 12:50 +0100
        Re: [RFC][PATCH 2/2] cpufreq: schedutil: Force max frequency on busy CPUs "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-03-20 13:50 +0100
          Re: [RFC][PATCH 2/2] cpufreq: schedutil: Force max frequency on busy  CPUs Peter Zijlstra <peterz@infradead.org> - 2017-03-20 14:00 +0100
            Re: [RFC][PATCH 2/2] cpufreq: schedutil: Force max frequency on busy  CPUs Patrick Bellasi <patrick.bellasi@arm.com> - 2017-03-20 14:10 +0100
              Re: [RFC][PATCH 2/2] cpufreq: schedutil: Force max frequency on busy CPUs "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-03-20 14:40 +0100
                Re: [RFC][PATCH 2/2] cpufreq: schedutil: Force max frequency on busy  CPUs Patrick Bellasi <patrick.bellasi@arm.com> - 2017-03-20 15:20 +0100
            Re: [RFC][PATCH 2/2] cpufreq: schedutil: Force max frequency on busy CPUs "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-03-20 14:20 +0100
      [RFC][PATCH v2 2/2] cpufreq: schedutil: Avoid decreasing frequency of busy CPUs "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-03-20 23:00 +0100
        Re: [RFC][PATCH v2 2/2] cpufreq: schedutil: Avoid decreasing  frequency of busy CPUs Viresh Kumar <viresh.kumar@linaro.org> - 2017-03-21 07:50 +0100
          Re: [RFC][PATCH v2 2/2] cpufreq: schedutil: Avoid decreasing  frequency of busy CPUs "Rafael J. Wysocki" <rafael@kernel.org> - 2017-03-21 13:40 +0100
        Re: [RFC][PATCH v2 2/2] cpufreq: schedutil: Avoid decreasing  frequency of busy CPUs Vincent Guittot <vincent.guittot@linaro.org> - 2017-03-21 10:00 +0100
          Re: [RFC][PATCH v2 2/2] cpufreq: schedutil: Avoid decreasing  frequency of busy CPUs Patrick Bellasi <patrick.bellasi@arm.com> - 2017-03-21 13:00 +0100
          Re: [RFC][PATCH v2 2/2] cpufreq: schedutil: Avoid decreasing  frequency of busy CPUs Peter Zijlstra <peterz@infradead.org> - 2017-03-21 14:30 +0100
            Re: [RFC][PATCH v2 2/2] cpufreq: schedutil: Avoid decreasing  frequency of busy CPUs Vincent Guittot <vincent.guittot@linaro.org> - 2017-03-21 14:40 +0100
              Re: [RFC][PATCH v2 2/2] cpufreq: schedutil: Avoid decreasing  frequency of busy CPUs Vincent Guittot <vincent.guittot@linaro.org> - 2017-03-21 15:30 +0100
              Re: [RFC][PATCH v2 2/2] cpufreq: schedutil: Avoid decreasing frequency of busy CPUs "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-03-21 15:40 +0100
                Re: [RFC][PATCH v2 2/2] cpufreq: schedutil: Avoid decreasing  frequency of busy CPUs Patrick Bellasi <patrick.bellasi@arm.com> - 2017-03-21 15:50 +0100
                  Re: [RFC][PATCH v2 2/2] cpufreq: schedutil: Avoid decreasing frequency of busy CPUs "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-03-21 16:00 +0100
                    Re: [RFC][PATCH v2 2/2] cpufreq: schedutil: Avoid decreasing  frequency of busy CPUs Peter Zijlstra <peterz@infradead.org> - 2017-03-21 16:10 +0100
                      Re: [RFC][PATCH v2 2/2] cpufreq: schedutil: Avoid decreasing frequency of busy CPUs "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-03-21 16:30 +0100
                        Re: [RFC][PATCH v2 2/2] cpufreq: schedutil: Avoid decreasing  frequency of busy CPUs Peter Zijlstra <peterz@infradead.org> - 2017-03-21 18:10 +0100
                          Re: [RFC][PATCH v2 2/2] cpufreq: schedutil: Avoid decreasing frequency of busy CPUs "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-03-21 18:30 +0100
                    Re: [RFC][PATCH v2 2/2] cpufreq: schedutil: Avoid decreasing  frequency of busy CPUs Patrick Bellasi <patrick.bellasi@arm.com> - 2017-03-21 16:10 +0100
                      Re: [RFC][PATCH v2 2/2] cpufreq: schedutil: Avoid decreasing  frequency of busy CPUs Peter Zijlstra <peterz@infradead.org> - 2017-03-21 16:20 +0100
                        Re: [RFC][PATCH v2 2/2] cpufreq: schedutil: Avoid decreasing  frequency of busy CPUs Patrick Bellasi <patrick.bellasi@arm.com> - 2017-03-21 20:30 +0100
                    Re: [RFC][PATCH v2 2/2] cpufreq: schedutil: Avoid decreasing frequency of busy CPUs "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-03-21 16:10 +0100
                Re: [RFC][PATCH v2 2/2] cpufreq: schedutil: Avoid decreasing  frequency of busy CPUs Peter Zijlstra <peterz@infradead.org> - 2017-03-21 16:10 +0100
              Re: [RFC][PATCH v2 2/2] cpufreq: schedutil: Avoid decreasing  frequency of busy CPUs Patrick Bellasi <patrick.bellasi@arm.com> - 2017-03-21 15:40 +0100
              Re: [RFC][PATCH v2 2/2] cpufreq: schedutil: Avoid decreasing  frequency of busy CPUs Peter Zijlstra <peterz@infradead.org> - 2017-03-21 16:00 +0100
                Re: [RFC][PATCH v2 2/2] cpufreq: schedutil: Avoid decreasing  frequency of busy CPUs Peter Zijlstra <peterz@infradead.org> - 2017-03-21 16:00 +0100
                  Re: [RFC][PATCH v2 2/2] cpufreq: schedutil: Avoid decreasing  frequency of busy CPUs Vincent Guittot <vincent.guittot@linaro.org> - 2017-03-21 18:10 +0100
                  Re: [RFC][PATCH v2 2/2] cpufreq: schedutil: Avoid decreasing  frequency of busy CPUs Vincent Guittot <vincent.guittot@linaro.org> - 2017-03-21 18:10 +0100
        Re: [RFC][PATCH v2 2/2] cpufreq: schedutil: Avoid decreasing  frequency of busy CPUs Patrick Bellasi <patrick.bellasi@arm.com> - 2017-03-21 13:00 +0100
        [RFC][PATCH v3 2/2] cpufreq: schedutil: Avoid reducing frequency of busy CPUs prematurely "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-03-22 00:20 +0100
          Re: [RFC][PATCH v3 2/2] cpufreq: schedutil: Avoid reducing frequency  of busy CPUs prematurely Peter Zijlstra <peterz@infradead.org> - 2017-03-22 10:30 +0100
          Re: [RFC][PATCH v3 2/2] cpufreq: schedutil: Avoid reducing frequency  of busy CPUs prematurely Viresh Kumar <viresh.kumar@linaro.org> - 2017-03-22 11:10 +0100
          Re: [RFC][PATCH v3 2/2] cpufreq: schedutil: Avoid reducing frequency  of busy CPUs prematurely Joel Fernandes <joelaf@google.com> - 2017-03-23 02:10 +0100
          Re: [RFC][PATCH v3 2/2] cpufreq: schedutil: Avoid reducing frequency  of busy CPUs prematurely Sai Gurrappadi <sgurrappadi@nvidia.com> - 2017-03-23 20:30 +0100
            Re: [RFC][PATCH v3 2/2] cpufreq: schedutil: Avoid reducing frequency  of busy CPUs prematurely Sai Gurrappadi <sgurrappadi@nvidia.com> - 2017-03-23 22:00 +0100
            Re: [RFC][PATCH v3 2/2] cpufreq: schedutil: Avoid reducing frequency  of busy CPUs prematurely "Rafael J. Wysocki" <rafael@kernel.org> - 2017-03-24 02:40 +0100
              Re: [RFC][PATCH v3 2/2] cpufreq: schedutil: Avoid reducing frequency  of busy CPUs prematurely Sai Gurrappadi <sgurrappadi@nvidia.com> - 2017-03-24 20:20 +0100
          Re: [RFC][PATCH v3 2/2] cpufreq: schedutil: Avoid reducing frequency  of busy CPUs prematurely Sai Gurrappadi <sgurrappadi@nvidia.com> - 2017-03-25 02:20 +0100
            Re: [RFC][PATCH v3 2/2] cpufreq: schedutil: Avoid reducing frequency  of busy CPUs prematurely "Rafael J. Wysocki" <rafael@kernel.org> - 2017-03-25 02:40 +0100
            Re: [RFC][PATCH v3 2/2] cpufreq: schedutil: Avoid reducing frequency  of busy CPUs prematurely Vincent Guittot <vincent.guittot@linaro.org> - 2017-03-27 09:20 +0200

Page 3 of 4 — ← Prev page 1 2 [3] 4  Next page →

#1605845 — Re: [RFC][PATCH v2 2/2] cpufreq: schedutil: Avoid decreasing frequency of busy CPUs

On Tuesday, March 21, 2017 06:00:17 PM Peter Zijlstra wrote:
> On Tue, Mar 21, 2017 at 04:18:52PM +0100, Rafael J. Wysocki wrote:
> > +static bool sugov_cpu_is_busy(struct sugov_cpu *sg_cpu)
> > +{
> > +	unsigned long idle_calls = tick_nohz_get_idle_calls();
> > +	bool not_idle = idle_calls == sg_cpu->saved_idle_calls;
> > +
> > +	sg_cpu->saved_idle_calls = idle_calls;
> > +	return not_idle && this_rq()->rd->overload;
> > +}
> 
> So I really don't understand the rd->overload thing. What is it supposed
> to do here?

The idea was that if the CPU was running one task saturating the capacity which
then was migrated out of it, the frequency should still be reduced.

And since rd->overload covers all CPUs (in general) it kind of tells us whether
or not there are other tasks to replace the migrated one any time soon.

However, if there are no tasks to replace the migrated one, the CPU will go
idle quickly (as there are no taks to run on it), in which case keeping the
current frequency on it shouldn't matter.

In all of the other cases keeping the current frequency is the right thing to
do IMO.

So, it looks like checking this_rq()->rd->overload doesn't really help after all. :-)

Thanks,
Rafael

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

#1605694 — Re: [RFC][PATCH v2 2/2] cpufreq: schedutil: Avoid decreasing frequency of busy CPUs

On 21-Mar 15:46, Rafael J. Wysocki wrote:
> On Tuesday, March 21, 2017 02:38:42 PM Patrick Bellasi wrote:
> > On 21-Mar 15:26, Rafael J. Wysocki wrote:
> > > On Tuesday, March 21, 2017 02:37:08 PM Vincent Guittot wrote:
> > > > On 21 March 2017 at 14:22, Peter Zijlstra <peterz@infradead.org> wrote:
> > > > > On Tue, Mar 21, 2017 at 09:50:28AM +0100, Vincent Guittot wrote:
> > > > >> On 20 March 2017 at 22:46, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > > > >
> > > > >> > To work around this issue use the observation that, from the
> > > > >> > schedutil governor's perspective, it does not make sense to decrease
> > > > >> > the frequency of a CPU that doesn't enter idle and avoid decreasing
> > > > >> > the frequency of busy CPUs.
> > > > >>
> > > > >> I don't fully agree with that statement.
> > > > >> If there are 2 runnable tasks on CPU A and scheduler migrates the
> > > > >> waiting task to another CPU B so CPU A is less loaded now, it makes
> > > > >> sense to reduce the OPP. That's even for that purpose that we have
> > > > >> decided to use scheduler metrics in cpufreq governor so we can adjust
> > > > >> OPP immediately when tasks migrate.
> > > > >> That being said, i probably know why you see such OPP switches in your
> > > > >> use case. When we migrate a task, we also migrate/remove its
> > > > >> utilization from CPU.
> > > > >> If the CPU is not overloaded, it means that runnable tasks have all
> > > > >> computation that they need and don't have any reason to use more when
> > > > >> a task migrates to another CPU. so decreasing the OPP makes sense
> > > > >> because the utilzation is decreasing
> > > > >> If the CPU is overloaded, it means that runnable tasks have to share
> > > > >> CPU time and probably don't have all computations that they would like
> > > > >> so when a task migrate, the remaining tasks on the CPU will increase
> > > > >> their utilization and fill space left by the task that has just
> > > > >> migrated. So the CPU's utilization will decrease when a task migrates
> > > > >> (and as a result the OPP) but then its utilization will increase with
> > > > >> remaining tasks running more time as well as the OPP
> > > > >>
> > > > >> So you need to make the difference between this 2 cases: Is a CPU
> > > > >> overloaded or not. You can't really rely on the utilization to detect
> > > > >> that but you could take advantage of the load which take into account
> > > > >> the waiting time of tasks
> > > > >
> > > > > I'm confused. What two cases? You only list the overloaded case, but he
> > > > 
> > > > overloaded vs not overloaded use case.
> > > > For the not overloaded case, it makes sense to immediately update to
> > > > OPP to be aligned with the new utilization of the CPU even if it was
> > > > not idle in the past couple of ticks
> > > 
> > > Yes, if the OPP (or P-state if you will) can be changed immediately.  If it can't,
> > > conditions may change by the time we actually update it and in that case It'd
> > > be better to wait and see IMO.
> > > 
> > > In any case, the theory about migrating tasks made sense to me, so below is
> > > what I tested.  It works, and besides it has a nice feature that I don't need
> > > to fetch for the timekeeping data. :-)
> > > 
> > > I only wonder if we want to do this or only prevent the frequency from
> > > decreasing in the overloaded case?
> > > 
> > > ---
> > >  kernel/sched/cpufreq_schedutil.c |    8 +++++---
> > >  1 file changed, 5 insertions(+), 3 deletions(-)
> > > 
> > > Index: linux-pm/kernel/sched/cpufreq_schedutil.c
> > > ===================================================================
> > > --- linux-pm.orig/kernel/sched/cpufreq_schedutil.c
> > > +++ linux-pm/kernel/sched/cpufreq_schedutil.c
> > > @@ -61,6 +61,7 @@ struct sugov_cpu {
> > >  	unsigned long util;
> > >  	unsigned long max;
> > >  	unsigned int flags;
> > > +	bool overload;
> > >  };
> > >  
> > >  static DEFINE_PER_CPU(struct sugov_cpu, sugov_cpu);
> > > @@ -207,7 +208,7 @@ static void sugov_update_single(struct u
> > >  	if (!sugov_should_update_freq(sg_policy, time))
> > >  		return;
> > >  
> > > -	if (flags & SCHED_CPUFREQ_RT_DL) {
> > > +	if ((flags & SCHED_CPUFREQ_RT_DL) || this_rq()->rd->overload) {
> > >  		next_f = policy->cpuinfo.max_freq;
> > 
> > Isn't this going to max OPP every time we have more than 1 task in
> > that CPU?
> > 
> > In that case it will not fit the case: we have two 10% tasks on that CPU.
> 
> Good point.
> 
> > Previous solution was better IMO, apart from using overloaded instead
> > of overutilized (which is not yet there) :-/
> 
> OK, so the one below works too.

Better... just one minor comment.


> ---
>  kernel/sched/cpufreq_schedutil.c |   11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> Index: linux-pm/kernel/sched/cpufreq_schedutil.c
> ===================================================================
> --- linux-pm.orig/kernel/sched/cpufreq_schedutil.c
> +++ linux-pm/kernel/sched/cpufreq_schedutil.c
> @@ -37,6 +37,7 @@ struct sugov_policy {
>  	s64 freq_update_delay_ns;
>  	unsigned int next_freq;
>  	unsigned int cached_raw_freq;
> +	bool overload;

Can we avoid using "overloaded" in favor of a more generic and
schedutil specific name. Mainly because in the future we would
probably like to switch from "overloaded" to "overutilized".

What about something like: "busy" ?

>  
>  	/* The next fields are only needed if fast switch cannot be used. */
>  	struct irq_work irq_work;
> @@ -61,6 +62,7 @@ struct sugov_cpu {
>  	unsigned long util;
>  	unsigned long max;
>  	unsigned int flags;
> +	bool overload;
>  };
>  
>  static DEFINE_PER_CPU(struct sugov_cpu, sugov_cpu);
> @@ -93,6 +95,9 @@ static void sugov_update_commit(struct s
>  {
>  	struct cpufreq_policy *policy = sg_policy->policy;
>  
> +	if (sg_policy->overload && next_freq < sg_policy->next_freq)
> +		next_freq = sg_policy->next_freq;
> +
>  	if (policy->fast_switch_enabled) {
>  		if (sg_policy->next_freq == next_freq) {
>  			trace_cpu_frequency(policy->cur, smp_processor_id());
> @@ -207,6 +212,8 @@ static void sugov_update_single(struct u
>  	if (!sugov_should_update_freq(sg_policy, time))
>  		return;
>  
> +	sg_policy->overload = this_rq()->rd->overload;

And than we can move this bit into an inline function, something like e.g.:

   static inline bool sugov_this_cpu_is_busy()
   {
           return this_rq()->rd->overloaded
   }

Where in future we can easily switch from usage of "overloaded" to
usage of "utilization".

> +
>  	if (flags & SCHED_CPUFREQ_RT_DL) {
>  		next_f = policy->cpuinfo.max_freq;
>  	} else {
> @@ -225,6 +232,8 @@ static unsigned int sugov_next_freq_shar
>  	unsigned long util = 0, max = 1;
>  	unsigned int j;
>  
> +	sg_policy->overload = false;
> +
>  	for_each_cpu(j, policy->cpus) {
>  		struct sugov_cpu *j_sg_cpu = &per_cpu(sugov_cpu, j);
>  		unsigned long j_util, j_max;
> @@ -253,6 +262,7 @@ static unsigned int sugov_next_freq_shar
>  		}
>  
>  		sugov_iowait_boost(j_sg_cpu, &util, &max);
> +		sg_policy->overload = sg_policy->overload || sg_cpu->overload;
>  	}
>  
>  	return get_next_freq(sg_policy, util, max);
> @@ -273,6 +283,7 @@ static void sugov_update_shared(struct u
>  	sg_cpu->util = util;
>  	sg_cpu->max = max;
>  	sg_cpu->flags = flags;
> +	sg_cpu->overload = this_rq()->rd->overload;
>  
>  	sugov_set_iowait_boost(sg_cpu, time, flags);
>  	sg_cpu->last_update = time;
> 

-- 
#include <best/regards.h>

Patrick Bellasi

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

#1605702 — Re: [RFC][PATCH v2 2/2] cpufreq: schedutil: Avoid decreasing frequency of busy CPUs

Seriously people, trim your replies.

On Tue, Mar 21, 2017 at 03:08:20PM +0000, Patrick Bellasi wrote:

> And than we can move this bit into an inline function, something like e.g.:
> 
>    static inline bool sugov_this_cpu_is_busy()
>    {
>            return this_rq()->rd->overloaded
>    }

No, that's just entirely and utterly wrong. It being in rd means its
very much not about _this_ CPU in any way.

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

#1605937 — Re: [RFC][PATCH v2 2/2] cpufreq: schedutil: Avoid decreasing frequency of busy CPUs

On 21-Mar 16:18, Peter Zijlstra wrote:
> On Tue, Mar 21, 2017 at 03:08:20PM +0000, Patrick Bellasi wrote:
> 
> > And than we can move this bit into an inline function, something like e.g.:
> > 
> >    static inline bool sugov_this_cpu_is_busy()
> >    {
> >            return this_rq()->rd->overloaded
> >    }
> 
> No, that's just entirely and utterly wrong. It being in rd means its
> very much not about _this_ CPU in any way.

You right (of course), we cannot really use "this_" in the name of
a function with such a code.

The suggestion here was at least to factor out whatever code we want
to use to check if the current CPU has to be subject to a down-scaling
constraint.

However, using rd->overload is not the best option, for the many reasons
you explained in your previous comment. Thus, we should probably stay
with the idle time tracking solution initially proposed by Rafael.

Sorry for the noise :-(

-- 
#include <best/regards.h>

Patrick Bellasi

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

#1605699 — Re: [RFC][PATCH v2 2/2] cpufreq: schedutil: Avoid decreasing frequency of busy CPUs

On Tuesday, March 21, 2017 03:46:07 PM Rafael J. Wysocki wrote:
> On Tuesday, March 21, 2017 02:38:42 PM Patrick Bellasi wrote:
> > On 21-Mar 15:26, Rafael J. Wysocki wrote:
> > > On Tuesday, March 21, 2017 02:37:08 PM Vincent Guittot wrote:
> > > > On 21 March 2017 at 14:22, Peter Zijlstra <peterz@infradead.org> wrote:
> > > > > On Tue, Mar 21, 2017 at 09:50:28AM +0100, Vincent Guittot wrote:
> > > > >> On 20 March 2017 at 22:46, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > > > >
> > > > >> > To work around this issue use the observation that, from the
> > > > >> > schedutil governor's perspective, it does not make sense to decrease
> > > > >> > the frequency of a CPU that doesn't enter idle and avoid decreasing
> > > > >> > the frequency of busy CPUs.
> > > > >>
> > > > >> I don't fully agree with that statement.
> > > > >> If there are 2 runnable tasks on CPU A and scheduler migrates the
> > > > >> waiting task to another CPU B so CPU A is less loaded now, it makes
> > > > >> sense to reduce the OPP. That's even for that purpose that we have
> > > > >> decided to use scheduler metrics in cpufreq governor so we can adjust
> > > > >> OPP immediately when tasks migrate.
> > > > >> That being said, i probably know why you see such OPP switches in your
> > > > >> use case. When we migrate a task, we also migrate/remove its
> > > > >> utilization from CPU.
> > > > >> If the CPU is not overloaded, it means that runnable tasks have all
> > > > >> computation that they need and don't have any reason to use more when
> > > > >> a task migrates to another CPU. so decreasing the OPP makes sense
> > > > >> because the utilzation is decreasing
> > > > >> If the CPU is overloaded, it means that runnable tasks have to share
> > > > >> CPU time and probably don't have all computations that they would like
> > > > >> so when a task migrate, the remaining tasks on the CPU will increase
> > > > >> their utilization and fill space left by the task that has just
> > > > >> migrated. So the CPU's utilization will decrease when a task migrates
> > > > >> (and as a result the OPP) but then its utilization will increase with
> > > > >> remaining tasks running more time as well as the OPP
> > > > >>
> > > > >> So you need to make the difference between this 2 cases: Is a CPU
> > > > >> overloaded or not. You can't really rely on the utilization to detect
> > > > >> that but you could take advantage of the load which take into account
> > > > >> the waiting time of tasks
> > > > >
> > > > > I'm confused. What two cases? You only list the overloaded case, but he
> > > > 
> > > > overloaded vs not overloaded use case.
> > > > For the not overloaded case, it makes sense to immediately update to
> > > > OPP to be aligned with the new utilization of the CPU even if it was
> > > > not idle in the past couple of ticks
> > > 
> > > Yes, if the OPP (or P-state if you will) can be changed immediately.  If it can't,
> > > conditions may change by the time we actually update it and in that case It'd
> > > be better to wait and see IMO.
> > > 
> > > In any case, the theory about migrating tasks made sense to me, so below is
> > > what I tested.  It works, and besides it has a nice feature that I don't need
> > > to fetch for the timekeeping data. :-)
> > > 
> > > I only wonder if we want to do this or only prevent the frequency from
> > > decreasing in the overloaded case?
> > > 
> > > ---
> > >  kernel/sched/cpufreq_schedutil.c |    8 +++++---
> > >  1 file changed, 5 insertions(+), 3 deletions(-)
> > > 
> > > Index: linux-pm/kernel/sched/cpufreq_schedutil.c
> > > ===================================================================
> > > --- linux-pm.orig/kernel/sched/cpufreq_schedutil.c
> > > +++ linux-pm/kernel/sched/cpufreq_schedutil.c
> > > @@ -61,6 +61,7 @@ struct sugov_cpu {
> > >  	unsigned long util;
> > >  	unsigned long max;
> > >  	unsigned int flags;
> > > +	bool overload;
> > >  };
> > >  
> > >  static DEFINE_PER_CPU(struct sugov_cpu, sugov_cpu);
> > > @@ -207,7 +208,7 @@ static void sugov_update_single(struct u
> > >  	if (!sugov_should_update_freq(sg_policy, time))
> > >  		return;
> > >  
> > > -	if (flags & SCHED_CPUFREQ_RT_DL) {
> > > +	if ((flags & SCHED_CPUFREQ_RT_DL) || this_rq()->rd->overload) {
> > >  		next_f = policy->cpuinfo.max_freq;
> > 
> > Isn't this going to max OPP every time we have more than 1 task in
> > that CPU?
> > 
> > In that case it will not fit the case: we have two 10% tasks on that CPU.
> 
> Good point.
> 
> > Previous solution was better IMO, apart from using overloaded instead
> > of overutilized (which is not yet there) :-/
> 
> OK, so the one below works too.

Admittedly, we could check the idle condition and the overload flag at the same
time, though.

Let me try that too.

Thanks,
Rafael

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

#1605701 — Re: [RFC][PATCH v2 2/2] cpufreq: schedutil: Avoid decreasing frequency of busy CPUs

On Tue, Mar 21, 2017 at 03:26:06PM +0100, Rafael J. Wysocki wrote:
> +	if ((flags & SCHED_CPUFREQ_RT_DL) || this_rq()->rd->overload) {
>  		next_f = policy->cpuinfo.max_freq;

So this I think is wrong; rd->overload is set if _any_ of the CPUs in the
root domain is overloaded. And given the root domain is typically the
_entire_ machine, this would have a tendency to run at max_freq far too
often.

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

#1605657 — Re: [RFC][PATCH v2 2/2] cpufreq: schedutil: Avoid decreasing frequency of busy CPUs

On 21-Mar 15:03, Peter Zijlstra wrote:
> On Tue, Mar 21, 2017 at 02:37:08PM +0100, Vincent Guittot wrote:
> > On 21 March 2017 at 14:22, Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > For the not overloaded case, it makes sense to immediately update to
> > OPP to be aligned with the new utilization of the CPU even if it was
> > not idle in the past couple of ticks
> 
> Yeah, but we cannot know. Also, who cares?
> 
> > > does exactly that. Note that the lack of idle time is an exact
> > > equivalent of 100% utilized.
> > >
> > > So even while we cannot currently detect the 100% utilized state through
> > > the running state tracking; because averages etc.. we can detect the
> > > lack of idle time.
> > 
> > But after how much lack of idle time do we consider that we are overloaded ?
> 
> 0 :-)

If we should use "utilization" this time can be non 0 and it depends
for example on how long PELT takes to build up a utilization value
which marks the CPU as "overutilized"... thus we already have a
suitable time at least for CFS tasks.

> Note that utilization is an absolute metric, not a windowed one. That
> is, there is no actual time associated with it. Now, for practical
> purposes we end up using windowed things in many places,
> 

-- 
#include <best/regards.h>

Patrick Bellasi

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

#1605675 — Re: [RFC][PATCH v2 2/2] cpufreq: schedutil: Avoid decreasing frequency of busy CPUs

On Tue, Mar 21, 2017 at 02:37:08PM +0100, Vincent Guittot wrote:
> On 21 March 2017 at 14:22, Peter Zijlstra <peterz@infradead.org> wrote:

> For the not overloaded case, it makes sense to immediately update to
> OPP to be aligned with the new utilization of the CPU even if it was
> not idle in the past couple of ticks

Yeah, but we cannot know. Also, who cares?

> > does exactly that. Note that the lack of idle time is an exact
> > equivalent of 100% utilized.
> >
> > So even while we cannot currently detect the 100% utilized state through
> > the running state tracking; because averages etc.. we can detect the
> > lack of idle time.
> 
> But after how much lack of idle time do we consider that we are overloaded ?

0 :-)

Note that utilization is an absolute metric, not a windowed one. That
is, there is no actual time associated with it. Now, for practical
purposes we end up using windowed things in many places,

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

#1605680 — Re: [RFC][PATCH v2 2/2] cpufreq: schedutil: Avoid decreasing frequency of busy CPUs

On Tue, Mar 21, 2017 at 03:16:19PM +0100, Vincent Guittot wrote:
> On 21 March 2017 at 15:03, Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > On Tue, Mar 21, 2017 at 02:37:08PM +0100, Vincent Guittot wrote:
> > > On 21 March 2017 at 14:22, Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > > For the not overloaded case, it makes sense to immediately update to
> > > OPP to be aligned with the new utilization of the CPU even if it was
> > > not idle in the past couple of ticks
> >
> > Yeah, but we cannot know. Also, who cares?
> >
> 
> embedded system that doesn't want to stay at higest OPP if significant part
> of the utilzation has moved away as an example
> AFAICT, schedutil tries to select the best OPP according to the current
> utilization of the CPU so if the utilization decreases, the OPP should also
> decrease

Sure I get that; but given the lack of crystal ball instructions we
cannot know if this is the case or not.

And if we really dropped below 100% utilization, we should hit idle
fairly soon.

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

#1605825 — Re: [RFC][PATCH v2 2/2] cpufreq: schedutil: Avoid decreasing frequency of busy CPUs

On 21 March 2017 at 18:00, Vincent Guittot <vincent.guittot@linaro.org> wrote:
> On 21 March 2017 at 15:58, Peter Zijlstra <peterz@infradead.org> wrote:
>>
>> On Tue, Mar 21, 2017 at 03:16:19PM +0100, Vincent Guittot wrote:
>> > On 21 March 2017 at 15:03, Peter Zijlstra <peterz@infradead.org> wrote:
>> >
>> > > On Tue, Mar 21, 2017 at 02:37:08PM +0100, Vincent Guittot wrote:
>> > > > On 21 March 2017 at 14:22, Peter Zijlstra <peterz@infradead.org> wrote:
>> > >
>> > > > For the not overloaded case, it makes sense to immediately update to
>> > > > OPP to be aligned with the new utilization of the CPU even if it was
>> > > > not idle in the past couple of ticks
>> > >
>> > > Yeah, but we cannot know. Also, who cares?
>> > >
>> >
>> > embedded system that doesn't want to stay at higest OPP if significant part
>> > of the utilzation has moved away as an example
>> > AFAICT, schedutil tries to select the best OPP according to the current
>> > utilization of the CPU so if the utilization decreases, the OPP should also
>> > decrease
>>
>> Sure I get that; but given the lack of crystal ball instructions we
>> cannot know if this is the case or not.
>
> cfs_rq->avg.load_avg account the waiting time of CPU (in addition to

sorry i wanted to say the waiting time of tasks on the CPU

> the weight of task) so i was wondering if we can't use it to detect if
> we are in the overloaded  case or not even if utilization is not mac
> capacity because we have just migrated a task (and its utilization)
> out
>
>
>
>>
>> And if we really dropped below 100% utilization, we should hit idle
>> fairly soon.

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

#1605826 — Re: [RFC][PATCH v2 2/2] cpufreq: schedutil: Avoid decreasing frequency of busy CPUs

On 21 March 2017 at 15:58, Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Tue, Mar 21, 2017 at 03:16:19PM +0100, Vincent Guittot wrote:
> > On 21 March 2017 at 15:03, Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > > On Tue, Mar 21, 2017 at 02:37:08PM +0100, Vincent Guittot wrote:
> > > > On 21 March 2017 at 14:22, Peter Zijlstra <peterz@infradead.org> wrote:
> > >
> > > > For the not overloaded case, it makes sense to immediately update to
> > > > OPP to be aligned with the new utilization of the CPU even if it was
> > > > not idle in the past couple of ticks
> > >
> > > Yeah, but we cannot know. Also, who cares?
> > >
> >
> > embedded system that doesn't want to stay at higest OPP if significant part
> > of the utilzation has moved away as an example
> > AFAICT, schedutil tries to select the best OPP according to the current
> > utilization of the CPU so if the utilization decreases, the OPP should also
> > decrease
>
> Sure I get that; but given the lack of crystal ball instructions we
> cannot know if this is the case or not.

cfs_rq->avg.load_avg account the waiting time of CPU (in addition to
the weight of task) so i was wondering if we can't use it to detect if
we are in the overloaded  case or not even if utilization is not mac
capacity because we have just migrated a task (and its utilization)
out



>
> And if we really dropped below 100% utilization, we should hit idle
> fairly soon.

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

#1605547 — Re: [RFC][PATCH v2 2/2] cpufreq: schedutil: Avoid decreasing frequency of busy CPUs

On 20-Mar 22:46, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> The way the schedutil governor uses the PELT metric causes it to
> underestimate the CPU utilization in some cases.
> 
> That can be easily demonstrated by running kernel compilation on
> a Sandy Bridge Intel processor, running turbostat in parallel with
> it and looking at the values written to the MSR_IA32_PERF_CTL
> register.  Namely, the expected result would be that when all CPUs
> were 100% busy, all of them would be requested to run in the maximum
> P-state, but observation shows that this clearly isn't the case.
> The CPUs run in the maximum P-state for a while and then are
> requested to run slower and go back to the maximum P-state after
> a while again.  That causes the actual frequency of the processor to
> visibly oscillate below the sustainable maximum in a jittery fashion
> which clearly is not desirable.
> 
> To work around this issue use the observation that, from the
> schedutil governor's perspective, it does not make sense to decrease
> the frequency of a CPU that doesn't enter idle and avoid decreasing
> the frequency of busy CPUs.
> 
> To that end, use the counter of idle calls in the timekeeping code.
> Namely, make the schedutil governor look at that counter for the
> current CPU every time before it is about to set a new frequency
> for that CPU's policy.  If the counter has not changed since the
> previous iteration, the CPU has been busy for all that time and
> its frequency should not be decreased, so if the new frequency would
> be lower than the one set previously, the governor will skip the
> frequency update.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> 
> This is a slightly different approach (avoid decreasing frequency for busy CPUs
> instead of bumping if for them to the max upfront) and it works around the
> original problem too.

I like much better this version where we do not enforce max frequency
as well as we removed the hardcoded time threshold. ;-)

Makes sense to me also to avoid down scaling until we don't hit an IDLE.

However, I also agree with Vincent's observation: this constraint should be
there only for "overutilized" CPUs... but unfortunately, in mainline we are
still missing that flag and thus we should probably use the
"overloaded" one for the time being.

> I tried to address a few Peter's comments here and the result doesn't seem to
> be too heavy-wieght.

Nice!

> Thanks,
> Rafael
> 
> ---
>  include/linux/tick.h             |    1 +
>  kernel/sched/cpufreq_schedutil.c |   28 ++++++++++++++++++++++++----
>  kernel/time/tick-sched.c         |   12 ++++++++++++
>  3 files changed, 37 insertions(+), 4 deletions(-)
> 
> Index: linux-pm/kernel/sched/cpufreq_schedutil.c
> ===================================================================
> --- linux-pm.orig/kernel/sched/cpufreq_schedutil.c
> +++ linux-pm/kernel/sched/cpufreq_schedutil.c
> @@ -56,6 +56,9 @@ struct sugov_cpu {
>  	unsigned long iowait_boost;
>  	unsigned long iowait_boost_max;
>  	u64 last_update;
> +#ifdef CONFIG_NO_HZ_COMMON
> +	unsigned long saved_idle_calls;
> +#endif
>  
>  	/* The fields below are only needed when sharing a policy. */
>  	unsigned long util;
> @@ -88,11 +91,28 @@ static bool sugov_should_update_freq(str
>  	return delta_ns >= sg_policy->freq_update_delay_ns;
>  }
>  
> -static void sugov_update_commit(struct sugov_policy *sg_policy, u64 time,
> -				unsigned int next_freq)
> +#ifdef CONFIG_NO_HZ_COMMON
> +static bool sugov_cpu_is_busy(struct sugov_cpu *sg_cpu)
> +{
> +	unsigned long idle_calls = tick_nohz_get_idle_calls();
> +	bool ret = idle_calls == sg_cpu->saved_idle_calls;
> +
> +	sg_cpu->saved_idle_calls = idle_calls;
> +	return ret;
> +}
> +#else
> +static inline bool sugov_cpu_is_busy(struct sugov_cpu *sg_cpu) { return false; }
> +#endif /* CONFIG_NO_HZ_COMMON */
> +
> +static void sugov_update_commit(struct sugov_cpu *sg_cpu,
> +				struct sugov_policy *sg_policy,
> +				u64 time, unsigned int next_freq)
>  {
>  	struct cpufreq_policy *policy = sg_policy->policy;
>  
> +	if (sugov_cpu_is_busy(sg_cpu) && next_freq < sg_policy->next_freq)
> +		next_freq = sg_policy->next_freq;
> +
>  	if (policy->fast_switch_enabled) {
>  		if (sg_policy->next_freq == next_freq) {
>  			trace_cpu_frequency(policy->cur, smp_processor_id());
> @@ -214,7 +234,7 @@ static void sugov_update_single(struct u
>  		sugov_iowait_boost(sg_cpu, &util, &max);
>  		next_f = get_next_freq(sg_policy, util, max);
>  	}
> -	sugov_update_commit(sg_policy, time, next_f);
> +	sugov_update_commit(sg_cpu, sg_policy, time, next_f);
>  }
>  
>  static unsigned int sugov_next_freq_shared(struct sugov_cpu *sg_cpu)
> @@ -283,7 +303,7 @@ static void sugov_update_shared(struct u
>  		else
>  			next_f = sugov_next_freq_shared(sg_cpu);
>  
> -		sugov_update_commit(sg_policy, time, next_f);
> +		sugov_update_commit(sg_cpu, sg_policy, time, next_f);
>  	}
>  
>  	raw_spin_unlock(&sg_policy->update_lock);
> Index: linux-pm/include/linux/tick.h
> ===================================================================
> --- linux-pm.orig/include/linux/tick.h
> +++ linux-pm/include/linux/tick.h
> @@ -117,6 +117,7 @@ extern void tick_nohz_idle_enter(void);
>  extern void tick_nohz_idle_exit(void);
>  extern void tick_nohz_irq_exit(void);
>  extern ktime_t tick_nohz_get_sleep_length(void);
> +extern unsigned long tick_nohz_get_idle_calls(void);
>  extern u64 get_cpu_idle_time_us(int cpu, u64 *last_update_time);
>  extern u64 get_cpu_iowait_time_us(int cpu, u64 *last_update_time);
>  #else /* !CONFIG_NO_HZ_COMMON */
> Index: linux-pm/kernel/time/tick-sched.c
> ===================================================================
> --- linux-pm.orig/kernel/time/tick-sched.c
> +++ linux-pm/kernel/time/tick-sched.c
> @@ -993,6 +993,18 @@ ktime_t tick_nohz_get_sleep_length(void)
>  	return ts->sleep_length;
>  }
>  
> +/**
> + * tick_nohz_get_idle_calls - return the current idle calls counter value
> + *
> + * Called from the schedutil frequency scaling governor in scheduler context.
> + */
> +unsigned long tick_nohz_get_idle_calls(void)
> +{
> +	struct tick_sched *ts = this_cpu_ptr(&tick_cpu_sched);
> +
> +	return ts->idle_calls;
> +}
> +
>  static void tick_nohz_account_idle_ticks(struct tick_sched *ts)
>  {
>  #ifndef CONFIG_VIRT_CPU_ACCOUNTING_NATIVE
> 

-- 
#include <best/regards.h>

Patrick Bellasi

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

#1606069 — [RFC][PATCH v3 2/2] cpufreq: schedutil: Avoid reducing frequency of busy CPUs prematurely

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

The way the schedutil governor uses the PELT metric causes it to
underestimate the CPU utilization in some cases.

That can be easily demonstrated by running kernel compilation on
a Sandy Bridge Intel processor, running turbostat in parallel with
it and looking at the values written to the MSR_IA32_PERF_CTL
register.  Namely, the expected result would be that when all CPUs
were 100% busy, all of them would be requested to run in the maximum
P-state, but observation shows that this clearly isn't the case.
The CPUs run in the maximum P-state for a while and then are
requested to run slower and go back to the maximum P-state after
a while again.  That causes the actual frequency of the processor to
visibly oscillate below the sustainable maximum in a jittery fashion
which clearly is not desirable.

That has been attributed to CPU utilization metric updates on task
migration that cause the total utilization value for the CPU to be
reduced by the utilization of the migrated task.  If that happens,
the schedutil governor may see a CPU utilization reduction and will
attempt to reduce the CPU frequency accordingly right away.  That
may be premature, though, for example if the system is generally
busy and there are other runnable tasks waiting to be run on that
CPU already.

This is unlikely to be an issue on systems where cpufreq policies are
shared between multiple CPUs, because in those cases the policy
utilization is computed as the maximum of the CPU utilization values
over the whole policy and if that turns out to be low, reducing the
frequency for the policy most likely is a good idea anyway.  On
systems with one CPU per policy, however, it may affect performance
adversely and even lead to increased energy consumption in some cases.

On those systems it may be addressed by taking another utilization
metric into consideration, like whether or not the CPU whose
frequency is about to be reduced has been idle recently, because if
that's not the case, the CPU is likely to be busy in the near future
and its frequency should not be reduced.

To that end, use the counter of idle calls in the timekeeping code.
Namely, make the schedutil governor look at that counter for the
current CPU every time before its frequency is about to be reduced.
If the counter has not changed since the previous iteration of the
governor computations for that CPU, the CPU has been busy for all
that time and its frequency should not be decreased, so if the new
frequency would be lower than the one set previously, the governor
will skip the frequency update.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 include/linux/tick.h             |    1 +
 kernel/sched/cpufreq_schedutil.c |   27 +++++++++++++++++++++++++++
 kernel/time/tick-sched.c         |   12 ++++++++++++
 3 files changed, 40 insertions(+)

Index: linux-pm/kernel/sched/cpufreq_schedutil.c
===================================================================
--- linux-pm.orig/kernel/sched/cpufreq_schedutil.c
+++ linux-pm/kernel/sched/cpufreq_schedutil.c
@@ -61,6 +61,11 @@ struct sugov_cpu {
 	unsigned long util;
 	unsigned long max;
 	unsigned int flags;
+
+	/* The field below is for single-CPU policies only. */
+#ifdef CONFIG_NO_HZ_COMMON
+	unsigned long saved_idle_calls;
+#endif
 };
 
 static DEFINE_PER_CPU(struct sugov_cpu, sugov_cpu);
@@ -192,6 +197,19 @@ static void sugov_iowait_boost(struct su
 	sg_cpu->iowait_boost >>= 1;
 }
 
+#ifdef CONFIG_NO_HZ_COMMON
+static bool sugov_cpu_is_busy(struct sugov_cpu *sg_cpu)
+{
+	unsigned long idle_calls = tick_nohz_get_idle_calls();
+	bool ret = idle_calls == sg_cpu->saved_idle_calls;
+
+	sg_cpu->saved_idle_calls = idle_calls;
+	return ret;
+}
+#else
+static inline bool sugov_cpu_is_busy(struct sugov_cpu *sg_cpu) { return false; }
+#endif /* CONFIG_NO_HZ_COMMON */
+
 static void sugov_update_single(struct update_util_data *hook, u64 time,
 				unsigned int flags)
 {
@@ -200,6 +218,7 @@ static void sugov_update_single(struct u
 	struct cpufreq_policy *policy = sg_policy->policy;
 	unsigned long util, max;
 	unsigned int next_f;
+	bool busy;
 
 	sugov_set_iowait_boost(sg_cpu, time, flags);
 	sg_cpu->last_update = time;
@@ -207,12 +226,20 @@ static void sugov_update_single(struct u
 	if (!sugov_should_update_freq(sg_policy, time))
 		return;
 
+	busy = sugov_cpu_is_busy(sg_cpu);
+
 	if (flags & SCHED_CPUFREQ_RT_DL) {
 		next_f = policy->cpuinfo.max_freq;
 	} else {
 		sugov_get_util(&util, &max);
 		sugov_iowait_boost(sg_cpu, &util, &max);
 		next_f = get_next_freq(sg_policy, util, max);
+		/*
+		 * Do not reduce the frequency if the CPU has not been idle
+		 * recently, as the reduction is likely to be premature then.
+		 */
+		if (busy && next_f < sg_policy->next_freq)
+			next_f = sg_policy->next_freq;
 	}
 	sugov_update_commit(sg_policy, time, next_f);
 }
Index: linux-pm/include/linux/tick.h
===================================================================
--- linux-pm.orig/include/linux/tick.h
+++ linux-pm/include/linux/tick.h
@@ -117,6 +117,7 @@ extern void tick_nohz_idle_enter(void);
 extern void tick_nohz_idle_exit(void);
 extern void tick_nohz_irq_exit(void);
 extern ktime_t tick_nohz_get_sleep_length(void);
+extern unsigned long tick_nohz_get_idle_calls(void);
 extern u64 get_cpu_idle_time_us(int cpu, u64 *last_update_time);
 extern u64 get_cpu_iowait_time_us(int cpu, u64 *last_update_time);
 #else /* !CONFIG_NO_HZ_COMMON */
Index: linux-pm/kernel/time/tick-sched.c
===================================================================
--- linux-pm.orig/kernel/time/tick-sched.c
+++ linux-pm/kernel/time/tick-sched.c
@@ -993,6 +993,18 @@ ktime_t tick_nohz_get_sleep_length(void)
 	return ts->sleep_length;
 }
 
+/**
+ * tick_nohz_get_idle_calls - return the current idle calls counter value
+ *
+ * Called from the schedutil frequency scaling governor in scheduler context.
+ */
+unsigned long tick_nohz_get_idle_calls(void)
+{
+	struct tick_sched *ts = this_cpu_ptr(&tick_cpu_sched);
+
+	return ts->idle_calls;
+}
+
 static void tick_nohz_account_idle_ticks(struct tick_sched *ts)
 {
 #ifndef CONFIG_VIRT_CPU_ACCOUNTING_NATIVE

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

#1606301 — Re: [RFC][PATCH v3 2/2] cpufreq: schedutil: Avoid reducing frequency of busy CPUs prematurely

On Wed, Mar 22, 2017 at 12:08:50AM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> The way the schedutil governor uses the PELT metric causes it to
> underestimate the CPU utilization in some cases.
> 
> That can be easily demonstrated by running kernel compilation on
> a Sandy Bridge Intel processor, running turbostat in parallel with
> it and looking at the values written to the MSR_IA32_PERF_CTL
> register.  Namely, the expected result would be that when all CPUs
> were 100% busy, all of them would be requested to run in the maximum
> P-state, but observation shows that this clearly isn't the case.
> The CPUs run in the maximum P-state for a while and then are
> requested to run slower and go back to the maximum P-state after
> a while again.  That causes the actual frequency of the processor to
> visibly oscillate below the sustainable maximum in a jittery fashion
> which clearly is not desirable.
> 
> That has been attributed to CPU utilization metric updates on task
> migration that cause the total utilization value for the CPU to be
> reduced by the utilization of the migrated task.  If that happens,
> the schedutil governor may see a CPU utilization reduction and will
> attempt to reduce the CPU frequency accordingly right away.  That
> may be premature, though, for example if the system is generally
> busy and there are other runnable tasks waiting to be run on that
> CPU already.
> 
> This is unlikely to be an issue on systems where cpufreq policies are
> shared between multiple CPUs, because in those cases the policy
> utilization is computed as the maximum of the CPU utilization values
> over the whole policy and if that turns out to be low, reducing the
> frequency for the policy most likely is a good idea anyway.  On
> systems with one CPU per policy, however, it may affect performance
> adversely and even lead to increased energy consumption in some cases.
> 
> On those systems it may be addressed by taking another utilization
> metric into consideration, like whether or not the CPU whose
> frequency is about to be reduced has been idle recently, because if
> that's not the case, the CPU is likely to be busy in the near future
> and its frequency should not be reduced.
> 
> To that end, use the counter of idle calls in the timekeeping code.
> Namely, make the schedutil governor look at that counter for the
> current CPU every time before its frequency is about to be reduced.
> If the counter has not changed since the previous iteration of the
> governor computations for that CPU, the CPU has been busy for all
> that time and its frequency should not be decreased, so if the new
> frequency would be lower than the one set previously, the governor
> will skip the frequency update.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Right; this makes sense to me. Of course it would be good to have some
more measurements on this, but in principle:

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

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

#1606322 — Re: [RFC][PATCH v3 2/2] cpufreq: schedutil: Avoid reducing frequency of busy CPUs prematurely

On 22-03-17, 00:08, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> The way the schedutil governor uses the PELT metric causes it to
> underestimate the CPU utilization in some cases.
> 
> That can be easily demonstrated by running kernel compilation on
> a Sandy Bridge Intel processor, running turbostat in parallel with
> it and looking at the values written to the MSR_IA32_PERF_CTL
> register.  Namely, the expected result would be that when all CPUs
> were 100% busy, all of them would be requested to run in the maximum
> P-state, but observation shows that this clearly isn't the case.
> The CPUs run in the maximum P-state for a while and then are
> requested to run slower and go back to the maximum P-state after
> a while again.  That causes the actual frequency of the processor to
> visibly oscillate below the sustainable maximum in a jittery fashion
> which clearly is not desirable.
> 
> That has been attributed to CPU utilization metric updates on task
> migration that cause the total utilization value for the CPU to be
> reduced by the utilization of the migrated task.  If that happens,
> the schedutil governor may see a CPU utilization reduction and will
> attempt to reduce the CPU frequency accordingly right away.  That
> may be premature, though, for example if the system is generally
> busy and there are other runnable tasks waiting to be run on that
> CPU already.
> 
> This is unlikely to be an issue on systems where cpufreq policies are
> shared between multiple CPUs, because in those cases the policy
> utilization is computed as the maximum of the CPU utilization values
> over the whole policy and if that turns out to be low, reducing the
> frequency for the policy most likely is a good idea anyway.  On
> systems with one CPU per policy, however, it may affect performance
> adversely and even lead to increased energy consumption in some cases.
> 
> On those systems it may be addressed by taking another utilization
> metric into consideration, like whether or not the CPU whose
> frequency is about to be reduced has been idle recently, because if
> that's not the case, the CPU is likely to be busy in the near future
> and its frequency should not be reduced.
> 
> To that end, use the counter of idle calls in the timekeeping code.
> Namely, make the schedutil governor look at that counter for the
> current CPU every time before its frequency is about to be reduced.
> If the counter has not changed since the previous iteration of the
> governor computations for that CPU, the CPU has been busy for all
> that time and its frequency should not be decreased, so if the new
> frequency would be lower than the one set previously, the governor
> will skip the frequency update.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  include/linux/tick.h             |    1 +
>  kernel/sched/cpufreq_schedutil.c |   27 +++++++++++++++++++++++++++
>  kernel/time/tick-sched.c         |   12 ++++++++++++
>  3 files changed, 40 insertions(+)

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

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

#1607105 — Re: [RFC][PATCH v3 2/2] cpufreq: schedutil: Avoid reducing frequency of busy CPUs prematurely

On Tue, Mar 21, 2017 at 4:08 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> The way the schedutil governor uses the PELT metric causes it to
> underestimate the CPU utilization in some cases.
>
> That can be easily demonstrated by running kernel compilation on
> a Sandy Bridge Intel processor, running turbostat in parallel with
> it and looking at the values written to the MSR_IA32_PERF_CTL
> register.  Namely, the expected result would be that when all CPUs
> were 100% busy, all of them would be requested to run in the maximum
> P-state, but observation shows that this clearly isn't the case.
> The CPUs run in the maximum P-state for a while and then are
> requested to run slower and go back to the maximum P-state after
> a while again.  That causes the actual frequency of the processor to
> visibly oscillate below the sustainable maximum in a jittery fashion
> which clearly is not desirable.
>
> That has been attributed to CPU utilization metric updates on task
> migration that cause the total utilization value for the CPU to be
> reduced by the utilization of the migrated task.  If that happens,
> the schedutil governor may see a CPU utilization reduction and will
> attempt to reduce the CPU frequency accordingly right away.  That
> may be premature, though, for example if the system is generally
> busy and there are other runnable tasks waiting to be run on that
> CPU already.
>
> This is unlikely to be an issue on systems where cpufreq policies are
> shared between multiple CPUs, because in those cases the policy
> utilization is computed as the maximum of the CPU utilization values
> over the whole policy and if that turns out to be low, reducing the
> frequency for the policy most likely is a good idea anyway.  On
> systems with one CPU per policy, however, it may affect performance
> adversely and even lead to increased energy consumption in some cases.
>
> On those systems it may be addressed by taking another utilization
> metric into consideration, like whether or not the CPU whose
> frequency is about to be reduced has been idle recently, because if
> that's not the case, the CPU is likely to be busy in the near future
> and its frequency should not be reduced.
>
> To that end, use the counter of idle calls in the timekeeping code.
> Namely, make the schedutil governor look at that counter for the
> current CPU every time before its frequency is about to be reduced.
> If the counter has not changed since the previous iteration of the
> governor computations for that CPU, the CPU has been busy for all
> that time and its frequency should not be decreased, so if the new
> frequency would be lower than the one set previously, the governor
> will skip the frequency update.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Makes sense,

Reviewed-by: Joel Fernandes <joelaf@google.com>

Thanks,
Joel

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

#1607829 — Re: [RFC][PATCH v3 2/2] cpufreq: schedutil: Avoid reducing frequency of busy CPUs prematurely

Hi Rafael,

On 03/21/2017 04:08 PM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

<snip>

> 
> That has been attributed to CPU utilization metric updates on task
> migration that cause the total utilization value for the CPU to be
> reduced by the utilization of the migrated task.  If that happens,
> the schedutil governor may see a CPU utilization reduction and will
> attempt to reduce the CPU frequency accordingly right away.  That
> may be premature, though, for example if the system is generally
> busy and there are other runnable tasks waiting to be run on that
> CPU already.
> 
> This is unlikely to be an issue on systems where cpufreq policies are
> shared between multiple CPUs, because in those cases the policy
> utilization is computed as the maximum of the CPU utilization values
> over the whole policy and if that turns out to be low, reducing the
> frequency for the policy most likely is a good idea anyway.  On

I have observed this issue even in the shared policy case (one clock domain for many CPUs). On migrate, the actual load update is split into two updates:

1. Add to removed_load on src_cpu (cpu_util(src_cpu) not updated yet)
2. Do wakeup on dst_cpu, add load to dst_cpu

Now if src_cpu manages to do a PELT update before 2. happens, ex: say a small periodic task woke up on src_cpu, it'll end up subtracting the removed_load from its utilization and issue a frequency update before 2. happens.

This causes a premature dip in frequency which doesn't get corrected until the next util update that fires after rate_limit_us. The dst_cpu freq. update from step 2. above gets rate limited in this scenario.


> systems with one CPU per policy, however, it may affect performance
> adversely and even lead to increased energy consumption in some cases.
> 
> On those systems it may be addressed by taking another utilization
> metric into consideration, like whether or not the CPU whose
> frequency is about to be reduced has been idle recently, because if
> that's not the case, the CPU is likely to be busy in the near future
> and its frequency should not be reduced.
> 
> To that end, use the counter of idle calls in the timekeeping code.
> Namely, make the schedutil governor look at that counter for the
> current CPU every time before its frequency is about to be reduced.
> If the counter has not changed since the previous iteration of the
> governor computations for that CPU, the CPU has been busy for all
> that time and its frequency should not be decreased, so if the new
> frequency would be lower than the one set previously, the governor
> will skip the frequency update.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  include/linux/tick.h             |    1 +
>  kernel/sched/cpufreq_schedutil.c |   27 +++++++++++++++++++++++++++
>  kernel/time/tick-sched.c         |   12 ++++++++++++
>  3 files changed, 40 insertions(+)
> 
> Index: linux-pm/kernel/sched/cpufreq_schedutil.c
> ===================================================================
> --- linux-pm.orig/kernel/sched/cpufreq_schedutil.c
> +++ linux-pm/kernel/sched/cpufreq_schedutil.c
> @@ -61,6 +61,11 @@ struct sugov_cpu {
>  	unsigned long util;
>  	unsigned long max;
>  	unsigned int flags;
> +
> +	/* The field below is for single-CPU policies only. */
> +#ifdef CONFIG_NO_HZ_COMMON
> +	unsigned long saved_idle_calls;
> +#endif
>  };
>  
>  static DEFINE_PER_CPU(struct sugov_cpu, sugov_cpu);
> @@ -192,6 +197,19 @@ static void sugov_iowait_boost(struct su
>  	sg_cpu->iowait_boost >>= 1;
>  }
>  
> +#ifdef CONFIG_NO_HZ_COMMON
> +static bool sugov_cpu_is_busy(struct sugov_cpu *sg_cpu)
> +{
> +	unsigned long idle_calls = tick_nohz_get_idle_calls();
> +	bool ret = idle_calls == sg_cpu->saved_idle_calls;
> +
> +	sg_cpu->saved_idle_calls = idle_calls;
> +	return ret;
> +}

Hm, sorry I am a bit confused perhaps you could help me understand the problem/solution better :)

Say we have the this simple case of only a single periodic task running on one CPU, wouldn't the PELT update on wakeup cause a frequency update which updates the sg_cpu->saved_idle_calls value here? That would then cause the frequency update on idle entry to always skip dropping frequency right?

If I am reading this correctly, the PELT update on the dequeue for the periodic task (in the scenario above) happens _before_ the idle_calls++ which is in tick_nohz_idle_enter.

Thanks!
-Sai 

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

#1607909 — Re: [RFC][PATCH v3 2/2] cpufreq: schedutil: Avoid reducing frequency of busy CPUs prematurely

On 03/23/2017 12:26 PM, Sai Gurrappadi wrote:

> 
> Hm, sorry I am a bit confused perhaps you could help me understand the problem/solution better :)
> 
> Say we have the this simple case of only a single periodic task running on one CPU, wouldn't the PELT update on wakeup cause a frequency update which updates the sg_cpu->saved_idle_calls value here? That would then cause the frequency update on idle entry to always skip dropping frequency right?
 
Apologies, this example here is flawed because on task dequeue, its utilization isn't removed. There is no problem in this case...


-Sai

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

#1608065 — Re: [RFC][PATCH v3 2/2] cpufreq: schedutil: Avoid reducing frequency of busy CPUs prematurely

On Thu, Mar 23, 2017 at 8:26 PM, Sai Gurrappadi <sgurrappadi@nvidia.com> wrote:
> Hi Rafael,

Hi,

> On 03/21/2017 04:08 PM, Rafael J. Wysocki wrote:
>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> <snip>
>
>>
>> That has been attributed to CPU utilization metric updates on task
>> migration that cause the total utilization value for the CPU to be
>> reduced by the utilization of the migrated task.  If that happens,
>> the schedutil governor may see a CPU utilization reduction and will
>> attempt to reduce the CPU frequency accordingly right away.  That
>> may be premature, though, for example if the system is generally
>> busy and there are other runnable tasks waiting to be run on that
>> CPU already.
>>
>> This is unlikely to be an issue on systems where cpufreq policies are
>> shared between multiple CPUs, because in those cases the policy
>> utilization is computed as the maximum of the CPU utilization values
>> over the whole policy and if that turns out to be low, reducing the
>> frequency for the policy most likely is a good idea anyway.  On
>
> I have observed this issue even in the shared policy case (one clock domain for many CPUs). On migrate, the actual load update is split into two updates:
>
> 1. Add to removed_load on src_cpu (cpu_util(src_cpu) not updated yet)
> 2. Do wakeup on dst_cpu, add load to dst_cpu
>
> Now if src_cpu manages to do a PELT update before 2. happens, ex: say a small periodic task woke up on src_cpu, it'll end up subtracting the removed_load from its utilization and issue a frequency update before 2. happens.
>
> This causes a premature dip in frequency which doesn't get corrected until the next util update that fires after rate_limit_us. The dst_cpu freq. update from step 2. above gets rate limited in this scenario.

Interesting, and this seems to be related to last_freq_update_time
being per-policy (which it has to be, because frequency updates are
per-policy too and that's what we need to rate-limit).

Does this happen often enough to be a real concern in practice on
those configurations, though?

The other CPUs in the policy need to be either idle (so schedutil
doesn't take them into account at all) or lightly utilized for that to
happen, so that would affect workloads with one CPU hog type of task
that is migrated from one CPU to another within a policy and that
doesn't happen too often AFAICS.

Thanks,
Rafael

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

#1608904 — Re: [RFC][PATCH v3 2/2] cpufreq: schedutil: Avoid reducing frequency of busy CPUs prematurely

On 03/23/2017 06:39 PM, Rafael J. Wysocki wrote:
> On Thu, Mar 23, 2017 at 8:26 PM, Sai Gurrappadi <sgurrappadi@nvidia.com> wrote:
>> Hi Rafael,
> 
> Hi,
> 
>> On 03/21/2017 04:08 PM, Rafael J. Wysocki wrote:
>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>
>> <snip>
>>
>>>
>>> That has been attributed to CPU utilization metric updates on task
>>> migration that cause the total utilization value for the CPU to be
>>> reduced by the utilization of the migrated task.  If that happens,
>>> the schedutil governor may see a CPU utilization reduction and will
>>> attempt to reduce the CPU frequency accordingly right away.  That
>>> may be premature, though, for example if the system is generally
>>> busy and there are other runnable tasks waiting to be run on that
>>> CPU already.
>>>
>>> This is unlikely to be an issue on systems where cpufreq policies are
>>> shared between multiple CPUs, because in those cases the policy
>>> utilization is computed as the maximum of the CPU utilization values
>>> over the whole policy and if that turns out to be low, reducing the
>>> frequency for the policy most likely is a good idea anyway.  On
>>
>> I have observed this issue even in the shared policy case (one clock domain for many CPUs). On migrate, the actual load update is split into two updates:
>>
>> 1. Add to removed_load on src_cpu (cpu_util(src_cpu) not updated yet)
>> 2. Do wakeup on dst_cpu, add load to dst_cpu
>>
>> Now if src_cpu manages to do a PELT update before 2. happens, ex: say a small periodic task woke up on src_cpu, it'll end up subtracting the removed_load from its utilization and issue a frequency update before 2. happens.
>>
>> This causes a premature dip in frequency which doesn't get corrected until the next util update that fires after rate_limit_us. The dst_cpu freq. update from step 2. above gets rate limited in this scenario.
> 
> Interesting, and this seems to be related to last_freq_update_time
> being per-policy (which it has to be, because frequency updates are
> per-policy too and that's what we need to rate-limit).
> 

Correct.

> Does this happen often enough to be a real concern in practice on
> those configurations, though?
> 
> The other CPUs in the policy need to be either idle (so schedutil
> doesn't take them into account at all) or lightly utilized for that to
> happen, so that would affect workloads with one CPU hog type of task
> that is migrated from one CPU to another within a policy and that
> doesn't happen too often AFAICS.

So it is possible, even likely in some cases for a heavy CPU task to migrate on wakeup between the policy->cpus via select_idle_sibling() if the prev_cpu it was on was !idle on wakeup.

This style of heavy thread + lots of light work is a common pattern on Android (games, browsing, etc.) given how Android does its threading for ipc (Binder stuff) + its rendering/audio pipelines. 

I unfortunately don't have any numbers atm though.

-Sai

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

Page 3 of 4 — ← Prev page 1 2 [3] 4  Next page →

Back to top | Article view | linux.kernel

csiph-web