[Bug 64271] New: Intel Pstate driver - "kick" code is not needed

[Bug 64271] New: Intel Pstate driver - "kick" code is not needed

[bugzilla-daemon]

https://bugzilla.kernel.org/show_bug.cgi?id=64271

            Bug ID: 64271
           Summary: Intel Pstate driver - "kick" code is not needed
           Product: Power Management
           Version: 2.5
    Kernel Version: 3.12rc7
          Hardware: All
                OS: Linux
              Tree: Mainline
            Status: NEW
          Severity: normal
          Priority: P1
         Component: cpufreq
          Assignee: cpufreq@vger.kernel.org
          Reporter: dsmythies@telus.net
        Regression: No

There is code in the routine intel_pstate_timer_func to "kick" the CPU out of
the minimum pstate.

The code was added back when several things were not working properly with the
driver. It was added due to a "ping-pong" issue uncovered by the Phoronix test
suite ffpmeg test. I have tried extensively to recreate the issue with the
"kick" code removed and have not been able to. I am unable to detect any
difference with the ffmpeg test between the normal code and code with the "kick
removed" The assertion is that the "kick" code is not required and should be
deleted.

This bug has been extracted from bug 59481 which ended up discussing a few
tangent issues. The main issue was solved and the bug closed. Now I am
following up on some of the tangent issues.

I am saying the kick portion of this code can be removed because it serves no
purpose and makes the code harder to read and understand:

static void intel_pstate_timer_func(unsigned long __data)
{
        struct cpudata *cpu = (struct cpudata *) __data;

        intel_pstate_sample(cpu);
        intel_pstate_adjust_busy_pstate(cpu);

        if (cpu->pstate.current_pstate == cpu->pstate.min_pstate) {
                cpu->min_pstate_count++;
                if (!(cpu->min_pstate_count % 5)) {
                        intel_pstate_set_pstate(cpu, cpu->pstate.max_pstate);
                }
        } else
                cpu->min_pstate_count = 0;

        intel_pstate_set_sample_time(cpu);
}

Where this was left on 2013.07.18 with Dirk Brandewie was that he wanted to go
back and verify in his test environment. Which, of course, should be done.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 64271] Intel Pstate driver - "kick" code is not needed

[bugzilla-daemon]

https://bugzilla.kernel.org/show_bug.cgi?id=64271

--- Comment #1 from Doug Smythies <dsmythies@telus.net> ---
Adding some phoronix ffmpeg test results:

Kernel: 3.12RC7 with No Kick modification.
Linux s15 3.12.0-rc7-nokick #49 SMP Sat Nov 2 21:40:01 PDT 2013 x86_64 x86_64
x86_64 GNU/Linux
Mode: Powersave with default settings.

Sample size 250 tests. (the limit seems to be 256)
Test run 1 is rejected as it includes extra time to load the test file. For
subsequent tests, the file is already cached.

ave    13.475713 seconds
max    13.70577598 seconds
min    13.29449391 seconds
stddev    0.073586378 seconds

Kernel: 3.12RC7
Linux s15 3.12.0-rc7+ #48 SMP Fri Nov 1 21:09:51 PDT 2013 x86_64 x86_64 x86_64
GNU/Linux
Mode: Performance with default settings.

Sample size 250 tests.
Test run 1 is NOT rejected as, due an error on my part, I restarted the test
and the so the test file was already cached.

ave    13.4565717 seconds
max    13.66979098 seconds
min    13.2799778 seconds
stddev    0.070516137 seconds

Kernel: 3.12RC7
Linux s15 3.12.0-rc7+ #48 SMP Fri Nov 1 21:09:51 PDT 2013 x86_64 x86_64 x86_64
GNU/Linux
Mode: Powersave with default settings.

Sample size 250 tests.
Test run 1 is NOT rejected as the test had been just run under different
conditions so the test file was already cached.

ave    13.45689662 seconds
min    13.64681292 seconds
max    13.30776 seconds
stddev    0.067261141 seconds

System Information

Hardware:
Processor: Intel Core i7-2600K @ 3.80GHz (8 Cores), Motherboard: ASUS P8Z68-M
PRO, Chipset: Intel 2nd Generation Core Family DRAM, Memory: 16384MB, Disk:
1000GB Seagate ST1000DM003-9YN1 + 1000GB Seagate ST1000DL002-9TT1 + 120GB INTEL
SSDSA2CW12, Graphics: Intel 2nd Generation Core Family IGP, Audio: Realtek
ALC892, Network: Intel 82574L Gigabit Connection

Software:
OS: Ubuntu 12.04, Kernel: 3.12.0-rc7+ (x86_64), Compiler: GCC 4.6, File-System:
ext4, Screen Resolution: 1680x1050

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 64271] Intel Pstate driver - "kick" code is not needed

[bugzilla-daemon]

https://bugzilla.kernel.org/show_bug.cgi?id=64271

Dirk Brandewie <dirk.brandewie@gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |dirk.brandewie@gmail.com

--- Comment #2 from Dirk Brandewie <dirk.brandewie@gmail.com> ---
Agreed.  Do you want to submit the patch or should I?

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 64271] Intel Pstate driver - "kick" code is not needed

[bugzilla-daemon]

https://bugzilla.kernel.org/show_bug.cgi?id=64271

--- Comment #3 from Doug Smythies <dsmythies@telus.net> ---
Dirk: I do not know how to submit a patch. Which doesn't mean I shouldn't learn
how, but I have also been cheating when I make my "no kick" test code by just
deleting the 3 lines that decide to and do the kick. The real solution would be
a bit more. I'll do it of you want (it would be a good way to get rid of me for
awhile (just joking)).

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 64271] Intel Pstate driver - "kick" code is not needed

[bugzilla-daemon]

https://bugzilla.kernel.org/show_bug.cgi?id=64271

--- Comment #4 from Dirk Brandewie <dirk.brandewie@gmail.com> ---
Here is the patch I will send upstream it may not make it until v3.14-rc1 since
the merge window is upon us :-)

commit 7d9aba45dbd356686eca097b31028eccec0e11d6
Author: Dirk Brandewie <dirk.j.brandewie@intel.com>
Date:   Mon Oct 14 07:54:45 2013 -0700

    cpufreq/intel_pstate: Remove periodic P state boost

    Remove the periodic P state boost.  This code required for some corner
    case benchmark tests.  The calculation of the required P state was
    incorrect/inaccurate and would not allow P state increase.

    This was fixed by a combination of commits:
      2134ed4 cpufreq / intel_pstate: Change to scale off of max P-state
      d253d2a intel_pstate: Improve accuracy by not truncating until final
result

    Fixes Bug:
      https://bugzilla.kernel.org/show_bug.cgi?id=64271

    Reported-by: dsmythies@telus.net
    Signed-off-by: Dirk Brandewie <dirk.j.brandewie@intel.com>
---
 drivers/cpufreq/intel_pstate.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
index 26ab4b8..4d07ae2 100644
--- a/drivers/cpufreq/intel_pstate.c
+++ b/drivers/cpufreq/intel_pstate.c
@@ -83,8 +83,6 @@ struct cpudata {
     struct pstate_data pstate;
     struct _pid pid;

-    int min_pstate_count;
-
     u64    prev_aperf;
     u64    prev_mperf;
     int    sample_ptr;
@@ -567,15 +565,6 @@ static void intel_pstate_timer_func(unsigned long __data)

     intel_pstate_sample(cpu);
     intel_pstate_adjust_busy_pstate(cpu);
-
-    if (cpu->pstate.current_pstate == cpu->pstate.min_pstate) {
-        cpu->min_pstate_count++;
-        if (!(cpu->min_pstate_count % 5)) {
-            intel_pstate_set_pstate(cpu, cpu->pstate.max_pstate);
-        }
-    } else
-        cpu->min_pstate_count = 0;
-
     intel_pstate_set_sample_time(cpu);
 }

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 64271] Intel Pstate driver - "kick" code is not needed

[bugzilla-daemon]

https://bugzilla.kernel.org/show_bug.cgi?id=64271

--- Comment #5 from Doug Smythies <dsmythies@telus.net> ---
Dirk: Thanks.
Note that I have tested this to death already, and now I only run with a nokick
version of intel_pstate.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 64271] Intel Pstate driver - "kick" code is not needed

[bugzilla-daemon]

https://bugzilla.kernel.org/show_bug.cgi?id=64271

Aaron Lu <aaron.lu@intel.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
                 CC|                            |aaron.lu@intel.com
         Resolution|---                         |CODE_FIX

--- Comment #6 from Aaron Lu <aaron.lu@intel.com> ---
commit 91a4cd4f3d8169d7398f9123683f64575927c682
Author: Dirk Brandewie <dirk.j.brandewie@intel.com>
Date:   Tue Dec 17 09:42:07 2013 -0800

    intel_pstate: Remove periodic P state boost

entered Linus' tree as v3.14 material.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 64271] Intel Pstate driver - "kick" code is not needed

[bugzilla-daemon]

https://bugzilla.kernel.org/show_bug.cgi?id=64271

Aaron Lu <aaron.lu@intel.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

-- 
You are receiving this mail because:
You are the assignee for the bug.