* [PATCH 2/7] cpufreq: intel_pstate: Avoid duplicate call of intel_pstate_get_scaled_busy
@ 2014-06-09 21:00 Stratos Karafotis
2014-06-10 16:05 ` Dirk Brandewie
0 siblings, 1 reply; 6+ messages in thread
From: Stratos Karafotis @ 2014-06-09 21:00 UTC (permalink / raw)
To: Rafael J. Wysocki, Viresh Kumar, Dirk Brandewie; +Cc: linux-pm, LKML
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 <stratosk@semaphore.gr>
---
drivers/cpufreq/intel_pstate.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
index 4e7f492..31e2ae5 100644
--- a/drivers/cpufreq/intel_pstate.c
+++ b/drivers/cpufreq/intel_pstate.c
@@ -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,
--
1.9.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/7] cpufreq: intel_pstate: Avoid duplicate call of intel_pstate_get_scaled_busy
2014-06-09 21:00 [PATCH 2/7] cpufreq: intel_pstate: Avoid duplicate call of intel_pstate_get_scaled_busy Stratos Karafotis
@ 2014-06-10 16:05 ` Dirk Brandewie
2014-06-10 16:26 ` Stratos Karafotis
0 siblings, 1 reply; 6+ messages in thread
From: Dirk Brandewie @ 2014-06-10 16:05 UTC (permalink / raw)
To: Stratos Karafotis, Rafael J. Wysocki, Viresh Kumar, Dirk Brandewie
Cc: dirk.brandewie, linux-pm, LKML
On 06/09/2014 02:00 PM, Stratos Karafotis wrote:
> 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()
> Also, rename the function to intel_pstate_calc_scaled_busy.
>
> Signed-off-by: Stratos Karafotis <stratosk@semaphore.gr>
> ---
> drivers/cpufreq/intel_pstate.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> index 4e7f492..31e2ae5 100644
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -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,
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/7] cpufreq: intel_pstate: Avoid duplicate call of intel_pstate_get_scaled_busy
2014-06-10 16:05 ` Dirk Brandewie
@ 2014-06-10 16:26 ` Stratos Karafotis
2014-06-14 15:45 ` Doug Smythies
0 siblings, 1 reply; 6+ messages in thread
From: Stratos Karafotis @ 2014-06-10 16:26 UTC (permalink / raw)
To: Dirk Brandewie, Rafael J. Wysocki, Viresh Kumar, Dirk Brandewie
Cc: linux-pm, LKML
On 10/06/2014 07:05 μμ, Dirk Brandewie wrote:
> On 06/09/2014 02:00 PM, Stratos Karafotis wrote:
>> 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.
> 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.
>> Also, rename the function to intel_pstate_calc_scaled_busy.
>>
>> Signed-off-by: Stratos Karafotis <stratosk@semaphore.gr>
>> ---
>> drivers/cpufreq/intel_pstate.c | 12 ++++++------
>> 1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
>> index 4e7f492..31e2ae5 100644
>> --- a/drivers/cpufreq/intel_pstate.c
>> +++ b/drivers/cpufreq/intel_pstate.c
>> @@ -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,
>>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH 2/7] cpufreq: intel_pstate: Avoid duplicate call of intel_pstate_get_scaled_busy
2014-06-10 16:26 ` Stratos Karafotis
@ 2014-06-14 15:45 ` Doug Smythies
0 siblings, 0 replies; 6+ messages in thread
From: Doug Smythies @ 2014-06-14 15:45 UTC (permalink / raw)
To: 'Stratos Karafotis', 'Dirk Brandewie',
'Rafael J. Wysocki', 'Viresh Kumar',
'Dirk Brandewie'
Cc: linux-pm, 'LKML'
I am sorry to be late chiming in on this one.
On 2014.06.10 09:27 Stratos Karafotis wrote:
> On 10/06/2014 07:05 μμ, Dirk Brandewie wrote:
> On 06/09/2014 02:00 PM, Stratos Karafotis wrote:
>> 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.
>> 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.
>>> Also, rename the function to intel_pstate_calc_scaled_busy.
>>>
>>> Signed-off-by: Stratos Karafotis <stratosk@semaphore.gr>
>>> ---
>>> drivers/cpufreq/intel_pstate.c | 12 ++++++------
>>> 1 file changed, 6 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
>>> index 4e7f492..31e2ae5 100644
>>> --- a/drivers/cpufreq/intel_pstate.c
>>> +++ b/drivers/cpufreq/intel_pstate.c
>>> @@ -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,
>>>
>>
>>
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH 2/7] cpufreq: intel_pstate: Avoid duplicate call of intel_pstate_get_scaled_busy
@ 2014-06-14 15:45 ` Doug Smythies
0 siblings, 0 replies; 6+ messages in thread
From: Doug Smythies @ 2014-06-14 15:45 UTC (permalink / raw)
To: 'Stratos Karafotis', 'Dirk Brandewie',
'Rafael J. Wysocki', 'Viresh Kumar',
'Dirk Brandewie'
Cc: linux-pm, 'LKML'
I am sorry to be late chiming in on this one.
On 2014.06.10 09:27 Stratos Karafotis wrote:
> On 10/06/2014 07:05 μμ, Dirk Brandewie wrote:
> On 06/09/2014 02:00 PM, Stratos Karafotis wrote:
>> 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.
>> 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.
>>> Also, rename the function to intel_pstate_calc_scaled_busy.
>>>
>>> Signed-off-by: Stratos Karafotis <stratosk@semaphore.gr>
>>> ---
>>> drivers/cpufreq/intel_pstate.c | 12 ++++++------
>>> 1 file changed, 6 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
>>> index 4e7f492..31e2ae5 100644
>>> --- a/drivers/cpufreq/intel_pstate.c
>>> +++ b/drivers/cpufreq/intel_pstate.c
>>> @@ -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,
>>>
>>
>>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/7] cpufreq: intel_pstate: Avoid duplicate call of intel_pstate_get_scaled_busy
2014-06-14 15:45 ` Doug Smythies
(?)
@ 2014-06-14 18:10 ` Stratos Karafotis
-1 siblings, 0 replies; 6+ messages in thread
From: Stratos Karafotis @ 2014-06-14 18:10 UTC (permalink / raw)
To: Doug Smythies, 'Dirk Brandewie',
'Rafael J. Wysocki', 'Viresh Kumar',
'Dirk Brandewie'
Cc: linux-pm, 'LKML'
On 14/06/2014 06:45 μμ, Doug Smythies wrote:
> I am sorry to be late chiming in on this one.
>
> On 2014.06.10 09:27 Stratos Karafotis wrote:
>> On 10/06/2014 07:05 μμ, Dirk Brandewie wrote:
>> On 06/09/2014 02:00 PM, Stratos Karafotis wrote:
>>> 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.
>
>>> 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.
>
I think you are right. Tracepoint data might be inconsistent.
I will re-submit this patch in v2 series, updating the changelog.
Thanks for pointing this out!
Stratos
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-06-14 18:10 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-06-09 21:00 [PATCH 2/7] cpufreq: intel_pstate: Avoid duplicate call of intel_pstate_get_scaled_busy Stratos Karafotis
2014-06-10 16:05 ` Dirk Brandewie
2014-06-10 16:26 ` Stratos Karafotis
2014-06-14 15:45 ` Doug Smythies
2014-06-14 15:45 ` Doug Smythies
2014-06-14 18:10 ` Stratos Karafotis
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.