All of lore.kernel.org
 help / color / mirror / Atom feed
From: Doug Smythies <doug.smythies@gmail.com>
To: kristen@linux.intel.com, rjw@rjwysocki.net
Cc: dsmythies@telus.net, linux-pm@vger.kernel.org
Subject: [PATCH 0/5] intel_pstate: Use C0 time, calculate target pstate directly
Date: Sat, 11 Apr 2015 21:10:25 -0700	[thread overview]
Message-ID: <1428811830-15006-1-git-send-email-dsmythies@telus.net> (raw)

This patch set attempts to address some issues with
the intel_pstate driver:

The driver has a finicky tradeoffs between fixed point
integer math and gain settings and the setpoint.

The application is not really best suited to a PID
(Proportional Integral Derivative) controller.

The duration method can incorrectly drive the target
pstate downwards, because a long duration might not
be due to idle.

The driver can incorrectly drive the target pstate
upwards, when the C0 time is virtually 0.

The patch set brings back the use of C0 time as the driving
input as to what to do next in terms of target pstate.
Recall that C0 time was used once before and was eventually
removed due to situations of pstate lockup at a low pstate.
Why is this time better? Because it is scaled properly,
whereas it was not before. Also the response curve is
flexible and adjustable.

Patch 1 basically adds better debug information to the
perf data. It was originally requested in June, and Dirk agreed
to include it in September. Regardless of the acceptance or
rejection of this whole patch set, please accept this one.
scripts/checkpatch.pl complains about one line as over 80
characters, not sure how to split the line.

Patch 2 brings back the use of C0 time for the calculation
of core_busy. Floor and ceiling hold values can be adjusted
Once the floor load is reached the response is linear until
the ceiling load is reached. The equations are just a simple
y=mx+b line, where m = slope and b = intercept.
Equation 1: min = m * floor + b
Equation 2: max = m * ceiling + b
Two equations and two unknowns, solved and coded.
scripts/checkpatch.pl O.K.

Patch 3 bypasses the PID controller and calculates the
target pstate directly. An IIR (Infinite Impulse
Response) filter takes care of filtering.
Again a simple y=mx+b line.
Equation 1: pmin = m * min + b
Equation 2: pmax = m * max + b
Again, two simple equations and two unknowns, solved
and coded.
scripts/checkpatch.pl O.K.
Note: not the same min and max as in patch 2.

Note: There are two definitions of percent within
this driver, one goes to 100% at the max turbo (if
it is enabled) and the other is 100% at the max
non-turbo (enabled or not). It can be very confusing.
The min used in patch 3 must be the mathematical min
for the processor under the current turbo enabled or
disabled state, which might not be the same
as /sys/devices/system/cpu/intel_pstate/min_perf_pct
when turbo is enabled.

Patch 4 adds weighting to longer duration events to,
at least partially, compensate for the asymmetric
rising and falling load typical durations.
scripts/checkpatch.pl O.K.

Patch 5 just adjusts the default IIR gain.
10 percent gives a rising edge response time similar
to the current version of the driver.
scripts/checkpatch.pl O.K.

Anticipated questions:

Q: Shouldn't the response curve be normalized to a reference
pstate, perhaps the max non-turbo pstate?

A: Doing so can create a lockup scenario, depending on the
response curve settings and the overall pstate range of the
particular processor. A downside to not normalizing has not been
detected.

Q: One of the patches in the set is titled V2. What was V1?

A: To a limit, V1 ran the IIR filter N times when the duration
was N times nominal. The method was expensive in terms of
multiplies and divides. V2 saves a lot of clock cycles, and
is good enough.

Migration path:

It should not be assumed that the current pstate is actually
what was in effect during the last sample time. If a higher
pstate was actually used, due to another active CPU, then
that should override the use of the current pstate in the filter.
This issue also exists in the current version of the driver,
and was only discovered a few days ago. see also:
https://bugzilla.kernel.org/show_bug.cgi?id=93521#c55
This will be addressed, if possible, in a future patch.

There are likely some math (multiply and divide) savings that
can be realized in future patches.

While the use of the PID controller has been eliminated, and
because the change is likely to be controversial, the
code has been left intact. It will be removed in a future patch.

Doug Smythies (5):
  intel_pstate: Add tsc collection and keep previous target pstate. Add
    both to trace.
  intel_pstate: Use C0 time for busy calculations (again).
  intel_pstate: Calculate target pstate directly.
  intel_pstate: Compensate for intermediate durations (v2).
  intel_pstate: Adjust default IIR filter gain.

 Documentation/cpu-freq/intel-pstate.txt |   2 +
 drivers/cpufreq/intel_pstate.c          | 188 ++++++++++++++++++++++++--------
 include/trace/events/power.h            |  25 +++--
 3 files changed, 163 insertions(+), 52 deletions(-)

-- 
1.9.1


             reply	other threads:[~2015-04-12  4:10 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-04-12  4:10 Doug Smythies [this message]
2015-04-12  4:10 ` [PATCH 1/5] intel_pstate: Add tsc collection and keep previous target pstate. Add both to trace Doug Smythies
2015-04-29 16:57   ` Kristen Carlson Accardi
2015-05-04 23:34     ` Rafael J. Wysocki
2015-04-12  4:10 ` [PATCH 2/5] intel_pstate: Use C0 time for busy calculations (again) Doug Smythies
2015-05-06 19:20   ` Kristen Carlson Accardi
2015-05-07  6:17     ` Doug Smythies
2015-04-12  4:10 ` [PATCH 3/5] intel_pstate: Calculate target pstate directly Doug Smythies
2015-04-12  4:10 ` [PATCH 4/5] intel_pstate: Compensate for intermediate durations (v2) Doug Smythies
2015-04-12  4:10 ` [PATCH 5/5] intel_pstate: Adjust default IIR filter gain Doug Smythies
2015-04-13 16:32 ` [PATCH 0/5] intel_pstate: Use C0 time, calculate target pstate directly Kristen Carlson Accardi
2015-04-13 21:16   ` Doug Smythies
2015-04-16 16:35 ` Doug Smythies
2015-04-17  0:46   ` Rafael J. Wysocki

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1428811830-15006-1-git-send-email-dsmythies@telus.net \
    --to=doug.smythies@gmail.com \
    --cc=dsmythies@telus.net \
    --cc=kristen@linux.intel.com \
    --cc=linux-pm@vger.kernel.org \
    --cc=rjw@rjwysocki.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.