cpufreq: intel_pstate: Avoid duplicate call of intel_pstate_get_scaled_busy

Store busy_scaled value to avoid to duplicate call of intel_pstate_get_scaled_busy on every sampling interval.

Also, rename the function to intel_pstate_calc_scaled_busy.

Signed-off-by Stratos Karafotis <[email protected]>

---
  drivers/cpufreq/intel_pstate.c | 12 ++++++------
  1 file changed, 6 insertions(+), 6 deletions(-)

drivers/cpufreq/intel_pstate.c +6 -6

index 4e7f492..31e2ae5 100644
@@ -55,6 +55,7 @@ static inline int32_t div_fp(int32_t x, int32_t y)
 
  struct sample {
  	int32_t core_pct_busy;
+	int32_t busy_scaled;
  	u64 aperf;
  	u64 mperf;
  	int freq;

@@ -604,7 +605,7 @@ static inline void intel_pstate_set_sample_time(struct cpudata *cpu)
  	mod_timer_pinned(&cpu->timer, jiffies + delay);
  }
 
-static inline int32_t intel_pstate_get_scaled_busy(struct cpudata *cpu)
+static inline void intel_pstate_calc_scaled_busy(struct cpudata *cpu)
  {
  	int32_t core_busy, max_pstate, current_pstate, sample_ratio;
  	u32 duration_us;

@@ -624,20 +625,19 @@ static inline int32_t intel_pstate_get_scaled_busy(struct cpudata *cpu)
  		core_busy = mul_fp(core_busy, sample_ratio);
  	}
 
-	return core_busy;
+	cpu->sample.busy_scaled = core_busy;
  }
 
  static inline void intel_pstate_adjust_busy_pstate(struct cpudata *cpu)
  {
-	int32_t busy_scaled;
  	struct _pid *pid;
  	signed int ctl = 0;
  	int steps;
 
  	pid = &cpu->pid;
-	busy_scaled = intel_pstate_get_scaled_busy(cpu);
+	intel_pstate_calc_scaled_busy(cpu);
 
-	ctl = pid_calc(pid, busy_scaled);
+	ctl = pid_calc(pid, cpu->sample.busy_scaled);
 
  	steps = abs(ctl);
 

@@ -659,7 +659,7 @@ static void intel_pstate_timer_func(unsigned long __data)
  	intel_pstate_adjust_busy_pstate(cpu);
 
  	trace_pstate_sample(fp_toint(sample->core_pct_busy),
-			fp_toint(intel_pstate_get_scaled_busy(cpu)),
+			fp_toint(sample->busy_scaled),
  			cpu->pstate.current_pstate,
  			sample->mperf,
  			sample->aperf,

Quoted 3 lines · Stratos Karafotis

Store busy_scaled value to avoid to duplicate call of intel_pstate_get_scaled_busy on every sampling interval.

The second call *only* happens if the tracepoint is being used otherwise the whole function call to trace_pstate_sample() is a noop.

This makes the code less readable IMHO the reader is left wondering how cpu->sample.busy_scaled was set in intel_pstate_adjust_busy_pstate()

Quoted 62 lines

Also, rename the function to intel_pstate_calc_scaled_busy.

Signed-off-by Stratos Karafotis <[email protected]>

---
  drivers/cpufreq/intel_pstate.c | 12 ++++++------
  1 file changed, 6 insertions(+), 6 deletions(-)

drivers/cpufreq/intel_pstate.c +6 -6

index 4e7f492..31e2ae5 100644
@@ -55,6 +55,7 @@ static inline int32_t div_fp(int32_t x, int32_t y)
 
  struct sample {
      int32_t core_pct_busy;
+    int32_t busy_scaled;
      u64 aperf;
      u64 mperf;
      int freq;

@@ -604,7 +605,7 @@ static inline void intel_pstate_set_sample_time(struct cpudata *cpu)
      mod_timer_pinned(&cpu->timer, jiffies + delay);
  }
 
-static inline int32_t intel_pstate_get_scaled_busy(struct cpudata *cpu)
+static inline void intel_pstate_calc_scaled_busy(struct cpudata *cpu)
  {
      int32_t core_busy, max_pstate, current_pstate, sample_ratio;
      u32 duration_us;

@@ -624,20 +625,19 @@ static inline int32_t intel_pstate_get_scaled_busy(struct cpudata *cpu)
          core_busy = mul_fp(core_busy, sample_ratio);
      }
 
-    return core_busy;
+    cpu->sample.busy_scaled = core_busy;
  }
 
  static inline void intel_pstate_adjust_busy_pstate(struct cpudata *cpu)
  {
-    int32_t busy_scaled;
      struct _pid *pid;
      signed int ctl = 0;
      int steps;
 
      pid = &cpu->pid;
-    busy_scaled = intel_pstate_get_scaled_busy(cpu);
+    intel_pstate_calc_scaled_busy(cpu);
 
-    ctl = pid_calc(pid, busy_scaled);
+    ctl = pid_calc(pid, cpu->sample.busy_scaled);
 
      steps = abs(ctl);
 

@@ -659,7 +659,7 @@ static void intel_pstate_timer_func(unsigned long __data)
      intel_pstate_adjust_busy_pstate(cpu);
 
      trace_pstate_sample(fp_toint(sample->core_pct_busy),
-            fp_toint(intel_pstate_get_scaled_busy(cpu)),
+            fp_toint(sample->busy_scaled),
              cpu->pstate.current_pstate,
              sample->mperf,
              sample->aperf,

On 10/06/2014 07:05 μμ, Dirk Brandewie wrote:

Quoted 6 lines · Stratos Karafotis

Store busy_scaled value to avoid to duplicate call of intel_pstate_get_scaled_busy on every sampling interval.

The second call *only* happens if the tracepoint is being used otherwise the whole function call to trace_pstate_sample() is a noop.

Yes, I'm sorry, I forgot to add this in my changelog. I have written this in cover letter.
I made this change mostly to support patch 3/7.

Quoted 3 lines

This makes the code less readable IMHO the reader is left wondering how cpu->sample.busy_scaled was set in intel_pstate_adjust_busy_pstate()

I agree that the the original code is more readable. If we don't care about the small overhead when tracing is on and forget patch 3/7, of course the original code is by far better.

Quoted 64 lines

Quoted 62 lines

Also, rename the function to intel_pstate_calc_scaled_busy.

Signed-off-by Stratos Karafotis <[email protected]>

---
  drivers/cpufreq/intel_pstate.c | 12 ++++++------
  1 file changed, 6 insertions(+), 6 deletions(-)

drivers/cpufreq/intel_pstate.c +6 -6

index 4e7f492..31e2ae5 100644
@@ -55,6 +55,7 @@ static inline int32_t div_fp(int32_t x, int32_t y)
 
  struct sample {
      int32_t core_pct_busy;
+    int32_t busy_scaled;
      u64 aperf;
      u64 mperf;
      int freq;

@@ -604,7 +605,7 @@ static inline void intel_pstate_set_sample_time(struct cpudata *cpu)
      mod_timer_pinned(&cpu->timer, jiffies + delay);
  }
 
-static inline int32_t intel_pstate_get_scaled_busy(struct cpudata *cpu)
+static inline void intel_pstate_calc_scaled_busy(struct cpudata *cpu)
  {
      int32_t core_busy, max_pstate, current_pstate, sample_ratio;
      u32 duration_us;

@@ -624,20 +625,19 @@ static inline int32_t intel_pstate_get_scaled_busy(struct cpudata *cpu)
          core_busy = mul_fp(core_busy, sample_ratio);
      }
 
-    return core_busy;
+    cpu->sample.busy_scaled = core_busy;
  }
 
  static inline void intel_pstate_adjust_busy_pstate(struct cpudata *cpu)
  {
-    int32_t busy_scaled;
      struct _pid *pid;
      signed int ctl = 0;
      int steps;
 
      pid = &cpu->pid;
-    busy_scaled = intel_pstate_get_scaled_busy(cpu);
+    intel_pstate_calc_scaled_busy(cpu);
 
-    ctl = pid_calc(pid, busy_scaled);
+    ctl = pid_calc(pid, cpu->sample.busy_scaled);
 
      steps = abs(ctl);
 

@@ -659,7 +659,7 @@ static void intel_pstate_timer_func(unsigned long __data)
      intel_pstate_adjust_busy_pstate(cpu);
 
      trace_pstate_sample(fp_toint(sample->core_pct_busy),
-            fp_toint(intel_pstate_get_scaled_busy(cpu)),
+            fp_toint(sample->busy_scaled),
              cpu->pstate.current_pstate,
              sample->mperf,
              sample->aperf,

I am sorry to be late chiming in on this one.

Quoted 13 lines · On 2014.06.10 09:27 Stratos Karafotis wrote:

On 10/06/2014 07:05 μμ, Dirk Brandewie wrote:

Quoted 6 lines · Stratos Karafotis

Store busy_scaled value to avoid to duplicate call of intel_pstate_get_scaled_busy on every sampling interval.

The second call *only* happens if the tracepoint is being used otherwise the whole function call to trace_pstate_sample() is a noop.

Yes, I'm sorry, I forgot to add this in my changelog. I have written this in cover letter.
I made this change mostly to support patch 3/7.

Quoted 7 lines

Quoted 3 lines

This makes the code less readable IMHO the reader is left wondering how cpu->sample.busy_scaled was set in intel_pstate_adjust_busy_pstate()

I agree that the the original code is more readable. If we don't care about the small overhead when tracing is on and forget patch 3/7, of course the original code is by far better.

Actually, when reading the code, I found it odd to call the function twice.

However by far the much more important issue here, in my opinion, is that if one is using the tracepoint stuff, then the second call to intel_pstate_get_scaled_busy can give a different result than the first call. Why? Because "cpu->pstate.current_pstate" may have changed between the two calls.

In the end the user (me in this case) of the tracepoint stuff can end up pulling (what's left of) their hair out and going around in circles attempting to figure out why doing the so simple math by hand doesn't seem to agree with the tracepoint data.

As a side note: I am now pulling the tracepoint data into a spreadsheet and calculating what "scaled" should be myself.