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 2 of 4 — ← Prev page 1 [2] 3 4  Next page →

#1604515 — Re: [RFC][PATCH 2/2] cpufreq: schedutil: Force max frequency on busy CPUs

On Monday, March 20, 2017 11:36:45 AM Peter Zijlstra wrote:
> On Sun, Mar 19, 2017 at 02:34:32PM +0100, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> > The PELT metric used by the schedutil governor underestimates the
> > CPU utilization in some cases.  The reason for that may be time spent
> > in interrupt handlers and similar which is not accounted for by PELT.
> > 
> > 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, CPUs that are never idle should
> > always run at the maximum frequency and make that happen.
> > 
> > To that end, add a counter of idle calls to struct sugov_cpu and
> > modify cpuidle_idle_call() to increment that counter every time it
> > is about to put the given CPU into an idle state.  Next, make the
> > schedutil governor look at that counter for the current CPU every
> > time before it is about to start heavy computations.  If the counter
> > has not changed for over SUGOV_BUSY_THRESHOLD time (equal to 50 ms),
> > the CPU has not been idle for at least that long and the governor
> > will choose the maximum frequency for it without looking at the PELT
> > metric at all.
> 
> Why the time limit?

One iteration appeared to be a bit too aggressive, but honestly I think
I need to check again if this thing is regarded as viable at all.

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

#1604525 — Re: [RFC][PATCH 2/2] cpufreq: schedutil: Force max frequency on busy CPUs

On Mon, Mar 20, 2017 at 01:35:12PM +0100, Rafael J. Wysocki wrote:
> On Monday, March 20, 2017 11:36:45 AM Peter Zijlstra wrote:
> > On Sun, Mar 19, 2017 at 02:34:32PM +0100, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > 
> > > The PELT metric used by the schedutil governor underestimates the
> > > CPU utilization in some cases.  The reason for that may be time spent
> > > in interrupt handlers and similar which is not accounted for by PELT.
> > > 
> > > 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, CPUs that are never idle should
> > > always run at the maximum frequency and make that happen.
> > > 
> > > To that end, add a counter of idle calls to struct sugov_cpu and
> > > modify cpuidle_idle_call() to increment that counter every time it
> > > is about to put the given CPU into an idle state.  Next, make the
> > > schedutil governor look at that counter for the current CPU every
> > > time before it is about to start heavy computations.  If the counter
> > > has not changed for over SUGOV_BUSY_THRESHOLD time (equal to 50 ms),
> > > the CPU has not been idle for at least that long and the governor
> > > will choose the maximum frequency for it without looking at the PELT
> > > metric at all.
> > 
> > Why the time limit?
> 
> One iteration appeared to be a bit too aggressive, but honestly I think
> I need to check again if this thing is regarded as viable at all.
> 

I don't hate the idea; if we don't hit idle; we shouldn't shift down. I
just wonder if we don't already keep a idle-seqcount somewhere; NOHZ and
RCU come to mind as things that might already use something like that.

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

#1604540 — Re: [RFC][PATCH 2/2] cpufreq: schedutil: Force max frequency on busy CPUs

On 20-Mar 13:50, Peter Zijlstra wrote:
> On Mon, Mar 20, 2017 at 01:35:12PM +0100, Rafael J. Wysocki wrote:
> > On Monday, March 20, 2017 11:36:45 AM Peter Zijlstra wrote:
> > > On Sun, Mar 19, 2017 at 02:34:32PM +0100, Rafael J. Wysocki wrote:
> > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > 
> > > > The PELT metric used by the schedutil governor underestimates the
> > > > CPU utilization in some cases.  The reason for that may be time spent
> > > > in interrupt handlers and similar which is not accounted for by PELT.
> > > > 
> > > > 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, CPUs that are never idle should
> > > > always run at the maximum frequency and make that happen.
> > > > 
> > > > To that end, add a counter of idle calls to struct sugov_cpu and
> > > > modify cpuidle_idle_call() to increment that counter every time it
> > > > is about to put the given CPU into an idle state.  Next, make the
> > > > schedutil governor look at that counter for the current CPU every
> > > > time before it is about to start heavy computations.  If the counter
> > > > has not changed for over SUGOV_BUSY_THRESHOLD time (equal to 50 ms),
> > > > the CPU has not been idle for at least that long and the governor
> > > > will choose the maximum frequency for it without looking at the PELT
> > > > metric at all.
> > > 
> > > Why the time limit?
> > 
> > One iteration appeared to be a bit too aggressive, but honestly I think
> > I need to check again if this thing is regarded as viable at all.
> > 
> 
> I don't hate the idea; if we don't hit idle; we shouldn't shift down. I
> just wonder if we don't already keep a idle-seqcount somewhere; NOHZ and
> RCU come to mind as things that might already use something like that.

Maybe the problem is not going down (e.g. when there are only small
CFS tasks it makes perfectly sense) but instead not being fast enough
on rampin-up when a new RT task is activated.

And this boils down to two main point:
1) throttling for up transitions perhaps is only harmful
2) the call sites for schedutils updates are not properly positioned
   in specific scheduler decision points.

The proposed patch is adding yet another throttling mechanism, perhaps
on top of one which already needs to be improved.

-- 
#include <best/regards.h>

Patrick Bellasi

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

#1604570 — Re: [RFC][PATCH 2/2] cpufreq: schedutil: Force max frequency on busy CPUs

On Monday, March 20, 2017 01:06:15 PM Patrick Bellasi wrote:
> On 20-Mar 13:50, Peter Zijlstra wrote:
> > On Mon, Mar 20, 2017 at 01:35:12PM +0100, Rafael J. Wysocki wrote:
> > > On Monday, March 20, 2017 11:36:45 AM Peter Zijlstra wrote:
> > > > On Sun, Mar 19, 2017 at 02:34:32PM +0100, Rafael J. Wysocki wrote:
> > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > 
> > > > > The PELT metric used by the schedutil governor underestimates the
> > > > > CPU utilization in some cases.  The reason for that may be time spent
> > > > > in interrupt handlers and similar which is not accounted for by PELT.
> > > > > 
> > > > > 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, CPUs that are never idle should
> > > > > always run at the maximum frequency and make that happen.
> > > > > 
> > > > > To that end, add a counter of idle calls to struct sugov_cpu and
> > > > > modify cpuidle_idle_call() to increment that counter every time it
> > > > > is about to put the given CPU into an idle state.  Next, make the
> > > > > schedutil governor look at that counter for the current CPU every
> > > > > time before it is about to start heavy computations.  If the counter
> > > > > has not changed for over SUGOV_BUSY_THRESHOLD time (equal to 50 ms),
> > > > > the CPU has not been idle for at least that long and the governor
> > > > > will choose the maximum frequency for it without looking at the PELT
> > > > > metric at all.
> > > > 
> > > > Why the time limit?
> > > 
> > > One iteration appeared to be a bit too aggressive, but honestly I think
> > > I need to check again if this thing is regarded as viable at all.
> > > 
> > 
> > I don't hate the idea; if we don't hit idle; we shouldn't shift down. I
> > just wonder if we don't already keep a idle-seqcount somewhere; NOHZ and
> > RCU come to mind as things that might already use something like that.
> 
> Maybe the problem is not going down (e.g. when there are only small
> CFS tasks it makes perfectly sense) but instead not being fast enough
> on rampin-up when a new RT task is activated.
> 
> And this boils down to two main point:
> 1) throttling for up transitions perhaps is only harmful
> 2) the call sites for schedutils updates are not properly positioned
>    in specific scheduler decision points.
> 
> The proposed patch is adding yet another throttling mechanism, perhaps
> on top of one which already needs to be improved.

It is not throttling anything.

Thanks,
Rafael

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

#1604612 — Re: [RFC][PATCH 2/2] cpufreq: schedutil: Force max frequency on busy CPUs

On 20-Mar 14:05, Rafael J. Wysocki wrote:
> On Monday, March 20, 2017 01:06:15 PM Patrick Bellasi wrote:
> > On 20-Mar 13:50, Peter Zijlstra wrote:
> > > On Mon, Mar 20, 2017 at 01:35:12PM +0100, Rafael J. Wysocki wrote:
> > > > On Monday, March 20, 2017 11:36:45 AM Peter Zijlstra wrote:
> > > > > On Sun, Mar 19, 2017 at 02:34:32PM +0100, Rafael J. Wysocki wrote:
> > > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > > 
> > > > > > The PELT metric used by the schedutil governor underestimates the
> > > > > > CPU utilization in some cases.  The reason for that may be time spent
> > > > > > in interrupt handlers and similar which is not accounted for by PELT.
> > > > > > 
> > > > > > 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, CPUs that are never idle should
> > > > > > always run at the maximum frequency and make that happen.
> > > > > > 
> > > > > > To that end, add a counter of idle calls to struct sugov_cpu and
> > > > > > modify cpuidle_idle_call() to increment that counter every time it
> > > > > > is about to put the given CPU into an idle state.  Next, make the
> > > > > > schedutil governor look at that counter for the current CPU every
> > > > > > time before it is about to start heavy computations.  If the counter
> > > > > > has not changed for over SUGOV_BUSY_THRESHOLD time (equal to 50 ms),
> > > > > > the CPU has not been idle for at least that long and the governor
> > > > > > will choose the maximum frequency for it without looking at the PELT
> > > > > > metric at all.
> > > > > 
> > > > > Why the time limit?
> > > > 
> > > > One iteration appeared to be a bit too aggressive, but honestly I think
> > > > I need to check again if this thing is regarded as viable at all.
> > > > 
> > > 
> > > I don't hate the idea; if we don't hit idle; we shouldn't shift down. I
> > > just wonder if we don't already keep a idle-seqcount somewhere; NOHZ and
> > > RCU come to mind as things that might already use something like that.
> > 
> > Maybe the problem is not going down (e.g. when there are only small
> > CFS tasks it makes perfectly sense) but instead not being fast enough
> > on rampin-up when a new RT task is activated.
> > 
> > And this boils down to two main point:
> > 1) throttling for up transitions perhaps is only harmful
> > 2) the call sites for schedutils updates are not properly positioned
> >    in specific scheduler decision points.
> > 
> > The proposed patch is adding yet another throttling mechanism, perhaps
> > on top of one which already needs to be improved.
> 
> It is not throttling anything.

It's a kind-of...

-       if (flags & SCHED_CPUFREQ_RT_DL) { 
+       if ((flags & SCHED_CPUFREQ_RT_DL) || sugov_cpu_is_busy(sg_cpu)) {
                next_f = policy->cpuinfo.max_freq;

This check disregard any signal the scheduler can provide.

A 60% CFS task with a 100ms period, with such a policy will end up
running at the highest OPP for just 10% of its entire activation.
Moreover, when it completes, we are likely to enter an idle OPP while
still remaining at the highest OPP.

IMHO the ultimate goal of scheduitl should be that to be driven by the
scheduler, which has (or can have) all the required information to
support OPP selection.

If something is not working, well, then we should properly fix the
signals and/or provide (at least) a per-task tunable interface.

Adding an hardcoded threshold is an easy fix but it will ultimately
increase the complexity of the governor.

-- 
#include <best/regards.h>

Patrick Bellasi

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

#1604551 — Re: [RFC][PATCH 2/2] cpufreq: schedutil: Force max frequency on busy CPUs

On Monday, March 20, 2017 01:50:09 PM Peter Zijlstra wrote:
> On Mon, Mar 20, 2017 at 01:35:12PM +0100, Rafael J. Wysocki wrote:
> > On Monday, March 20, 2017 11:36:45 AM Peter Zijlstra wrote:
> > > On Sun, Mar 19, 2017 at 02:34:32PM +0100, Rafael J. Wysocki wrote:
> > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > 
> > > > The PELT metric used by the schedutil governor underestimates the
> > > > CPU utilization in some cases.  The reason for that may be time spent
> > > > in interrupt handlers and similar which is not accounted for by PELT.
> > > > 
> > > > 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, CPUs that are never idle should
> > > > always run at the maximum frequency and make that happen.
> > > > 
> > > > To that end, add a counter of idle calls to struct sugov_cpu and
> > > > modify cpuidle_idle_call() to increment that counter every time it
> > > > is about to put the given CPU into an idle state.  Next, make the
> > > > schedutil governor look at that counter for the current CPU every
> > > > time before it is about to start heavy computations.  If the counter
> > > > has not changed for over SUGOV_BUSY_THRESHOLD time (equal to 50 ms),
> > > > the CPU has not been idle for at least that long and the governor
> > > > will choose the maximum frequency for it without looking at the PELT
> > > > metric at all.
> > > 
> > > Why the time limit?
> > 
> > One iteration appeared to be a bit too aggressive, but honestly I think
> > I need to check again if this thing is regarded as viable at all.
> > 
> 
> I don't hate the idea; if we don't hit idle; we shouldn't shift down.

OK

> I just wonder if we don't already keep a idle-seqcount somewhere; NOHZ and
> RCU come to mind as things that might already use something like that.

NOHZ does that, but I did't want this to artificially depend on NOHZ.  That said,
yes, we can use that one too.

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

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

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 tried to address a few Peter's comments here and the result doesn't seem to
be too heavy-wieght.

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

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

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

On 20-03-17, 22:46, Rafael J. Wysocki wrote:
> Index: linux-pm/kernel/sched/cpufreq_schedutil.c

> +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;
> +

In the earlier version you said that we want to be opportunistic and
don't want to do heavy computation and so check only for current CPU.

But in this version, all those computations are already done by now.
Why shouldn't we check all CPUs in the policy now? I am asking as we
will still have the same problem, we are trying to work-around if the
current CPU isn't busy but others sharing the policy are.

Also, why not return directly from within the if block? To run
trace_cpu_frequency()?

I don't remember exactly, but why don't we run that for !fast-switch
case? We can simplify the code a bit if we check for no freq change at
the top of the routine.

-- 
viresh

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

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

On Tue, Mar 21, 2017 at 7:40 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 20-03-17, 22:46, Rafael J. Wysocki wrote:
>> Index: linux-pm/kernel/sched/cpufreq_schedutil.c
>
>> +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;
>> +
>
> In the earlier version you said that we want to be opportunistic and
> don't want to do heavy computation and so check only for current CPU.
>
> But in this version, all those computations are already done by now.
> Why shouldn't we check all CPUs in the policy now? I am asking as we
> will still have the same problem, we are trying to work-around if the
> current CPU isn't busy but others sharing the policy are.

This isn't the way I'm looking at that.

This is an easy (and relatively cheap) check to make for the *current*
*CPU* and our frequency selection algorithm turns out to have
problems, so it would be kind of unreasonable to not use the
opportunity to fix up the value coming from it - if we can do that
easily enough.

For the other CPUs in the policy that would require extra
synchronization etc., so not that easy any more.

> Also, why not return directly from within the if block? To run
> trace_cpu_frequency()?

Yes.

> I don't remember exactly, but why don't we run that for !fast-switch
> case?

That's an interesting question.

We do that in the fast switch case, because otherwise utilities get
confused if the frequency is not updated for a long enough time.

I'm not really sure why they don't get confused in the other case,
though.  [In that case the core calls trace_cpu_frequency() for us,
but only if we actually run the async work.]

It looks like it wouldn't hurt to always run trace_cpu_frequency()
when we want to bail out early for next_freq == sg_policy->next_freq.

Let me prepare a patch for that. :-)

> We can simplify the code a bit if we check for no freq change at
> the top of the routine.

Right.

Thanks,
Rafael

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

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

On 20 March 2017 at 22:46, 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.
>
> 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

Vincent
>
> 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 tried to address a few Peter's comments here and the result doesn't seem to
> be too heavy-wieght.
>
> 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
>

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

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

On 21-Mar 09:50, Vincent Guittot wrote:
> On 20 March 2017 at 22:46, 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.
> >
> > 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

Right, we can use "overloaded" for the time being until we push the
"overutilized" bits.

[...]

> > +#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;

Vincent: are you proposing something like this?

   +     if (this_rq()->rd->overload)
   +             return false;

> > +
> > +       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);
> >  }
> >

[...]

-- 
#include <best/regards.h>

Patrick Bellasi

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

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

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
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.

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

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

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

> 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 ?

>

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

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

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

>
>> > 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]

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

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;
 	} else {
 		sugov_get_util(&util, &max);
@@ -242,7 +243,7 @@ static unsigned int sugov_next_freq_shar
 			j_sg_cpu->iowait_boost = 0;
 			continue;
 		}
-		if (j_sg_cpu->flags & SCHED_CPUFREQ_RT_DL)
+		if ((j_sg_cpu->flags & SCHED_CPUFREQ_RT_DL) || j_sg_cpu->overload)
 			return policy->cpuinfo.max_freq;
 
 		j_util = j_sg_cpu->util;
@@ -273,12 +274,13 @@ 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;
 
 	if (sugov_should_update_freq(sg_policy, time)) {
-		if (flags & SCHED_CPUFREQ_RT_DL)
+		if ((flags & SCHED_CPUFREQ_RT_DL) || sg_cpu->overload)
 			next_f = sg_policy->policy->cpuinfo.max_freq;
 		else
 			next_f = sugov_next_freq_shared(sg_cpu);

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

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

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.

Previous solution was better IMO, apart from using overloaded instead
of overutilized (which is not yet there) :-/

>  	} else {
>  		sugov_get_util(&util, &max);
> @@ -242,7 +243,7 @@ static unsigned int sugov_next_freq_shar
>  			j_sg_cpu->iowait_boost = 0;
>  			continue;
>  		}
> -		if (j_sg_cpu->flags & SCHED_CPUFREQ_RT_DL)
> +		if ((j_sg_cpu->flags & SCHED_CPUFREQ_RT_DL) || j_sg_cpu->overload)
>  			return policy->cpuinfo.max_freq;
>  
>  		j_util = j_sg_cpu->util;
> @@ -273,12 +274,13 @@ 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;
>  
>  	if (sugov_should_update_freq(sg_policy, time)) {
> -		if (flags & SCHED_CPUFREQ_RT_DL)
> +		if ((flags & SCHED_CPUFREQ_RT_DL) || sg_cpu->overload)
>  			next_f = sg_policy->policy->cpuinfo.max_freq;
>  		else
>  			next_f = sugov_next_freq_shared(sg_cpu);
> 

-- 
#include <best/regards.h>

Patrick Bellasi

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

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

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.

---
 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;
 
 	/* 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;
+
 	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;

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

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

On Tue, Mar 21, 2017 at 03:46:07PM +0100, Rafael J. Wysocki wrote:
> @@ -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;
> +

Same problem as before; rd->overload is set if _any_ CPU in the root
domain has more than 1 runnable task at a random point in history (when
we ran the load balance tick -- and since that is the same tick used for
timers, there's a bias to over-account there).

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

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

On Tuesday, March 21, 2017 04:04:03 PM Peter Zijlstra wrote:
> On Tue, Mar 21, 2017 at 03:46:07PM +0100, Rafael J. Wysocki wrote:
> > @@ -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;
> > +
> 
> Same problem as before; rd->overload is set if _any_ CPU in the root
> domain has more than 1 runnable task at a random point in history (when
> we ran the load balance tick -- and since that is the same tick used for
> timers, there's a bias to over-account there).

OK

What about the one below then?

It checks both the idle calls count and overload and only then it will prevent
the frequency from being decreased.

It is sufficient for the case at hand.

I guess if rd->overload is not set, this means that none of the CPUs is
oversubscribed and we just saturate the capacity in a one-task-per-CPU kind
of fashion.  Right?

---
 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
@@ -37,6 +37,7 @@ struct sugov_policy {
 	s64 freq_update_delay_ns;
 	unsigned int next_freq;
 	unsigned int cached_raw_freq;
+	bool busy;
 
 	/* The next fields are only needed if fast switch cannot be used. */
 	struct irq_work irq_work;
@@ -56,11 +57,15 @@ 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;
 	unsigned long max;
 	unsigned int flags;
+	bool busy;
 };
 
 static DEFINE_PER_CPU(struct sugov_cpu, sugov_cpu);
@@ -93,6 +98,9 @@ static void sugov_update_commit(struct s
 {
 	struct cpufreq_policy *policy = sg_policy->policy;
 
+	if (sg_policy->busy && 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());
@@ -192,6 +200,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 not_idle = idle_calls == sg_cpu->saved_idle_calls;
+
+	sg_cpu->saved_idle_calls = idle_calls;
+	return not_idle && this_rq()->rd->overload;
+}
+#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)
 {
@@ -207,6 +228,8 @@ static void sugov_update_single(struct u
 	if (!sugov_should_update_freq(sg_policy, time))
 		return;
 
+	sg_policy->busy = sugov_cpu_is_busy(sg_cpu);
+
 	if (flags & SCHED_CPUFREQ_RT_DL) {
 		next_f = policy->cpuinfo.max_freq;
 	} else {
@@ -225,6 +248,8 @@ static unsigned int sugov_next_freq_shar
 	unsigned long util = 0, max = 1;
 	unsigned int j;
 
+	sg_policy->busy = 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 +278,7 @@ static unsigned int sugov_next_freq_shar
 		}
 
 		sugov_iowait_boost(j_sg_cpu, &util, &max);
+		sg_policy->busy = sg_policy->busy || sg_cpu->busy;
 	}
 
 	return get_next_freq(sg_policy, util, max);
@@ -273,6 +299,7 @@ static void sugov_update_shared(struct u
 	sg_cpu->util = util;
 	sg_cpu->max = max;
 	sg_cpu->flags = flags;
+	sg_cpu->busy = sugov_cpu_is_busy(sg_cpu);
 
 	sugov_set_iowait_boost(sg_cpu, time, flags);
 	sg_cpu->last_update = time;
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]

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

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?

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

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

Back to top | Article view | linux.kernel

csiph-web