Bug 64271

Summary:	Intel Pstate driver - "kick" code is not needed
Product:	Power Management	Reporter:	Doug Smythies (dsmythies)
Component:	cpufreq	Assignee:	cpufreq
Status:	CLOSED CODE_FIX
Severity:	normal	CC:	aaron.lu, dirk.brandewie
Priority:	P1
Hardware:	All
OS:	Linux
Kernel Version:	3.12rc7	Subsystem:
Regression:	No	Bisected commit-id:

Description Doug Smythies 2013-11-02 22:17:51 UTC

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.

Comment 1 Doug Smythies 2013-11-03 21:13:50 UTC

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

Comment 2 Dirk Brandewie 2013-11-04 16:17:17 UTC

Agreed.  Do you want to submit the patch or should I?

Comment 3 Doug Smythies 2013-11-04 16:42:26 UTC

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

Comment 4 Dirk Brandewie 2013-11-04 17:01:13 UTC

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

Comment 5 Doug Smythies 2013-11-05 23:12:19 UTC

Dirk: Thanks.
Note that I have tested this to death already, and now I only run with a nokick version of intel_pstate.

Comment 6 Aaron Lu 2014-04-15 06:30:48 UTC

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.